Bug 20677 - perror() changes orientation of stream when it is redirected
Summary: perror() changes orientation of stream when it is redirected
Status: UNCONFIRMED
Alias: None
Product: glibc
Classification: Unclassified
Component: stdio (show other bugs)
Version: 2.24
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-10 05:41 UTC by Igor Liferenko
Modified: 2018-11-14 17:54 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Do not use fdopen on not-oriented streams. Just get the fd with fileno() and then write directly to that fd. (840 bytes, patch)
2016-10-11 08:56 UTC, Igor Liferenko
Details | Diff
There was a typo in the previous patch. This is the correct one. (844 bytes, patch)
2016-10-11 09:22 UTC, Igor Liferenko
Details | Diff
do not use fdopen on not-oriented streams (1.32 KB, patch)
2016-10-17 07:06 UTC, Igor Liferenko
Details | Diff
added TODO about streams which are not backed by a POSIX file descriptor (1.26 KB, patch)
2016-10-18 02:05 UTC, Igor Liferenko
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Igor Liferenko 2016-10-10 05:41:38 UTC
Posix insists that the orientation of stderr not be changed by perror, even if stderr is not yet oriented.

Following is the quote from "XSH 2.5 Standard I/O Streams" [1]:

The perror(), psiginfo(), and psignal() functions shall behave as described above for the byte output functions if the stream is already byte-oriented, and shall behave as described above for the wide-character output functions if the stream is already wide-oriented. If the stream has no orientation, they shall behave as described for the byte output functions except that they shall not change the orientation of the stream.
And glibc attempts to implement Posix semantics. Unfortunately, it doesn't quite get it right.

Here is also quote from "perror()" reference page [2]:

The perror() function shall not change the orientation of the standard error stream.

And glibc attempts to implement Posix semantics. Unfortunately, it doesn't quite get it right.

Following are the tests when stderr is wide-oriented, multibyte-oriented and not oriented, prior to calling perror(). Tests 1) and 2) are OK. The issue is in test 3).

1) stderr is wide-oriented:

#include <stdio.h>
#include <wchar.h>
#include <errno.h>
int main(void)
{
  fwide(stderr, 1);
  errno = EINVAL;
  perror("");
  int x = fwide(stderr, 0);
  printf("fwide: %d\n",x);
  return 0;
}

$ ./a.out
Invalid argument
fwide: 1
$ ./a.out 2>/dev/null
fwide: 1

2) stderr is multibyte-oriented:

#include <stdio.h>
#include <wchar.h>
#include <errno.h>
int main(void)
{
  fwide(stderr, -1);
  errno = EINVAL;
  perror("");
  int x = fwide(stderr, 0);
  printf("fwide: %d\n",x);
  return 0;
}

$ ./a.out
Invalid argument
fwide: -1
$ ./a.out 2>/dev/null
fwide: -1

3) stderr is not oriented:

#include <stdio.h>
#include <wchar.h>
#include <errno.h>
int main(void)
{
  printf("initial fwide: %d\n", fwide(stderr, 0));
  errno = EINVAL;
  perror("");
  int x = fwide(stderr, 0);
  printf("fwide: %d\n", x);
  return 0;
}

$ ./a.out
initial fwide: 0
Invalid argument
fwide: 0
$ ./a.out 2>/dev/null
initial fwide: 0
fwide: -1

Of course, it is impossible to write to a stream without setting its orientation. So in an attempt to comply with this curious requirement, glibc attempts to make a new stream based on the same fd as stderr by using dup().

fileno() extracts the fd from a standard C library stream. dup() takes an fd, duplicates it, and returns the number of the copy. And fdopen() creates a standard C library stream from an fd. In short, that doesn't reopen stderr; rather, it creates (or attempts to create) a copy of stderr which can be written to without affecting the orientation of stderr.

Unfortunately, it doesn't work reliably because of the mode:

    fp = fdopen(fd, "w+");

That attempts to open a stream which allows both reading and writing. And it will work with the original stderr, which is just a copy of the console fd, originally opened for both reading and writing. But when you bind stderr to some other device with a redirect:

$ ./a.out 2>/dev/null

you are passing the executable an fd opened only for output. And  fdopen won't let you get away with that:

The application shall ensure that the mode of the stream as expressed by the mode argument is allowed by the file access mode of the open file description to which fildes refers.

The glibc implementation of fdopen actually checks, and returns NULL with errno set to EINVAL if you specify a mode which requires access rights not available to the fd.

