gh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic #94519

arhadthedev · 2022-07-02T22:58:44Z

Edit: in addition to what a title says, this PR also removes theoretical situation of /call_set[ug]id==true/ with uninitialized /[ug]id/. This is required to avoid undefined behavior (as Christian Heimes initially pointed out). A full removal of call_setgid will be done in a separate PR.

To break nothing, the porting was done in steps, a commit per each:

Employment of clinic from 3.11 to preserve format strings for visual comparison
Type changing of two parameters that are passed from Python as a boolean but treated in C as integers
Upgrade of clinic to 3.12 because verification of format strings is not required anymore
Fix of optimization blockers (BTW, it inspired Fix forced arg format in AC-processed modules with custom converters #94512)

Also, it fixes minor bug traps:

converges variable declaration and initialization in an attempt to hunt down a memory leak detected by ASAN
assigns (...id)-1 as an impossible default value to gid and uid local variables (thanks to Christian for proposing this)

Issue: Port 23-argument _posixsubprocess.fork_exec to Argument Clinic #94518

tiran · 2022-07-05T07:52:39Z

You are passing uid and gid without initializing the variables. It is considered bad style to pass non-initialized variables to function unless you pass by reference.

You can solve the problem and get rid of call_set[ug]id arguments with a simple trick. The values (uid_t)-1 and (gid_t)-1 are reserved. No valid user can have the value (uid_t)-1. You can initialize uid_t uid = (uid_t)-1; and then later check for the sentinel:

if (uid != (uid_t)-1)
        POSIX_CALL(setreuid(uid, uid));

arhadthedev · 2022-07-07T07:23:54Z

Now it builds but crashes the interpreter while Ubuntu CI run:

2022-07-07T07:08:46.6519690Z test_seq_bytes_to_charp_array (test.test_capi.CAPITest.test_seq_bytes_to_charp_array) ... python: ../cpython-ro-srcdir/Python/specialize.c:1707: _Py_Specialize_Call: Assertion `!PyErr_Occurred()' failed.
2022-07-07T07:08:46.6521593Z Fatal Python error: Aborted
2022-07-07T07:08:46.6521762Z 
2022-07-07T07:08:46.6523089Z Current thread 0x00007f7fe531b4c0 (most recent call first):
2022-07-07T07:08:46.6524082Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 236 in handle
2022-07-07T07:08:46.6524920Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 766 in assertRaises
2022-07-07T07:08:46.6525806Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_capi.py", line 140 in test_seq_bytes_to_charp_array
2022-07-07T07:08:46.6526627Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 579 in _callTestMethod
2022-07-07T07:08:46.6527449Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 623 in run
2022-07-07T07:08:46.6528158Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 678 in __call__
2022-07-07T07:08:46.6529256Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 122 in run
2022-07-07T07:08:46.6530010Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 84 in __call__
2022-07-07T07:08:46.6530956Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 122 in run
2022-07-07T07:08:46.6531631Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 84 in __call__
2022-07-07T07:08:46.6532293Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 122 in run
2022-07-07T07:08:46.6532985Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/suite.py", line 84 in __call__
2022-07-07T07:08:46.6533703Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/runner.py", line 208 in run
2022-07-07T07:08:46.6534410Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/support/__init__.py", line 1090 in _run_suite
2022-07-07T07:08:46.6535133Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/support/__init__.py", line 1216 in run_unittest
2022-07-07T07:08:46.6535886Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 281 in _test_module
2022-07-07T07:08:46.6536664Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 317 in _runtest_inner2
2022-07-07T07:08:46.6537413Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 360 in _runtest_inner
2022-07-07T07:08:46.6538269Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 235 in _runtest
2022-07-07T07:08:46.6539007Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 265 in runtest
2022-07-07T07:08:46.6539761Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/main.py", line 347 in rerun_failed_tests
2022-07-07T07:08:46.6540478Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/main.py", line 746 in _main
2022-07-07T07:08:46.6541184Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/main.py", line 701 in main
2022-07-07T07:08:46.6541871Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/main.py", line 763 in main
2022-07-07T07:08:46.6542538Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/__main__.py", line 2 in <module>
2022-07-07T07:08:46.6543204Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/runpy.py", line 88 in _run_code
2022-07-07T07:08:46.6543899Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/runpy.py", line 198 in _run_module_as_main
2022-07-07T07:08:46.6544222Z 
2022-07-07T07:08:46.6544536Z Extension modules: _testcapi, _testmultiphase, _testinternalcapi (total: 3)
2022-07-07T07:08:46.8508081Z make: *** [Makefile:1863: buildbottest] Aborted (core dumped)
2022-07-07T07:08:46.8593973Z ##[error]Process completed with exit code 2.

arhadthedev · 2022-07-07T14:19:12Z

From Ubuntu runner build logs:

##[warning]../cpython-ro-srcdir/Include/longobject.h:36:24: warning: ‘pid’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   36 | #define PyLong_FromPid PyLong_FromLong
      |                        ^~~~~~~~~~~~~~~
/home/runner/work/cpython/cpython-ro-srcdir/Modules/_posixsubprocess.c:1083:11: note: ‘pid’ was declared here
 1083 |     pid_t pid = do_fork_exec(exec_array, argv, envp, cwd,
      |           ^~~

@tiran Any ideas how it can be possible? A compiler points to a fused declaration and assignment complaining that an assignment may be missing.

arhadthedev · 2022-07-22T10:08:24Z

You are passing uid and gid without initializing the variables. It is considered bad style to pass non-initialized variables to function unless you pass by reference.

You can solve the problem and get rid of call_set[ug]id arguments with a simple trick. The values (uid_t)-1 and (gid_t)-1 are reserved. No valid user can have the value (uid_t)-1. You can initialize uid_t uid = (uid_t)-1; and then later check for the sentinel:
if (uid != (uid_t)-1)
        POSIX_CALL(setreuid(uid, uid));

@tiran Thank you, addressed in gh-94687. Would you mind to take a look please (and add skip news by the way)?

tiran · 2022-07-22T10:13:52Z

Please include a news entry.

We typically only skip news if the change is a trivial internal change or change is already covered by a previous PR. Non-trivial commits get a news entry so other core devs, release manager, distributors, and users have an easier way to locate a problem when a change introduces a regression.

arhadthedev · 2022-07-24T08:24:22Z

Please include a news entry.

We typically only skip news if the change is a trivial internal change or change is already covered by a previous PR. Non-trivial commits get a news entry so other core devs, release manager, distributors, and users have an easier way to locate a problem when a change introduces a regression.

Done (initially I've thought this change is invisible to users so skipped it).

gpshead

overall this is in good shape. one change to make.

gpshead · 2022-07-25T23:47:02Z

Modules/_posixsubprocess.c

@@ -476,6 +498,10 @@ reset_signal_handlers(const sigset_t *child_sigmask)
 #endif /* VFORK_USABLE */


+#define RESERVED_GID (gid_t)-1
+#define RESERVED_PID (pid_t)-1


Don't define RESERVED_PID. A pid is never passed in, the pid == -1 checks in the existing code are idiomatic ways to determine a pid_t returning call is reporting an error. Adopting this level of abstraction for pid_t -1 will just confuse readers.

I'm fine with the constants for their GID and UID purposes.

bedevere-bot · 2022-07-25T23:47:07Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

arhadthedev · 2022-07-26T04:23:50Z

I have made the requested changes; please review again.

bedevere-bot · 2022-07-26T04:23:53Z

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

arhadthedev · 2022-07-26T04:31:20Z

@gpshead Addressed. By the way, I would like to merge *id_t-related changes separately to not dilute a purpose of this PR. So I've copied them into gh-94687, could you review it please?

arhadthedev · 2022-08-24T09:07:36Z

Um... I didn't order to force-push it. When I clicked Update with rebase site button near This PR has conflicts with a base branch, I expected a usual conflict resolution editor prompt (at least it always happened before for Doc/whatsnew conflicts). However, I've got an immediate and silent rewrite of a history instead.

arhadthedev requested a review from gpshead as a code owner Jul 2, 2022

bedevere-bot added the awaiting review label Jul 2, 2022

gpshead self-assigned this Jul 2, 2022

arhadthedev marked this pull request as draft Jul 5, 2022

arhadthedev mentioned this pull request Jul 8, 2022

gh-94518: [_posixsubprocess] Replace variable validity flags with reserved values #94687

Open

gpshead requested changes Jul 25, 2022

View changes

bedevere-bot added awaiting changes and removed awaiting review labels Jul 25, 2022

arhadthedev added a commit to arhadthedev/cpython that referenced this pull request Jul 26, 2022

Address Gregory's review in pythongh-94519

06f2a01

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 26, 2022

bedevere-bot requested a review from gpshead Jul 26, 2022

arhadthedev added 8 commits Aug 24, 2022

Convert fork_exec to pre-inlined-argparser Argument Clinic

2e438d9

Make flag parameters boolean

514a377

Regenerate clinic 3.10 files using clinic 3.12

f6ffbb7

Fix forced arg format

bed7d60

Fixed a non-existent name left after renaming

603be00

Revert named arguments in multiprocessing/util.py

a0646e6

Add a NEWS entry

86edd0e

Move uninitialized variables closer to their initialization

fe48164

arhadthedev added 7 commits Aug 24, 2022

Mark do_fork_exec as C11 _Noreturn and remove return 0 hack

90bb494

Address the review on undefined gid/uid

d0a586c

Fix an unitialized PID

df6bb72

Remove confusing out-of-place variable definitions

c927533

Fix fluke (?) "pid may be used uninitialized in this function"

9cb1268

Fix a minor grammar error

18e8821

Address Gregory's review

c822f10

arhadthedev force-pushed the burn-posixsubprocess-with-ac branch from 7ad2a62 to c822f10 Compare Aug 24, 2022

arhadthedev mentioned this pull request Nov 14, 2022

Port 23-argument _posixsubprocess.fork_exec to Argument Clinic #94518

Open

gh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic #94519

gh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic #94519

arhadthedev commented Jul 2, 2022 •

edited

tiran commented Jul 5, 2022

arhadthedev commented Jul 7, 2022

arhadthedev commented Jul 7, 2022

arhadthedev commented Jul 22, 2022

tiran commented Jul 22, 2022

arhadthedev commented Jul 24, 2022

gpshead left a comment

gpshead Jul 25, 2022

bedevere-bot commented Jul 25, 2022

arhadthedev commented Jul 26, 2022

bedevere-bot commented Jul 26, 2022

arhadthedev commented Jul 26, 2022

arhadthedev commented Aug 24, 2022

gh-94518: Port 23-argument _posixsubprocess.fork_exec to Argument Clinic #94519

Are you sure you want to change the base?

gh-94518: Port 23-argument _posixsubprocess.fork_exec to Argument Clinic #94519

Conversation

arhadthedev commented Jul 2, 2022 • edited

tiran commented Jul 5, 2022

arhadthedev commented Jul 7, 2022

arhadthedev commented Jul 7, 2022

arhadthedev commented Jul 22, 2022

tiran commented Jul 22, 2022

arhadthedev commented Jul 24, 2022

gpshead left a comment

gpshead Jul 25, 2022

Choose a reason for hiding this comment

bedevere-bot commented Jul 25, 2022

arhadthedev commented Jul 26, 2022

bedevere-bot commented Jul 26, 2022

arhadthedev commented Jul 26, 2022

arhadthedev commented Aug 24, 2022

gh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic #94519

gh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic #94519

arhadthedev commented Jul 2, 2022 •

edited