gh-107913: Fix possible losses of OSError error codes by serhiy-storchaka · Pull Request #107930 · python/cpython

serhiy-storchaka · 2023-08-14T08:19:03Z

Functions like PyErr_SetFromErrno() and SetFromWindowsErr() should be called immediately after using the C API which sets errno or the Windows error code.

Issue: PyErr_SetFromErrno() etc should be called immediately after setting the error code #107913

Functions like PyErr_SetFromErrno() and SetFromWindowsErr() should be called immediately after using the C API which sets errno or the Windows error code.

vstinner

Maybe it would be just easier/safer to save the errno and then pass it to the function raising OSError, rather than relying on the magic errno variable.

When it's about exchanging two lines, it's fine. But when you move large cleanup code just to preserve errno, IMO it's not a good tradeoff. I prefer code readability and code easy to follow, to read/audit. If you care about the original errno value, just save it and add an API which takes an error code. It's weird that Python has such API for Windows, but not for Unix :-)

This pattern } while (n < 0 && errno == EINTR && !(async_err = PyErr_CheckSignals())); is kind of fragile if you can about the original errno.

vstinner · 2023-08-26T02:20:24Z

Modules/_io/fileio.c

+            if (self->fd < 0) {
+                PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, nameobj);
+                goto error;
+            }


fd_is_own = 1; changes the code in the error label, it's unclear to me if your change is correct or not.

It only has effect if self->fd >= 0.

Ok, I see. This change is non-trivial, I preferred to double check.

vstinner · 2023-08-26T02:22:07Z

Modules/_ssl.c

        else if (errno != 0) {
-            ERR_clear_error();
            PyErr_SetFromErrno(PyExc_OSError);
+            ERR_clear_error();


Is this function related to errno? Or is it specific to ssl and it leaves errno unchanged?

I do not know, but it is better to be on safe side.

Ok, I'm fine with this change.

vstinner · 2023-08-26T02:24:23Z

Modules/mmapmodule.c

        PyErr_SetFromErrno(PyExc_OSError);
+        if (devzero != -1) {
+            close(devzero);
+        }


Copying these 3 lines sounds overkill. The code would look better if you would just save/restore errno.

vstinner · 2023-08-26T02:26:21Z

Modules/posixmodule.c

-        Py_DECREF(list);
-        list = path_error(path);
+        path_error(path);
+        Py_SETREF(list, NULL);


Py_CLEAR() can be used. Same remark below.

Oh wow, list = path_error(path); code is wild! But it works :-D

Yes, for some reasons I used code like Py_SETREF(list, NULL) in few places when introduced this macro (Py_CLEAR() has an additional check and Py_SETREF() clearer expressed the intention). But in this case I think we can ignore the difference and simply use Py_CLEAR().

Done.

Modules/posixmodule.c

vstinner · 2023-08-26T02:29:41Z

Modules/posixmodule.c

-        return -1;
+        n = -1;
    }
+    iov_cleanup(iov, buf, cnt);


Would it be possible to leave it where it is and save/restore errno?

Modules/posixmodule.c

vstinner · 2023-08-26T02:34:18Z

Modules/posixmodule.c

+        PyMem_Free(path);
+#else
+        Py_DECREF(ub);
+#endif


i dislike the fact that you have to copy/paste the cleanup code. maybe save/restor errno?

vstinner · 2023-08-26T02:35:40Z

Modules/posixmodule.c

+            PyMem_Free(path);
+            return NULL;
+        }
+        PyMem_Free(path);


For me, it's easier to reason about PyMem_Free() when it's closer to where it's used, I would prefer to keep it where it is. In the previous code, for me, the scope of "path" is more obvious to me, thanks to the grouped lines.

vstinner · 2023-08-26T02:36:57Z

Python/fileutils.c

+#endif

-    if (set_inheritable(fileno(f), 0, 1, NULL) < 0) {
+    if (f != NULL && set_inheritable(fileno(f), 0, 1, NULL) < 0) {


I dislike changes in this file :-( I prefer to exit ASAP on error.

serhiy-storchaka

Thank you for your review comments, Victor. I hope that I addressed them all.

serhiy-storchaka · 2023-08-26T04:34:11Z

Modules/_io/fileio.c

+            if (self->fd < 0) {
+                PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, nameobj);
+                goto error;
+            }


It only has effect if self->fd >= 0.

serhiy-storchaka · 2023-08-26T04:34:52Z

Modules/_ssl.c

        else if (errno != 0) {
-            ERR_clear_error();
            PyErr_SetFromErrno(PyExc_OSError);
+            ERR_clear_error();


I do not know, but it is better to be on safe side.

serhiy-storchaka · 2023-08-26T04:40:46Z

Modules/mmapmodule.c

        PyErr_SetFromErrno(PyExc_OSError);
+        if (devzero != -1) {
+            close(devzero);
+        }


serhiy-storchaka · 2023-08-26T04:47:50Z

Modules/posixmodule.c

-        Py_DECREF(list);
-        list = path_error(path);
+        path_error(path);
+        Py_SETREF(list, NULL);


Yes, for some reasons I used code like Py_SETREF(list, NULL) in few places when introduced this macro (Py_CLEAR() has an additional check and Py_SETREF() clearer expressed the intention). But in this case I think we can ignore the difference and simply use Py_CLEAR().

Done.

Modules/posixmodule.c

serhiy-storchaka · 2023-08-26T04:53:05Z

Modules/posixmodule.c

-        return -1;
+        n = -1;
    }
+    iov_cleanup(iov, buf, cnt);


Modules/posixmodule.c

serhiy-storchaka · 2023-08-26T05:07:00Z

Modules/posixmodule.c

+        PyMem_Free(path);
+#else
+        Py_DECREF(ub);
+#endif


serhiy-storchaka · 2023-08-26T05:08:50Z

Modules/posixmodule.c

+            PyMem_Free(path);
+            return NULL;
+        }
+        PyMem_Free(path);


serhiy-storchaka · 2023-08-26T05:13:07Z

Python/fileutils.c

+#endif

-    if (set_inheritable(fileno(f), 0, 1, NULL) < 0) {
+    if (f != NULL && set_inheritable(fileno(f), 0, 1, NULL) < 0) {


vstinner

LGTM! Thanks, it's a nice fix.

I'm concerned about the complex async_err/EINTR code path, but this one is complicated and can deserve its own separated PR.

vstinner · 2023-08-26T14:42:02Z

Modules/_io/fileio.c

+            if (self->fd < 0) {
+                PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, nameobj);
+                goto error;
+            }


Ok, I see. This change is non-trivial, I preferred to double check.

vstinner · 2023-08-26T14:45:24Z

Modules/posixmodule.c

    if (dirp == NULL) {
-        list = path_error(path);
+        path_error(path);
+        list = NULL;


This list parameter is like really surprising. Would you mind to take the opportunity to convert it to a regular local variable (and maybe always initialize it to NULL at the beginning)?

It seems like in the case, the caller could pass their own list.

I have a personal preference to code like this for the code below:

list = PyList_New(0); if (list == NULL) { goto exit; }

miss-islington · 2023-08-26T21:35:10Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington · 2023-08-26T21:35:27Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2b15536fa94d07e9e286826c23507402313ec7f4 3.11

bedevere-bot · 2023-08-26T21:35:29Z

GH-108523 is a backport of this pull request to the 3.12 branch.

…-107930) Functions like PyErr_SetFromErrno() and SetFromWindowsErr() should be called immediately after using the C API which sets errno or the Windows error code. (cherry picked from commit 2b15536) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…ythonGH-107930) Functions like PyErr_SetFromErrno() and SetFromWindowsErr() should be called immediately after using the C API which sets errno or the Windows error code.. (cherry picked from commit 2b15536) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-bot · 2023-08-26T21:48:44Z

GH-108524 is a backport of this pull request to the 3.11 branch.

bedevere-bot · 2023-08-26T22:47:38Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 Fedora Stable LTO 3.x has failed when building commit 2b15536.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/336/builds/3841) and take a look at the build logs.
Check if the failure is related to this commit (2b15536) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/336/builds/3841

Failed tests:

test.test_concurrent_futures.test_shutdown

Failed subtests:

test_interpreter_shutdown - test.test_concurrent_futures.test_shutdown.ProcessPoolForkserverProcessPoolShutdownTest.test_interpreter_shutdown

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

448 tests OK.

10 slowest tests:

