Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os.dup2() raises wrong OSError for negative fds #102179

Closed
izbyshev opened this issue Feb 23, 2023 · 1 comment
Closed

os.dup2() raises wrong OSError for negative fds #102179

izbyshev opened this issue Feb 23, 2023 · 1 comment
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@izbyshev
Copy link
Contributor

izbyshev commented Feb 23, 2023

>>> os.dup2(-1, 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  OSError: [Errno 0] Error

The bug is caused by the old manual sign check that at some point lost the part that set errno:

cpython/Modules/posixmodule.c

Lines 9832 to 9834 in c3a1783

if (fd < 0 || fd2 < 0) {
posix_error();
return -1;

I'm going to submit a PR with the fix.

Linked PRs

@izbyshev izbyshev added the type-bug An unexpected behavior, bug, or error label Feb 23, 2023
izbyshev pushed a commit to izbyshev/cpython that referenced this issue Feb 23, 2023
A remnant of old code manually checks the sign of fds, but doesn't set
errno before raising OSError:

>>> os.dup2(-1, 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  OSError: [Errno 0] Error

Remove the manual check altogether: the C library functions used to
implement os.dup2() check fd validity anyway.
izbyshev pushed a commit to izbyshev/cpython that referenced this issue Feb 23, 2023
A remnant of old code manually checks the sign of fds, but doesn't set
errno before raising OSError:
```
>>> os.dup2(-1, 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  OSError: [Errno 0] Error
```
Remove the manual check altogether: the C library functions used to
implement os.dup2() check fd validity anyway.
@arhadthedev arhadthedev added the extension-modules C modules in the Modules dir label Feb 25, 2023
@izbyshev
Copy link
Contributor Author

izbyshev commented Mar 4, 2023

It turned out that on wasm32-emscripten dup2() for negative fds is currently broken. I could reproduce that with the C equivalent of the failed test case and the latest emsdk:

$ ./emsdk list | grep INSTALLED
         3.1.32             INSTALLED
         3.1.32    INSTALLED
    *    sdk-releases-29ad1037cd6b99e5d8a1bd75bc188c1e9a6fda8d-64bit    INSTALLED
    (*)    releases-29ad1037cd6b99e5d8a1bd75bc188c1e9a6fda8d-64bit      INSTALLED
    (*)    node-14.18.2-64bit           INSTALLED
$ cat t.c
#include <errno.h>
#include <fcntl.h>
#include <limits.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>

int main() {
        int valid_fd = open(".", O_RDONLY);
        if (valid_fd < 0) {
                perror("open");
                return 1;
        }
        int fds[] = {
                valid_fd,
                -1,
                INT_MIN,
        };
        for (int i = 0; i < sizeof fds / sizeof *fds; i++)
                for (int j = 0; j < sizeof fds / sizeof *fds; j++)
                        if (i != j) {
                                int fd = fds[i], fd2 = fds[j];
                                printf("dup2(%d, %d): ", fd, fd2);
                                errno = 0;
                                if (dup2(fd, fd2) < 0) {
                                        printf("%d (%s)\n", errno, strerror(errno));
                                } else {
                                        printf("succeeded\n");
                                }
                        }
}

$ emcc t.c
$ node a.out.js
dup2(3, -1): 1 (Argument list too long)
dup2(3, -2147483648): 0 (No error information)
dup2(-1, 3): succeeded
dup2(-1, -2147483648): 0 (No error information)
dup2(-2147483648, 3): succeeded
dup2(-2147483648, -1): 1 (Argument list too long)

Apparently, in Emscripten there is ongoing work on WASMFS which fixed this particular issue, and comments in this PR additionally confirm that dup2() is broken in the current default FS implementation.

Indeed, compiling with -sWASMFS makes the test work as expected:

$ emcc -sWASMFS t.c
$ node a.out.js
dup2(3, -1): 8 (Bad file descriptor)
dup2(3, -2147483648): 8 (Bad file descriptor)
dup2(-1, 3): 8 (Bad file descriptor)
dup2(-1, -2147483648): 8 (Bad file descriptor)
dup2(-2147483648, 3): 8 (Bad file descriptor)
dup2(-2147483648, -1): 8 (Bad file descriptor)

Given the current state of Emscripten, I don't think we should care that os.dup2() will start to behave differently after removing the manual fd check in CPython. I propose to simply skip the failed test case on Emscripten.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 4, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 4, 2023
miss-islington added a commit that referenced this issue Mar 4, 2023
(cherry picked from commit c2bd55d)

Co-authored-by: Alexey Izbyshev <[email protected]>
kumaraditya303 added a commit that referenced this issue Mar 4, 2023
…102180) (#102419)

* gh-102179: Fix `os.dup2` error reporting for negative fds (GH-102180)
(cherry picked from commit c2bd55d)

Co-authored-by: Alexey Izbyshev <[email protected]>
Co-authored-by: Kumar Aditya <[email protected]>
carljm added a commit to carljm/cpython that referenced this issue Mar 6, 2023
* main: (21 commits)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives in sub interpreters module (python#102472)
  pythongh-95672: Fix versionadded indentation of get_pagesize in test.rst (pythongh-102455)
  pythongh-102416: Do not memoize incorrectly loop rules in the parser (python#102467)
  pythonGH-101362: Optimise PurePath(PurePath(...)) (pythonGH-101667)
  pythonGH-101362: Check pathlib.Path flavour compatibility at import time (pythonGH-101664)
  pythonGH-101362: Call join() only when >1 argument supplied to pathlib.PurePath() (python#101665)
  pythongh-102444: Fix minor bugs in `test_typing` highlighted by pyflakes (python#102445)
  pythonGH-102341: Improve the test function for pow (python#102342)
  Fix unused classes in a typing test (pythonGH-102437)
  pythongh-101979: argparse: fix a bug where parentheses in metavar argument of add_argument() were dropped (python#102318)
  pythongh-102356: Add thrashcan macros to filter object dealloc (python#102426)
  Move around example in to_bytes() to avoid confusion (python#101595)
  pythonGH-97546: fix flaky asyncio `test_wait_for_race_condition` test (python#102421)
  pythongh-96821: Add config option `--with-strict-overflow` (python#96823)
  pythongh-101992: update pstlib module documentation (python#102133)
  pythongh-63301: Set exit code when tabnanny CLI exits on error (python#7699)
  pythongh-101863: Fix wrong comments in EUC-KR codec (pythongh-102417)
  pythongh-102302 Micro-optimize `inspect.Parameter.__hash__` (python#102303)
  pythongh-102179: Fix `os.dup2` error reporting for negative fds (python#102180)
  pythongh-101892: Fix `SystemError` when a callable iterator call exhausts the iterator (python#101896)
  ...
carljm added a commit to carljm/cpython that referenced this issue Mar 7, 2023
* main: (37 commits)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives in sub interpreters module (python#102472)
  pythongh-95672: Fix versionadded indentation of get_pagesize in test.rst (pythongh-102455)
  pythongh-102416: Do not memoize incorrectly loop rules in the parser (python#102467)
  pythonGH-101362: Optimise PurePath(PurePath(...)) (pythonGH-101667)
  pythonGH-101362: Check pathlib.Path flavour compatibility at import time (pythonGH-101664)
  pythonGH-101362: Call join() only when >1 argument supplied to pathlib.PurePath() (python#101665)
  pythongh-102444: Fix minor bugs in `test_typing` highlighted by pyflakes (python#102445)
  pythonGH-102341: Improve the test function for pow (python#102342)
  Fix unused classes in a typing test (pythonGH-102437)
  pythongh-101979: argparse: fix a bug where parentheses in metavar argument of add_argument() were dropped (python#102318)
  pythongh-102356: Add thrashcan macros to filter object dealloc (python#102426)
  Move around example in to_bytes() to avoid confusion (python#101595)
  pythonGH-97546: fix flaky asyncio `test_wait_for_race_condition` test (python#102421)
  pythongh-96821: Add config option `--with-strict-overflow` (python#96823)
  pythongh-101992: update pstlib module documentation (python#102133)
  pythongh-63301: Set exit code when tabnanny CLI exits on error (python#7699)
  pythongh-101863: Fix wrong comments in EUC-KR codec (pythongh-102417)
  pythongh-102302 Micro-optimize `inspect.Parameter.__hash__` (python#102303)
  pythongh-102179: Fix `os.dup2` error reporting for negative fds (python#102180)
  pythongh-101892: Fix `SystemError` when a callable iterator call exhausts the iterator (python#101896)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants