Re: [HACKERS] Bad bug in fopen() wrapper code - Mailing list pgsql-patches

From Bruce Momjian
Subject Re: [HACKERS] Bad bug in fopen() wrapper code
Date
Msg-id 200610032044.k93KiSI10376@momjian.us
Whole thread Raw
In response to Re: [HACKERS] Bad bug in fopen() wrapper code  (Bruce Momjian <bruce@momjian.us>)
List pgsql-patches
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. +

pgsql-patches by date:

Previous
From: Tom Lane
Date:
Subject: Re: vcbuild bison check
Next
From: Bruce Momjian
Date:
Subject: Re: guc units cleanup