test_gdb: 3 min 30 sec
test.test_concurrent_futures.test_wait: 1 min 12 sec
test.test_multiprocessing_spawn.test_processes: 1 min 5 sec
test_signal: 1 min
test_subprocess: 54.8 sec
test_socket: 52.7 sec
test.test_multiprocessing_forkserver.test_processes: 48.6 sec
test_unparse: 46.7 sec
test_io: 42.7 sec
test_math: 38.6 sec

1 test failed:
test.test_concurrent_futures.test_shutdown

14 tests skipped:
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test_devpoll test_ioctl
test_kqueue test_launcher test_startfile test_tkinter test_ttk
test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test.test_concurrent_futures.test_shutdown

Total duration: 4 min 2 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/test/test_concurrent_futures/test_shutdown.py", line 49, in test_interpreter_shutdown
    self.assertFalse(err)
AssertionError: b'Exception in thread Thread-1:\nTraceback (most recent call last):\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 1059, in _bootstrap_inner\n    self.run()\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/concurrent/futures/process.py", line 339, in run\n    self.add_call_item_to_queue()\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/concurrent/futures/process.py", line 394, in add_call_item_to_queue\n    self.call_queue.put(_CallItem(work_id,\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/queues.py", line 94, in put\n    self._start_thread()\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/queues.py", line 177, in _start_thread\n    self._thread.start()\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 978, in start\n    _start_new_thread(self._bootstrap, ())\nRuntimeError: can\'t create new thread at interpreter shutdown\nTraceback (most recent call last):\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/forkserver.py", line 274, in main\n    code = _serve_one(child_r, fds,\n           ^^^^^^^^^^^^^^^^^^^^^^^^\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/forkserver.py", line 313, in _serve_one\n    code = spawn._main(child_r, parent_sentinel)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/spawn.py", line 132, in _main\n    self = reduction.pickle.load(from_parent)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/synchronize.py", line 115, in __setstate__\n    self._semlock = _multiprocessing.SemLock._rebuild(*state)\n                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\nFileNotFoundError: [Errno 2] No such file or directory\n' is not false

…) (#108523) gh-107913: Fix possible losses of OSError error codes (GH-107930) Functions like PyErr_SetFromErrno() and SetFromWindowsErr() should be called immediately after using the C API which sets errno or the Windows error code. (cherry picked from commit 2b15536) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…) (GH-108524) Functions like PyErr_SetFromErrno() and SetFromWindowsErr() should be called immediately after using the C API which sets errno or the Windows error code. (cherry picked from commit 2b15536)

pythongh-107913: Fix possible losses of OSError error codes

29f6f36

Functions like PyErr_SetFromErrno() and SetFromWindowsErr() should be called immediately after using the C API which sets errno or the Windows error code.

serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Aug 14, 2023

bedevere-bot mentioned this pull request Aug 13, 2023

PyErr_SetFromErrno() etc should be called immediately after setting the error code #107913

Closed

bedevere-bot added the awaiting core review label Aug 14, 2023

serhiy-storchaka requested a review from vstinner August 15, 2023 06:35

vstinner reviewed Aug 26, 2023

View reviewed changes

serhiy-storchaka added 2 commits August 26, 2023 07:38

Merge branch 'main' into use-PyErr_SetFromErrno

0485db3

Address review comments.

ba06d0a

serhiy-storchaka commented Aug 26, 2023

View reviewed changes

Add missed "return NULL".

695dbdb

vstinner approved these changes Aug 26, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 26, 2023

serhiy-storchaka merged commit 2b15536 into python:main Aug 26, 2023

bedevere-bot removed the awaiting merge label Aug 26, 2023

serhiy-storchaka deleted the use-PyErr_SetFromErrno branch August 26, 2023 21:35

miss-islington assigned serhiy-storchaka Aug 26, 2023

bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 26, 2023

bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 26, 2023

serhiy-storchaka removed their assignment Sep 8, 2023

Uh oh!

Conversation

serhiy-storchaka commented Aug 14, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Aug 26, 2023

Uh oh!

miss-islington commented Aug 26, 2023

Uh oh!

bedevere-bot commented Aug 26, 2023

Uh oh!

serhiy-storchaka commented Aug 14, 2023 •

edited by bedevere-bot

Loading