Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
gpshead merged 26 commits intopython:mainfromarhadthedev:burn-posixsubprocess-with-ac
Apr 24, 2023

Conversation

@arhadthedev
Copy link
Member

@arhadthedevarhadthedev commentedJul 2, 2022
edited
Loading

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 ofcall_setgid will be done in a separate PR.

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

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

Also, it fixes minor bug traps:

@tiran
Copy link
Member

You are passinguid andgid 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 ofcall_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 initializeuid_t uid = (uid_t)-1; and then later check for the sentinel:

if (uid != (uid_t)-1)        POSIX_CALL(setreuid(uid, uid));
arhadthedev reacted with thumbs up emoji

@arhadthedevarhadthedev marked this pull request as draftJuly 5, 2022 10:17
@arhadthedev
Copy link
MemberAuthor

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: Aborted2022-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 handle2022-07-07T07:08:46.6524920Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 766 in assertRaises2022-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_array2022-07-07T07:08:46.6526627Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 579 in _callTestMethod2022-07-07T07:08:46.6527449Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/case.py", line 623 in run2022-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 run2022-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 run2022-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 run2022-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 run2022-07-07T07:08:46.6534410Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/support/__init__.py", line 1090 in _run_suite2022-07-07T07:08:46.6535133Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/support/__init__.py", line 1216 in run_unittest2022-07-07T07:08:46.6535886Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 281 in _test_module2022-07-07T07:08:46.6536664Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 317 in _runtest_inner22022-07-07T07:08:46.6537413Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 360 in _runtest_inner2022-07-07T07:08:46.6538269Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 235 in _runtest2022-07-07T07:08:46.6539007Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/runtest.py", line 265 in runtest2022-07-07T07:08:46.6539761Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/main.py", line 347 in rerun_failed_tests2022-07-07T07:08:46.6540478Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/main.py", line 746 in _main2022-07-07T07:08:46.6541184Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/main.py", line 701 in main2022-07-07T07:08:46.6541871Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/libregrtest/main.py", line 763 in main2022-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_code2022-07-07T07:08:46.6543899Z   File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/runpy.py", line 198 in _run_module_as_main2022-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
Copy link
MemberAuthor

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
Copy link
MemberAuthor

You are passinguid andgid 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 ofcall_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 initializeuid_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 ingh-94687. Would you mind to take a look please (and addskip news by the way)?

@tiran
Copy link
Member

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 reacted with thumbs up emoji

@arhadthedev
Copy link
MemberAuthor

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).

Copy link
Member

@gpsheadgpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@bedevere-bot
Copy link

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 phraseI 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 added a commit to arhadthedev/cpython that referenced this pull requestJul 26, 2022
@arhadthedev
Copy link
MemberAuthor

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@arhadthedev
Copy link
MemberAuthor

@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 intogh-94687, could you review it please?

gpshead reacted with thumbs up emoji

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@arhadthedev for commit7251169 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelApr 12, 2023
@arhadthedevarhadthedev added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelApr 13, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@arhadthedev for commit7251169 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelApr 13, 2023
@arhadthedev
Copy link
MemberAuthor

buildbot/AMD64 Arch Linux TraceRefs PR failed with:

./_bootstrap_python ./Programs/_freeze_module.py abc ./Lib/abc.py Python/frozen_modules/abc.hModules/gcmodule.c:450: visit_decref: Assertion "!_PyObject_IsFreed(op)" failedEnable tracemalloc to get the memory block allocation tracebackobject address  : 0x7feca1c0a7b0object refcount : 1object type     : 0x55d286fd2980object type name: dictobject repr     : make: *** [Makefile:1282: Python/frozen_modules/abc.h] Segmentation fault (core dumped)

Restarted the tests to check that it isn't a fluke (other runners finished successfully).

@arhadthedevarhadthedev marked this pull request as draftApril 13, 2023 08:33
@arhadthedevarhadthedev added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelApr 15, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@arhadthedev for commitc7e1a5e 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelApr 15, 2023
@arhadthedevarhadthedev added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelApr 23, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@arhadthedev for commitcda283a 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelApr 23, 2023
@gpshead
Copy link
Member

