Summary: | freopen would close oldfd even though oldfd is same as newfd | ||
---|---|---|---|
Product: | glibc | Reporter: | lizhijian <lizhijian> |
Component: | stdio | Assignee: | Adhemerval Zanella <adhemerval.zanella> |
Status: | RESOLVED DUPLICATE | ||
Severity: | normal | CC: | adhemerval.zanella, bugdal, drepper.fsp, eblake, eggert, fweimer, licquia |
Priority: | P2 | Flags: | fweimer:
security-
|
Version: | 2.16 | ||
Target Milestone: | 2.26 | ||
Host: | Target: | ||
Build: | Last reconfirmed: | ||
Attachments: |
patch for freopen
Fix bugs of freopen |
Description
lizhijian
2013-06-06 05:50:23 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
> # ./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
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
Patch is slightly incorrect: you can never assign 0 to errno in libc. Simply leave errno alone and it would be fine. Created attachment 7079 [details]
Fix bugs of freopen
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. *** Bug 15701 has been marked as a duplicate of this bug. *** 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. 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. 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. (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. 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 (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. I agree with Eric's assessment in comment 13. Fixed by f1a67a2c78601599be51a17250ca02c7d830d79d. *** This bug has been marked as a duplicate of bug 21393 *** |