bug in freopen

bug in freopen

[Eric Blake]

POSIX requires that freopen(NULL, mode, f) reopen f in the new
mode, and allows implementations the option of not even closing
f in the first place.  But in cygwin, it is failing with EFAULT, which is
not even one of the errors allowed by POSIX.

http://www.opengroup.org/susv3xsh/freopen.html

#include <stdio.h>
#include <errno.h>
int main(void)
{
   FILE* f = freopen (NULL, "rb", stdin); /* Ensure that stdin is binary */
   printf ("file is %s, errno %d:%s\n", f ? "good" : "null", errno,
           strerror(errno));
   return 0;
}

CVS coreutils recently switched to this idiom, replacing its former
use of the nonstandard <io.h> and setmode() with something
that is required by the standards.  But until this bug is fixed, CVS
coreutils will not work with cygwin.  The strace in cygwin shows
that newlib tried to perform open(NULL), which is the cause of
the EFAULT.

--
Eric Blake

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Re: bug in freopen

[Christopher Faylor]

On Thu, Jul 14, 2005 at 11:13:35PM +0000, Eric Blake wrote:
>POSIX requires that freopen(NULL, mode, f) reopen f in the new
>mode, and allows implementations the option of not even closing
>f in the first place.  But in cygwin, it is failing with EFAULT, which is
>not even one of the errors allowed by POSIX.
>
>http://www.opengroup.org/susv3xsh/freopen.html
>
>#include <stdio.h>
>#include <errno.h>
>int main(void)
>{
>   FILE* f = freopen (NULL, "rb", stdin); /* Ensure that stdin is binary */
>   printf ("file is %s, errno %d:%s\n", f ? "good" : "null", errno,
>           strerror(errno));
>   return 0;
>}
>
>CVS coreutils recently switched to this idiom, replacing its former
>use of the nonstandard <io.h> and setmode() with something
>that is required by the standards.  But until this bug is fixed, CVS
>coreutils will not work with cygwin.  The strace in cygwin shows
>that newlib tried to perform open(NULL), which is the cause of
>the EFAULT.

Hmm.  It should be pretty simple to just make this always fail with
EBADF.  That seems to be a valid thing to do as far as SUSv3 is
concerned.

I am curious, though, as to the rationale behind this change.  I just
tried this with -mno-cygwin and it dies with a ENOENT.  So, they
apparently removed windows-specific code and replaced it with code
that doesn't work on windows...

cgf

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Re: bug in freopen

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Christopher Faylor on 7/14/2005 5:32 PM:
>>#include <stdio.h>
>>#include <errno.h>
>>int main(void)
>>{
>>  FILE* f = freopen (NULL, "rb", stdin); /* Ensure that stdin is binary */
>>  printf ("file is %s, errno %d:%s\n", f ? "good" : "null", errno,
>>          strerror(errno));
>>  return 0;
>>}
>>
> 
> Hmm.  It should be pretty simple to just make this always fail with
> EBADF.  That seems to be a valid thing to do as far as SUSv3 is
> concerned.

Just because SUSv3 permits EBADF doesn't make it a very good QofI choice.
 I don't think it would be too hard to add code that at least gives an
attempt to do the right thing using setmode()/fcntl(F_SETFL).

> 
> I am curious, though, as to the rationale behind this change.  I just
> tried this with -mno-cygwin and it dies with a ENOENT.  So, they
> apparently removed windows-specific code and replaced it with code
> that doesn't work on windows...

The rationale was that the main body of code should be as portable as
possible by sticking to standards, and that platforms that don't meet the
standards then use wrappers from gnulib.  If newlib's freopen is not
patched, then coreutils will probably have to add an autoconf test to
detect that, then add something like this to gnulib:

[freopen.h]
#if FREOPEN_NULL_BROKEN
# include <stdio.h>
# define freopen rpl_freopen
#endif

[freopen.c]
FILE*
rpl_freopen (const char *name, const char *mode, FILE* f)
{
  if (name)
    return freopen (name, mode, f);
  /* Some combination of setmode()/fcntl(fileno(f),F_SETFL,...) */
}

The other part of the rationale is that if cygwin implements this part of
freopen, it can choose to change the modes on regular files but be a no-op
on ttys.  In other words, it would replace code that looks like:
int fd = fileno (f);
if (! isatty (fd))
  setmode (fd, O_BINARY);

with the easier to read
freopen (NULL, "wb", f);

- --
Life is short - so eat dessert first!

Eric Blake             ebb9@byu.net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFC1zJU84KuGfSFAYARAtinAJ9uep5VI7eg56YvNSg6EeirY1tttwCcCv2s
mrKruO0cE0QyiTlbrsyHjVk=
=3qVU
-----END PGP SIGNATURE-----

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

RE: bug in freopen

[Dave Korn]

----Original Message----
>From: Eric Blake
>Sent: 15 July 2005 00:14

> POSIX requires that freopen(NULL, mode, f) reopen f in the new
> mode, and allows implementations the option of not even closing
> f in the first place.  But in cygwin, it is failing with EFAULT, which is
> not even one of the errors allowed by POSIX.
> 
> http://www.opengroup.org/susv3xsh/freopen.html
> 
> #include <stdio.h>
> #include <errno.h>
> int main(void)
> {
>    FILE* f = freopen (NULL, "rb", stdin); /* Ensure that stdin is binary
>    */ printf ("file is %s, errno %d:%s\n", f ? "good" : "null", errno,
>            strerror(errno));
>    return 0;
> }
> 
> CVS coreutils recently switched to this idiom, replacing its former
> use of the nonstandard <io.h> and setmode() with something
> that is required by the standards.  But until this bug is fixed, CVS
> coreutils will not work with cygwin.  The strace in cygwin shows
> that newlib tried to perform open(NULL), which is the cause of
> the EFAULT.


  I could take a look at this over the weekend if nobody gets to it first :)


    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Re: bug in freopen

[Christopher Faylor]

On Thu, Jul 14, 2005 at 09:49:40PM -0600, Eric Blake wrote:
>According to Christopher Faylor on 7/14/2005 5:32 PM:
>>>#include <stdio.h>
>>>#include <errno.h>
>>>int main(void)
>>>{
>>>  FILE* f = freopen (NULL, "rb", stdin); /* Ensure that stdin is binary */
>>>  printf ("file is %s, errno %d:%s\n", f ? "good" : "null", errno,
>>>          strerror(errno));
>>>  return 0;
>>>}
>>>
>> 
>> Hmm.  It should be pretty simple to just make this always fail with
>> EBADF.  That seems to be a valid thing to do as far as SUSv3 is
>> concerned.
>
>Just because SUSv3 permits EBADF doesn't make it a very good QofI choice.
> I don't think it would be too hard to add code that at least gives an
>attempt to do the right thing using setmode()/fcntl(F_SETFL).

My point here is that I find it rather impertinent to break the behavior
on a platform (cygwin) with the apparent assumption that "someone" is
going to do the work to unbreak it.  So, we could very easily make cygwin
compliant with SUSv3 and that would throw the ball back into the coreutils
court.

OTOH, if you don't think it would be hard to do then why don't *you*
provide a patch?

>> 
>> I am curious, though, as to the rationale behind this change.  I just
>> tried this with -mno-cygwin and it dies with a ENOENT.  So, they
>> apparently removed windows-specific code and replaced it with code
>> that doesn't work on windows...
>
>The rationale was that the main body of code should be as portable as
>possible by sticking to standards, and that platforms that don't meet the
>standards then use wrappers from gnulib.  If newlib's freopen is not
>patched, then coreutils will probably have to add an autoconf test to
>detect that,

Er, yes.  I understand this.

I'm all for making code portable.  This doesn't make the code portable.
It *breaks* the code and assumes that freopen will do the right thing
even though it doesn't do the right thing on the two platforms which
require the behavior that they are coding for.  And, cygwin's behavior
is not that far from being compliant.  This is not portable.

What you are describing is making the code "cleaner" not making it more
portable.

If they wanted to make the code "portable", they should have implemented
the gnulib wrapper along with the cleanup.  Just nuking the existing
code and using an interface which isn't even documented to work on
linux, AFAICT, seems odd to me.

cgf

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Re: bug in freopen

[Jeff Johnston]

Dave Korn wrote:
> ----Original Message----
> 
>>From: Eric Blake
>>Sent: 15 July 2005 00:14
> 
> 
>>POSIX requires that freopen(NULL, mode, f) reopen f in the new
>>mode, and allows implementations the option of not even closing
>>f in the first place.  But in cygwin, it is failing with EFAULT, which is
>>not even one of the errors allowed by POSIX.
>>
>>http://www.opengroup.org/susv3xsh/freopen.html
>>
>>#include <stdio.h>
>>#include <errno.h>
>>int main(void)
>>{
>>   FILE* f = freopen (NULL, "rb", stdin); /* Ensure that stdin is binary
>>   */ printf ("file is %s, errno %d:%s\n", f ? "good" : "null", errno,
>>           strerror(errno));
>>   return 0;
>>}
>>
>>CVS coreutils recently switched to this idiom, replacing its former
>>use of the nonstandard <io.h> and setmode() with something
>>that is required by the standards.  But until this bug is fixed, CVS
>>coreutils will not work with cygwin.  The strace in cygwin shows
>>that newlib tried to perform open(NULL), which is the cause of
>>the EFAULT.
> 
> 
> 
>   I could take a look at this over the weekend if nobody gets to it first :)
>

A little late, but please do.  I'm a little busy at the moment.

-- Jeff J.

> 
>     cheers,
>       DaveK


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

RE: bug in freopen

[Dave Korn]

----Original Message----
>From: Jeff Johnston
>Sent: 18 July 2005 20:12

>> 
>>   I could take a look at this over the weekend if nobody gets to it
>> first :) 
>> 
> 
> A little late, but please do.  I'm a little busy at the moment.


  My weekend was busier than expected, but I've still got this one on my
list.


    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

RE: bug in freopen

[Schwarz, Konrad]

> -----Original Message-----
> From: newlib-owner@sources.redhat.com 
> [mailto:newlib-owner@sources.redhat.com] On Behalf Of Eric Blake
> Sent: Friday, July 15, 2005 1:14 AM
> To: cygwin@cygwin.com; newlib@sourceware.org
> Subject: bug in freopen
> 
> POSIX requires that freopen(NULL, mode, f) reopen f in the 
> new mode, and allows implementations the option of not even 
> closing f in the first place.  But in cygwin, it is failing 
> with EFAULT, which is not even one of the errors allowed by POSIX.
> 
> http://www.opengroup.org/susv3xsh/freopen.html

Just to be pedantic: POSIX allows system interfaces to set <errno> to
values additional to those documented in the standard, so this in itself
is not necessarily a bug.

Of course, freopen() should not be calling open() with NULL, it appears
this case has not been implemented in newlib. 

> 
> #include <stdio.h>
> #include <errno.h>
> int main(void)
> {
>    FILE* f = freopen (NULL, "rb", stdin); /* Ensure that 
> stdin is binary */
>    printf ("file is %s, errno %d:%s\n", f ? "good" : "null", errno,
>            strerror(errno));
>    return 0;
> }
> 
> CVS coreutils recently switched to this idiom, replacing its 
> former use of the nonstandard <io.h> and setmode() with 
> something that is required by the standards.  But until this 
> bug is fixed, CVS coreutils will not work with cygwin.  The 
> strace in cygwin shows that newlib tried to perform 
> open(NULL), which is the cause of the EFAULT.
> 
> --
> Eric Blake
> 

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/