Bug 15589 - freopen would close oldfd even though oldfd is same as newfd
Summary: freopen would close oldfd even though oldfd is same as newfd
Status: RESOLVED DUPLICATE of bug 21393
Alias: None
Product: glibc
Classification: Unclassified
Component: stdio (show other bugs)
Version: 2.16
: P2 normal
Target Milestone: 2.26
Assignee: Adhemerval Zanella
URL:
Keywords:
: 15701 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-06-06 05:50 UTC by lizhijian
Modified: 2018-10-19 12:35 UTC (History)
7 users (show)

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


Attachments
patch for freopen (680 bytes, patch)
2013-06-06 06:23 UTC, lizhijian
Details | Diff
Fix bugs of freopen (647 bytes, patch)
2013-06-14 04:03 UTC, lizhijian
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description lizhijian 2013-06-06 05:50:23 UTC
after glibc commit:
http://repo.or.cz/w/glibc.git/commit/94b7cc3711b0b74c1d3ae18b9a2e019e51a8e0bf

It look like cause some problems,
freopen would close oldfd even though oldfd is same as newfd.
Comment 1 lizhijian 2013-06-06 05:53:03 UTC
A simple C program(freopen-test.c) to reproduce this problem.

#include <stdio.h>
#include <errno.h>
#include <string.h>
#include <sys/stat.h>

int main(void)
{
        char *file1 = "./file1";
        char *file2 = "./file2";
        FILE *fp1, *fp2;
        struct stat stat_buf;

        if((fp1 = freopen(file1, "r", stdin)) == NULL)
                printf("[E+] freopen error(%s)\n", strerror(errno));

        if (fstat(fileno(stdin), &stat_buf) == 0) {
                printf("[S+] close stdin\n");
                close(0);  //close stdin
        }

        if (fstat(fileno(stdin), &stat_buf) == -1) {
                printf("[S+] stat return error(%s)\n", strerror(errno));
                if ((fp2 = freopen(file2, "r", stdin)) == NULL) {
                        printf("[E+] freopen return error(%s)\n",strerror(errno));
                        return -1;
                }
        }
        if (fstat(fileno(fp2), &stat_buf) == -1) {
                printf("[E+] freopen return success, but the newfd was closed\n");
                return -1;
        }

        fclose(fp1);
        fclose(fp2);
        printf("[S+] freopen test PASS\n");
}
------------------------
# gcc freopen-test.c -o freopen-test
# touch file1 file2
# ./freopen-test
[E+] freopen error(No such file or directory)
[S+] stat return error(Bad file descriptor)
[E+] freopen return error(No such file or directory)

# rpm -q glibc
glibc-2.16-29.el7.x86_64
Comment 2 lizhijian 2013-06-06 05:56:56 UTC
> # ./freopen-test
> [E+] freopen error(No such file or directory)
> [S+] stat return error(Bad file descriptor)
> [E+] freopen return error(No such file or directory)
> 
sorry ,make some miss,forget to create testfile.
# touch file1 file2
test ouput as follows(glibc=2.16):
# ./freopen-test
[S+] close stdin
[S+] stat return error(Bad file descriptor)
[E+] freopen return success, but the newfd was closed
Comment 3 lizhijian 2013-06-06 06:23:38 UTC
Created attachment 7059 [details]
patch for freopen

When newfd is same as oldfd, __dup3 return -1 and set errno EINVAL
In this case, it don't close oldfd
Comment 4 Rich Felker 2013-06-07 13:02:51 UTC
Patch is slightly incorrect: you can never assign 0 to errno in libc. Simply leave errno alone and it would be fine.
Comment 5 lizhijian 2013-06-14 04:03:21 UTC
Created attachment 7079 [details]
Fix bugs of freopen
Comment 6 Paul Eggert 2016-11-26 02:26:50 UTC
This bug turns out to break GNU core utilities like 'uniq' and 'shuf'. See coreutils Bug#25029:

https://bugs.gnu.org/25029

I suppose we will look into a workaround by adding a Gnulib wrapper for freopen, but it would be nicer if this bug were fixed in glibc.

Also, Bug#15701 appears to be a duplicate of this bug.
Comment 7 Florian Weimer 2016-11-26 10:21:19 UTC
*** Bug 15701 has been marked as a duplicate of this bug. ***
Comment 8 Florian Weimer 2016-11-26 10:29:28 UTC
I'm struggling to come up with an interpretation of POSIX which allows one to close the underlying file descriptor of a stream.  Our implementation does not support this for several reasons (flushing on exit is one).

POSIX requires ignoring errors on freopen, which is not quite the same thing.  It also makes it impossible to check for data loss when reopening a writable stream, so I don't see why anyone would want to use this interface.
Comment 9 Rich Felker 2016-11-26 14:41:35 UTC
It's most certainly nonsense to close the underlying fd for a stream then perform any subsequent operations on the stream, but the situation could arise as an implementation detail when a program is exec'd with the standard streams closed. In that case, fd 0/1/2 may not be valid but the standard FILE objects would reference them. I'm not sure whether this should be considered a valid usage; arguably it's undefined to access the FILE objects in this case. Alternatively, it's arguable that the implementation should detect this condition at program start and lock out any access to the FILE objects.
Comment 10 Paul Eggert 2016-11-26 16:19:21 UTC
The usage that caused the bug is something like this, in a program that accepts a single arg FILE that may be "-" to denote standard input:

   if (strcmp (file, "-") != 0 && !freopen (file, "r", stdin))
      error (EXIT_FAILURE, errno, file);

Here any error due to fd 0 being closed is irrelevant, as it's the first time the program has accessed stdin. This is a typical use case for freopen, and to some extent it's the reason freopen exists at all.

This sort of thing works in BSD and other non-GNU operating systems that I know of, and it would be a shame for glibc to break it. Although it's perhaps not crystal clear that POSIX allows such breakage, it is clear that POSIX does not require it, and the intent of the freopen API is to support such usage instead of to break it.
Comment 11 Florian Weimer 2016-11-26 16:41:34 UTC
(In reply to Paul Eggert from comment #10)
> The usage that caused the bug is something like this, in a program that
> accepts a single arg FILE that may be "-" to denote standard input:
> 
>    if (strcmp (file, "-") != 0 && !freopen (file, "r", stdin))
>       error (EXIT_FAILURE, errno, file);

This looks far more reasonable.

> Here any error due to fd 0 being closed is irrelevant, as it's the first
> time the program has accessed stdin. This is a typical use case for freopen,
> and to some extent it's the reason freopen exists at all.

On the other hand, POSIX is quite clear that calling execve with the descriptor 0 not open for reading and 1/2 not open for writing is undefined.  If this happens as the result of a shell command, it suggests a shell bug to me.

We could work around it in glibc by associating descriptors 0/1/2 with /dev/null unconditionally, a not just for SUID programs, but this may impact backwards compatibility for programs which never plan to use the standard descriptors in the intended way.
Comment 12 Paul Eggert 2016-11-27 01:32:46 UTC
It's not a POSIX violation if the shell executes "./a.out <&-" with stdin being closed. Although the command is not portable as-is and POSIX allows the shell to act as if the command were "./a.out </dev/null", POSIX doesn't require the shell to do so. Common shells close stdin (which POSIX also allows) and GNU utilities should do the right thing in this case. This action by common shells is what caused coreutils Bug#25029.

For what it's worth, I changed Gnulib to work around the glibc freopen behavior, which is unexpected by so many uses of freopen that I think it qualifies as a bug even if it's not a POSIX-conformance issue. This should fix the coreutils bug. Non-Gnulib-using applications are still vulnerable to the glibc problem.

The Gnulib patch and followup fix are here:

http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=b947d0524d64b5a139282fd48caa7a866e20513c

http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=ea96186d0b25c89aab0de3129fc4bb3f7a5ccd37
Comment 13 Eric Blake 2016-11-28 16:34:42 UTC
(In reply to Paul Eggert from comment #12)
> It's not a POSIX violation if the shell executes "./a.out <&-" with stdin
> being closed. Although the command is not portable as-is and POSIX allows
> the shell to act as if the command were "./a.out </dev/null", POSIX doesn't
> require the shell to do so. Common shells close stdin (which POSIX also
> allows) and GNU utilities should do the right thing in this case. This
> action by common shells is what caused coreutils Bug#25029.

POSIX explicitly states that starting an executable with fd 0, 1, or 2 closed puts the behavior of that executable outside the bounds of POSIX (that is, it is undefined behavior to attempt to run "./a.out <&-", so POSIX no longer has any say, and all bets are off).  So I definitely agree that this is not a conformance issue, at least for freopen(stdin).

> 
> For what it's worth, I changed Gnulib to work around the glibc freopen
> behavior, which is unexpected by so many uses of freopen that I think it
> qualifies as a bug even if it's not a POSIX-conformance issue. This should
> fix the coreutils bug. Non-Gnulib-using applications are still vulnerable to
> the glibc problem.

Indeed, quality of implementation demands that we tolerate closed fds during freopen(), since we already go out of our way to tolerate closed fds during popen(), which is yet another place where POSIX has no direct say.
Comment 14 Rich Felker 2016-11-28 19:53:18 UTC
I agree with Eric's assessment in comment 13.
Comment 15 Adhemerval Zanella 2018-10-19 12:35:48 UTC
Fixed by f1a67a2c78601599be51a17250ca02c7d830d79d.

*** This bug has been marked as a duplicate of bug 21393 ***