Thread: Bad bug in fopen() wrapper code
There is a small bug in the fopen() wrapper code that was applied a couple of weeks back for win32. It sets the wrong flags for files opened in "append" mode. This makes the logfile writing not work - syslog.c opens the logfile in append mode, but if the file does not already exist, it will not be opened and an error is returned - causing the postmaster to terminate. This is pretty bad and pretty urgent - with this, systems installed by the MSI installer simply *do not start*, because they are by default configured to write logs to a file... Attached patch sets the O_CREAT option when appending to files. //Ma <<open.diff>> gnus
Attachment
"Magnus Hagander" <mha@sollentuna.net> writes:
> Attached patch sets the O_CREAT option when appending to files.
That looks correct, but I went looking to see if there were any other
mistakes of the same ilk, and I'm wondering what the sense is in
openFlagsToCreateFileFlags ... seems like it's ignoring O_EXCL in
some combinations and not others. Is that right?
regards, tom lane
> > Attached patch sets the O_CREAT option when appending to files. > > That looks correct, but I went looking to see if there were > any other mistakes of the same ilk, and I'm wondering what > the sense is in openFlagsToCreateFileFlags ... seems like > it's ignoring O_EXCL in some combinations and not others. Is > that right? That is part of the original open() code that Claudio did back for 8.0, so it has definitly been working since then. I haven't really read into the code, though... But a qiuck look doesn't show me any place wher eit does ignore O_EXCL - which combination would that be? //Magnus
"Magnus Hagander" <mha@sollentuna.net> writes:
> That is part of the original open() code that Claudio did back for 8.0,
> so it has definitly been working since then.
Hm, maybe best not to touch it, but still...
> I haven't really read into
> the code, though... But a qiuck look doesn't show me any place wher eit
> does ignore O_EXCL - which combination would that be?
What's bugging me is that 0 and O_EXCL give the same answer, and
O_TRUNC and O_TRUNC | O_EXCL give the same answer, but O_CREAT and
O_CREAT | O_EXCL give different answers, as do O_CREAT | O_TRUNC
and O_CREAT | O_TRUNC | O_EXCL. I'm also pretty suspicious of
both O_CREAT | O_EXCL and O_CREAT | O_TRUNC | O_EXCL giving the
same answer. However, I have no idea what the semantics are of
the symbols the function is mapping into, so maybe it's OK.
regards, tom lane
"Magnus Hagander" <mha@sollentuna.net> writes:
> This is pretty bad and pretty urgent - with this, systems installed by
> the MSI installer simply *do not start*, because they are by default
> configured to write logs to a file...
> Attached patch sets the O_CREAT option when appending to files.
Applied, but I'm still wondering about openFlagsToCreateFileFlags() ...
regards, tom lane
Hello guys, it's been a while, but... > What's bugging me is that 0 and O_EXCL give the same answer, and > O_TRUNC and O_TRUNC | O_EXCL give the same answer, This is ok, as (iirc) O_EXCL only has effect in the presence of O_CREAT. (a comment to this effect would help here, as well as perhaps links to the CreateFile and open specs) > but O_CREAT and O_CREAT | O_EXCL give different answers, > as do O_CREAT | O_TRUNC and O_CREAT | O_TRUNC | O_EXCL. See above. > I'm also pretty suspicious of both O_CREAT | O_EXCL and > O_CREAT | O_TRUNC | O_EXCL giving the same answer. O_CREAT | O_EXCL is effectively "create, but fail if the file exists", which is the behaviour specified by CREATE_NEW. AddingO_TRUNC to this combination, which can only apply to a successfully opened existent file, can have no impact afaics. Cheers, Claudio
> > What's bugging me is that 0 and O_EXCL give the same answer, and
> > O_TRUNC and O_TRUNC | O_EXCL give the same answer,
>
> This is ok, as (iirc) O_EXCL only has effect in the presence
> of O_CREAT.
<snip more explanation>
Thanks, Claudio!
After looking at the code some more, and actually reading up on the
specs a bit more, it certainly does look like it's safe. So I don't
think we need to do anything about that.
Now, I still twist my head around the lines:
if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
||
(fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd,
fileFlags & (O_TEXT | O_BINARY)) < 0)))
With the _setmode() call deep in the if statement... I would suggest we
split that up into a couple of lines to make it more readable - I'm sure
all compilers will easily optimise it into the same code anyway.
Reasonable?
//Magnus
Magnus Hagander writes: > Now, I still twist my head around the lines: > if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 > || > (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, > fileFlags & (O_TEXT | O_BINARY)) < 0))) > > > With the _setmode() call deep in the if statement... I would suggest we > split that up into a couple of lines to make it more readable - I'm sure > all compilers will easily optimise it into the same code anyway. > Reasonable? I agree it would be clearer if split up. Without having studied it closely, it might also highlight a bug on failure of the second clause -- if the _setmode fails,shouldn't _close be called instead of CloseHandle, and -1 returned? (CloseHandle would still be called on failureof the _open_osfhandle, obviously) Cheers, Claudio
"Claudio Natoli" <claudio.natoli@memetrics.com> writes:
> Magnus Hagander writes:
>> Now, I still twist my head around the lines:
>> if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0
>> ||
>> (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd,
>> fileFlags & (O_TEXT | O_BINARY)) < 0)))
> Without having studied it closely, it might also highlight a bug on failure of the second clause -- if the _setmode
fails,shouldn't _close be called instead of CloseHandle, and -1 returned? (CloseHandle would still be called on
failureof the _open_osfhandle, obviously)
I agree that this code is both wrong and unreadable (although in
practice the _setmode will probably never fail, which is why our
attention hasn't been drawn to it). Is someone going to submit a
patch? I'm hesitant to change the code myself since I'm not in
a position to test it.
regards, tom lane
> >> Now, I still twist my head around the lines: > >> if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 > >> || > >> (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags & > (O_TEXT > >> | O_BINARY)) < 0))) > > > Without having studied it closely, it might also highlight a bug > on > > failure of the second clause -- if the _setmode fails, shouldn't > > _close be called instead of CloseHandle, and -1 returned? > > (CloseHandle would still be called on failure of the > _open_osfhandle, > > obviously) > > I agree that this code is both wrong and unreadable (although in > practice the _setmode will probably never fail, which is why our > attention hasn't been drawn to it). Is someone going to submit a > patch? I'm hesitant to change the code myself since I'm not in a > position to test it. I can look at fixing that. Is it something we want to do for 8.2, or wait until 8.3? If there's a hidden bug, I guess 8.2? //Magnus
"Magnus Hagander" <mha@sollentuna.net> writes:
>> I agree that this code is both wrong and unreadable (although in
>> practice the _setmode will probably never fail, which is why our
>> attention hasn't been drawn to it). Is someone going to submit a
>> patch? I'm hesitant to change the code myself since I'm not in a
>> position to test it.
> I can look at fixing that. Is it something we want to do for 8.2, or
> wait until 8.3? If there's a hidden bug, I guess 8.2?
Yeah, I think it's 8.2 material.
regards, tom lane
Magnus Hagander wrote: > > >> Now, I still twist my head around the lines: > > >> if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 > > >> || > > >> (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags & > > (O_TEXT > > >> | O_BINARY)) < 0))) > > > > > Without having studied it closely, it might also highlight a bug > > on > > > failure of the second clause -- if the _setmode fails, shouldn't > > > _close be called instead of CloseHandle, and -1 returned? > > > (CloseHandle would still be called on failure of the > > _open_osfhandle, > > > obviously) > > > > I agree that this code is both wrong and unreadable (although in > > practice the _setmode will probably never fail, which is why our > > attention hasn't been drawn to it). Is someone going to submit a > > patch? I'm hesitant to change the code myself since I'm not in a > > position to test it. > > I can look at fixing that. Is it something we want to do for 8.2, or > wait until 8.3? If there's a hidden bug, I guess 8.2? Magnus, is this the right fix? -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/port/open.c =================================================================== RCS file: /cvsroot/pgsql/src/port/open.c,v retrieving revision 1.15 diff -c -c -r1.15 open.c *** src/port/open.c 24 Sep 2006 17:19:53 -0000 1.15 --- src/port/open.c 3 Oct 2006 01:16:43 -0000 *************** *** 105,113 **** } /* _open_osfhandle will, on error, set errno accordingly */ ! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 || ! (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0))) CloseHandle(h); /* will not affect errno */ return fd; } --- 105,119 ---- } /* _open_osfhandle will, on error, set errno accordingly */ ! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0) CloseHandle(h); /* will not affect errno */ + else if (fileFlags & (O_TEXT | O_BINARY) && + _setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0) + { + _close(fd) /* will not affect errno */ + return -1; + } + return fd; }
Bruce Momjian wrote: > Magnus Hagander wrote: > > > >> Now, I still twist my head around the lines: > > > >> if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 > > > >> || > > > >> (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags & > > > (O_TEXT > > > >> | O_BINARY)) < 0))) > > > > > > > Without having studied it closely, it might also highlight a bug > > > on > > > > failure of the second clause -- if the _setmode fails, shouldn't > > > > _close be called instead of CloseHandle, and -1 returned? > > > > (CloseHandle would still be called on failure of the > > > _open_osfhandle, > > > > obviously) > > > > > > I agree that this code is both wrong and unreadable (although in > > > practice the _setmode will probably never fail, which is why our > > > attention hasn't been drawn to it). Is someone going to submit a > > > patch? I'm hesitant to change the code myself since I'm not in a > > > position to test it. > > > > I can look at fixing that. Is it something we want to do for 8.2, or > > wait until 8.3? If there's a hidden bug, I guess 8.2? > > Magnus, is this the right fix? Claudio sent me some small updates to the patch; updated version attached. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/port/open.c =================================================================== RCS file: /cvsroot/pgsql/src/port/open.c,v retrieving revision 1.15 diff -c -c -r1.15 open.c *** src/port/open.c 24 Sep 2006 17:19:53 -0000 1.15 --- src/port/open.c 3 Oct 2006 01:16:43 -0000 *************** *** 105,113 **** } /* _open_osfhandle will, on error, set errno accordingly */ ! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 || ! (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0))) CloseHandle(h); /* will not affect errno */ return fd; } --- 105,119 ---- } /* _open_osfhandle will, on error, set errno accordingly */ ! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0) CloseHandle(h); /* will not affect errno */ + else if (fileFlags & (O_TEXT | O_BINARY) && + _setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0) + { + _close(fd); + return -1; + } + return fd; }
> > > > > Without having studied it closely, it might also
> highlight a bug
> > > > on
> > > > > failure of the second clause -- if the _setmode
> fails, shouldn't
> > > > > _close be called instead of CloseHandle, and -1 returned?
> > > > > (CloseHandle would still be called on failure of the
> > > > _open_osfhandle,
> > > > > obviously)
> > > >
> > > > I agree that this code is both wrong and unreadable
> (although in
> > > > practice the _setmode will probably never fail, which
> is why our
> > > > attention hasn't been drawn to it). Is someone going
> to submit a
> > > > patch? I'm hesitant to change the code myself since
> I'm not in a
> > > > position to test it.
> > >
> > > I can look at fixing that. Is it something we want to do
> for 8.2, or
> > > wait until 8.3? If there's a hidden bug, I guess 8.2?
> >
> > Magnus, is this the right fix?
>
> Claudio sent me some small updates to the patch; updated
> version attached.
If we're going for maximum readability, I'd actually split
+ else if (fileFlags & (O_TEXT | O_BINARY) &&
+ _setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0)
into two different if statements as wlel. Ee.g.
else if (fileFlags (O_TEXT | O_BINARY)) {
if (_setmode() < 0) {
failure();
}
}
Haven't tested the patch yet, but it looks ok.
//Magnus
> Magnus, is this the right fix? Well, actually msdn states: "Return Value If successful, _setmode returns the previous translation mode. A return value of -1 indicates an error" So, shouldn't we be testing for -1 instead of < 0 ? The thing is probably academic, since _setmode is only supposed to fail on invalid file handle or invalid mode. So basically, given our code, it should only fail if filemode is (O_BINARY | O_TEXT) both flags set. Andreas
"Zeugswetter Andreas DCP SD" <ZeugswetterA@spardat.at> writes:
> "If successful, _setmode returns the previous translation mode. A return
> value of -1 indicates an error"
> So, shouldn't we be testing for -1 instead of < 0 ?
I think the usual convention is to test for < 0, unless there are other
negative return values that are legal. This is doubtless a silly
cycle-shaving habit (on nearly all machines, test against 0 is a bit
more compact than test against other constants), but it is a widespread
habit anyway, and if you sometimes do it one way and sometimes another
you just create a distraction for readers.
regards, tom lane
> > > > I agree that this code is both wrong and unreadable > (although in > > > > practice the _setmode will probably never fail, which > is why our > > > > attention hasn't been drawn to it). Is someone going > to submit a > > > > patch? I'm hesitant to change the code myself since > I'm not in a > > > > position to test it. > > > > > > I can look at fixing that. Is it something we want to do > for 8.2, or > > > wait until 8.3? If there's a hidden bug, I guess 8.2? > > > > Magnus, is this the right fix? > > Claudio sent me some small updates to the patch; updated > version attached. Tested, passes all regression tests on MingW. //Magnus
Applied. --------------------------------------------------------------------------- Bruce Momjian wrote: > Bruce Momjian wrote: > > Magnus Hagander wrote: > > > > >> Now, I still twist my head around the lines: > > > > >> if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 > > > > >> || > > > > >> (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags & > > > > (O_TEXT > > > > >> | O_BINARY)) < 0))) > > > > > > > > > Without having studied it closely, it might also highlight a bug > > > > on > > > > > failure of the second clause -- if the _setmode fails, shouldn't > > > > > _close be called instead of CloseHandle, and -1 returned? > > > > > (CloseHandle would still be called on failure of the > > > > _open_osfhandle, > > > > > obviously) > > > > > > > > I agree that this code is both wrong and unreadable (although in > > > > practice the _setmode will probably never fail, which is why our > > > > attention hasn't been drawn to it). Is someone going to submit a > > > > patch? I'm hesitant to change the code myself since I'm not in a > > > > position to test it. > > > > > > I can look at fixing that. Is it something we want to do for 8.2, or > > > wait until 8.3? If there's a hidden bug, I guess 8.2? > > > > Magnus, is this the right fix? > > Claudio sent me some small updates to the patch; updated version > attached. > > -- > Bruce Momjian bruce@momjian.us > EnterpriseDB http://www.enterprisedb.com > > + If your life is a hard drive, Christ can be your backup. + [ text/x-diff is unsupported, treating like TEXT/PLAIN ] > Index: src/port/open.c > =================================================================== > RCS file: /cvsroot/pgsql/src/port/open.c,v > retrieving revision 1.15 > diff -c -c -r1.15 open.c > *** src/port/open.c 24 Sep 2006 17:19:53 -0000 1.15 > --- src/port/open.c 3 Oct 2006 01:16:43 -0000 > *************** > *** 105,113 **** > } > > /* _open_osfhandle will, on error, set errno accordingly */ > ! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0 || > ! (fileFlags & (O_TEXT | O_BINARY) && (_setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0))) > CloseHandle(h); /* will not affect errno */ > return fd; > } > > --- 105,119 ---- > } > > /* _open_osfhandle will, on error, set errno accordingly */ > ! if ((fd = _open_osfhandle((long) h, fileFlags & O_APPEND)) < 0) > CloseHandle(h); /* will not affect errno */ > + else if (fileFlags & (O_TEXT | O_BINARY) && > + _setmode(fd, fileFlags & (O_TEXT | O_BINARY)) < 0) > + { > + _close(fd); > + return -1; > + } > + > return fd; > } > -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +