Thread: Re: [PATCHES] Bad bug in fopen() wrapper code
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. +