buildbot test failures appear unrelated at a glance. i'm going to try and get this merged today.

arhadthedev reacted with heart emoji

@arhadthedevarhadthedev marked this pull request as ready for reviewApril 24, 2023 18:07
@gpshead
Copy link
Member

thanks for sticking with this and improving this code!

@arhadthedevarhadthedev deleted the burn-posixsubprocess-with-ac branchApril 24, 2023 18:48
@arhadthedev
Copy link
MemberAuthor

@gpshead Thank you for merging (and ten-month-long patience despite a constant stream of notifications)!

gpshead reacted with laugh emoji

carljm added a commit to carljm/cpython that referenced this pull requestApr 24, 2023
* main: (53 commits)pythongh-102498 Clean up unused variables and imports in the email module  (python#102482)pythongh-99184: Bypass instance attribute access in `repr` of `weakref.ref` (python#99244)pythongh-99032: datetime docs: Encoding is no longer relevant (python#93365)pythongh-94300: Update datetime.strptime documentation (python#95318)pythongh-103776: Remove explicit uses of $(SHELL) from Makefile (pythonGH-103778)pythongh-87092: fix a few cases of incorrect error handling in compiler (python#103456)pythonGH-103727: Avoid advancing tokenizer too far in f-string mode (pythonGH-103775)  Revert "Add tests for empty range equality (python#103751)" (python#103770)pythongh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic (python#94519)pythonGH-65022: Fix description of copyreg.pickle function (python#102656)pythongh-103323: Get the "Current" Thread State from a Thread-Local Variable (pythongh-103324)pythongh-91687: modernize dataclass example typing (python#103773)pythongh-103746: Test `types.UnionType` and `Literal` types together (python#103747)pythongh-103765: Fix 'Warning: py:class reference target not found: ModuleSpec' (pythonGH-103769)pythongh-87452: Improve the Popen.returncode docs  Removed unnecessary escaping of asterisks (python#103714)pythonGH-102973: Slim down Fedora packages in the dev container (python#103283)pythongh-103091: Add PyUnstable_Type_AssignVersionTag (python#103095)  Add tests for empty range equality (python#103751)pythongh-103712: Increase the length of the type name in AttributeError messages (python#103713)  ...
carljm added a commit to carljm/cpython that referenced this pull requestApr 24, 2023
* superopt: (82 commits)pythongh-101517: fix line number propagation in code generated for except* (python#103550)pythongh-103780: Use patch instead of mock in asyncio unix events test (python#103782)pythongh-102498 Clean up unused variables and imports in the email module  (python#102482)pythongh-99184: Bypass instance attribute access in `repr` of `weakref.ref` (python#99244)pythongh-99032: datetime docs: Encoding is no longer relevant (python#93365)pythongh-94300: Update datetime.strptime documentation (python#95318)pythongh-103776: Remove explicit uses of $(SHELL) from Makefile (pythonGH-103778)pythongh-87092: fix a few cases of incorrect error handling in compiler (python#103456)pythonGH-103727: Avoid advancing tokenizer too far in f-string mode (pythonGH-103775)  Revert "Add tests for empty range equality (python#103751)" (python#103770)pythongh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic (python#94519)pythonGH-65022: Fix description of copyreg.pickle function (python#102656)pythongh-103323: Get the "Current" Thread State from a Thread-Local Variable (pythongh-103324)pythongh-91687: modernize dataclass example typing (python#103773)pythongh-103746: Test `types.UnionType` and `Literal` types together (python#103747)pythongh-103765: Fix 'Warning: py:class reference target not found: ModuleSpec' (pythonGH-103769)pythongh-87452: Improve the Popen.returncode docs  Removed unnecessary escaping of asterisks (python#103714)pythonGH-102973: Slim down Fedora packages in the dev container (python#103283)pythongh-103091: Add PyUnstable_Type_AssignVersionTag (python#103095)  ...
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@gpsheadgpsheadgpshead approved these changes

Assignees

@gpsheadgpshead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@arhadthedev@tiran@bedevere-bot@gpshead

[8]ページ先頭

©2009-2025 Movatter.jp