Re: [PATCHES] Bad bug in fopen() wrapper code

Поиск
Список
Период
Сортировка
Искать

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
"Magnus Hagander"  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

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
"Claudio Natoli"  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 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.

			regards, tom lane

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
Bruce Momjian <bruce@momjian.us>
Дата:
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;
  }

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
Bruce Momjian <bruce@momjian.us>
Дата:
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;
  }

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
Bruce Momjian <bruce@momjian.us>
Дата:

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. +

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
"Zeugswetter Andreas DCP SD"  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

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
"Magnus Hagander" <mha@sollentuna.net>
Дата:
> > > > > 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

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
"Magnus Hagander" <mha@sollentuna.net>
Дата:
> > > > 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

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
"Magnus Hagander" <mha@sollentuna.net>
Дата:
> >> 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

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
"Claudio Natoli" <claudio.natoli@memetrics.com>
Дата:

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 failure of the _open_osfhandle, obviously)

Cheers,
Claudio

Re: [PATCHES] Bad bug in fopen() wrapper code

От:
"Zeugswetter Andreas DCP SD" <ZeugswetterA@spardat.at>
Дата:

> 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
FAQ