Thread: pgsql: Fix race condition in pg_ctl reading postmaster.pid.
Fix race condition in pg_ctl reading postmaster.pid. If postmaster changed postmaster.pid while pg_ctl was reading it, pg_ctl could overrun the buffer it allocated for the file. Fix by reading the whole file to memory with one read() call. initdb contains an identical copy of the readfile() function, but the files that initdb reads are static, not modified concurrently. Nevertheless, add a simple bounds-check there, if only to silence static analysis tools. Per report from Dave Vitek. Backpatch to all supported branches. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/5c89684e08cda82727bd8bdad155b9235fb7246e Modified Files -------------- src/bin/initdb/initdb.c | 9 +++-- src/bin/pg_ctl/pg_ctl.c | 88 +++++++++++++++++++++++++++++++---------------- 2 files changed, 63 insertions(+), 34 deletions(-)
Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
> Fix race condition in pg_ctl reading postmaster.pid.
> If postmaster changed postmaster.pid while pg_ctl was reading it, pg_ctl
> could overrun the buffer it allocated for the file. Fix by reading the
> whole file to memory with one read() call.
Maybe I'm just not awake enough, but that code doesn't look to me like
it does the right thing with a non-newline-terminated file. Doesn't it
drop the last character of the last line?
Given the way that pg_ctl uses the file, I think that the old logic of
"pretend the file ends with a newline" is wrong anyway. If we do manage
to see an intermediate state of the file, it would be better to not
return the last line at all than to return a truncated (a/k/a wrong)
value for that line. So I'd vote to rejigger the logic to ignore any
data after the last newline.
regards, tom lane
On 15.10.2012 17:05, Tom Lane wrote: > Heikki Linnakangas<heikki.linnakangas@iki.fi> writes: >> Fix race condition in pg_ctl reading postmaster.pid. >> If postmaster changed postmaster.pid while pg_ctl was reading it, pg_ctl >> could overrun the buffer it allocated for the file. Fix by reading the >> whole file to memory with one read() call. > > Maybe I'm just not awake enough, but that code doesn't look to me like > it does the right thing with a non-newline-terminated file. Doesn't it > drop the last character of the last line? No. It loops until the next-to-last character when it searches for newlines. When it hits that, it creates the final string to return, which includes the final character, whether it's a newline or not. BTW, an easy way to test this is to edit postmaster.opts on a running server, and do "pg_ctl status". It prints out the contents of postmaster.opts, as parsed by the function. > Given the way that pg_ctl uses the file, I think that the old logic of > "pretend the file ends with a newline" is wrong anyway. If we do manage > to see an intermediate state of the file, it would be better to not > return the last line at all than to return a truncated (a/k/a wrong) > value for that line. So I'd vote to rejigger the logic to ignore any > data after the last newline. Good point. I'll rejigger.. - Heikki