I don't know why the glibc code uses "w+" as a mode argument, since it has no intention of reading from stderr.

Actually, you could just write directly to the new fd using write instead of fwrite, so I don't think fdopen is actually necessary at all.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_05

[2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/perror.html
Comment 1 Igor Liferenko 2016-10-10 06:10:57 UTC
Originally the issue was raised here:

http://stackoverflow.com/questions/39950678/why-perror-changes-orientation-of-stream-when-it-is-redirected
Comment 2 Florian Weimer 2016-10-10 11:25:19 UTC
stderr does not have to be backed by a POSIX file descriptor because glibc explicitly permits assignments to stderr.  So any fileno-based hack, with our without stderr, is not quite correct.
Comment 3 Igor Liferenko 2016-10-11 08:56:04 UTC
Created attachment 9556 [details]
Do not use fdopen on not-oriented streams. Just get the fd with fileno() and then write directly to that fd.

Instead of leaving it completely broken, I suggest the attached patch to fix it at least partially, until a better solution (without using "fileno") will be found.

FWIW, the bug was introduced in this commit:

    https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=1fc0e33153186a90140c3d25f5d9b4537890d7cc

Since this commit, perror.c was modified in these commits:

    https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=eaad82e00522075b805621309775131e27388791
    https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=aec84f53952315ac1bd91036e37113d9cb3a303b
Comment 4 Igor Liferenko 2016-10-11 09:22:00 UTC
Created attachment 9557 [details]
There was a typo in the previous patch. This is the correct one.
Comment 5 Igor Liferenko 2016-10-17 07:06:33 UTC
Created attachment 9562 [details]
do not use fdopen on not-oriented streams

Hi all,

Attached is the new patch. I tested this patch and it works good. Specifically, the output from example 3) now is:

    $ ./a.out 2>/dev/null
    initial fwide: 0
    fwide: 0

In comparison with the previous patch, this patch corrects these things:
1) get rid of "perror_internal" (because it is used only once);
2) fix "write" of newline character;
3) use internal "__write"
4) add verbose comments

What is your opinion about this patch? Especially, I'd like to know what to do if "__write()" fails and whether "if (_IO_ferror_unlocked(fp))" test may be safely deleted.

Regards,
Igor
Comment 6 Florian Weimer 2016-10-17 07:27:30 UTC
(In reply to Igor Liferenko from comment #5)

> What is your opinion about this patch?

See comment 2.  You cannot assume stderr is backed by a POSIX file descriptor.
Comment 7 Igor Liferenko 2016-10-17 07:56:46 UTC
It is true. But this patch does not get things worse than they are now - current code does use fileno anyway. What should be done about this?
Comment 8 Igor Liferenko 2016-10-17 08:17:46 UTC
This little program demonstrates that the idea with "write()" works perfectly:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>

int main(void)
{
  int fd;
  int fd2;
  char buf[200];
  int n;
  char fname[] = "/tmp/tst-perror.XXXXXX";
  fd = mkstemp (fname);
  fclose (stderr);
  dup2 (fd, 2);
  stderr = fdopen (2, "w");
  fd2 = fileno(stderr);
  char *s = "this is a test\n";
  n = write(fd2, s, strlen(s));
  fprintf(stderr, "multibyte string\n");
  fclose (stderr);
  lseek (fd, 0, SEEK_SET);
  n = read (fd, buf, sizeof (buf));
  printf("%.*s", (int) n, buf);
  close (fd);
  return 0;
}
Comment 9 Igor Liferenko 2016-10-18 02:05:31 UTC
Created attachment 9580 [details]
added TODO about streams which are not backed by a POSIX file descriptor

Hi,

This is the new patch.
I added comment with TODO about the streams which are not backed by a POSIX
file descriptor.
Please somebody say if this patch is correct and if you will consider applying it sometime in the future.

Thanks,
Igor
Comment 10 cquike 2018-11-14 17:34:55 UTC
Hi,

I have found a weird behaviour with perror() that might be related to this bug. The following simple code:

#include <stdio.h>
int main (void)
{
  perror("");
  perror("");
}

will generate the following when executed directly:

$ ./a.out
Success
Success

But if standard error is redirected the second perror call generates an "Invalid argument" output:

$ ./a.out |& cat
Success
Invalid argument
Comment 11 Andreas Schwab 2018-11-14 17:54:06 UTC
That's not a bug.  In general, any library function call can change the value of errno.