Skip to content

gh-127081: use getlogin_r if available #132751

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

Merged
merged 3 commits into from
Jun 3, 2025
Merged

Conversation

duaneg
Copy link
Contributor

@duaneg duaneg commented Apr 20, 2025

The getlogin function is not thread-safe: replace with getlogin_r where available.

Note that this function is untested (unit test is skipped with a note it caused CI failures as behaviour differs between NIX environments).

The getlogin function is not thread-safe: replace with getlogin_r where
available.

Note that this function is untested (unit test is skipped with a note it caused
CI failures as behaviour differs between NIX environments).
@picnixz picnixz changed the title gh-127081: use getlogin_r if available gh-127081: use getlogin_r if available Apr 20, 2025
duaneg added 2 commits April 22, 2025 11:39
…at order)

if available, falling back to a hard-coded maximum login name length of 255 if
neither is available.
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

This should get a test case.

@duaneg
Copy link
Contributor Author

duaneg commented May 18, 2025

This should get a test case.

The specific change or os.getlogin generally?

The existing (unconditionally skipped) test could be re-enabled, but it doesn't test very much at all, and presumably will still fail on some CI systems. Perhaps we could expand it to get the "correct" result to check against via a shell command? Then if/when it fails on specific platforms either skip or special-case the test for them?

Testing the specific change: I can add a multi-threaded test, but it would ideally be conditional on having getlogin_r or it would crash on platforms that don't (assuming there actually are any). Probably easiest to just selectively skip the test on any such platforms I suppose.

@colesbury colesbury self-requested a review May 19, 2025 03:27
@colesbury colesbury added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 20, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @colesbury for commit 675342c 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F132751%2Fmerge

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 20, 2025
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

This LGTM assuming the build bots pass

@colesbury
Copy link
Contributor

Buildbot failures look unrelated to me.

@colesbury colesbury merged commit 1ffe913 into python:main Jun 3, 2025
128 of 130 checks passed
@colesbury colesbury added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jun 3, 2025
@miss-islington-app
Copy link

Thanks @duaneg for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @duaneg for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 3, 2025
The `getlogin` function is not thread-safe: replace with `getlogin_r` where
available.
(cherry picked from commit 1ffe913)

Co-authored-by: Duane Griffin <duaneg@dghda.com>
@miss-islington-app
Copy link

Sorry, @duaneg and @colesbury, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 1ffe913c2017b44804aca18befd45689df06c069 3.13

@bedevere-app
Copy link

bedevere-app bot commented Jun 3, 2025

GH-135097 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Jun 3, 2025
colesbury pushed a commit to colesbury/cpython that referenced this pull request Jun 3, 2025
The `getlogin` function is not thread-safe: replace with `getlogin_r` where
available.
(cherry picked from commit 1ffe913)

Co-authored-by: Duane Griffin <duaneg@dghda.com>
@bedevere-app
Copy link

bedevere-app bot commented Jun 3, 2025

GH-135098 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 3, 2025
colesbury added a commit that referenced this pull request Jun 3, 2025
The `getlogin` function is not thread-safe: replace with `getlogin_r` where
available.
(cherry picked from commit 1ffe913)

Co-authored-by: Duane Griffin <duaneg@dghda.com>
colesbury pushed a commit that referenced this pull request Jun 3, 2025
The `getlogin` function is not thread-safe: replace with `getlogin_r` where
available.
(cherry picked from commit 1ffe913)

Co-authored-by: Duane Griffin <duaneg@dghda.com>
@bedevere-bot
Copy link

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

Hi! The buildbot AMD64 CentOS9 NoGIL Refleaks 3.14 (tier-1) has failed when building commit d8c2bfa.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1774/builds/150) and take a look at the build logs.
  4. Check if the failure is related to this commit (d8c2bfa) or if it is a false positive.
  5. 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/#/builders/1774/builds/150

Failed tests:

  • test.test_concurrent_futures.test_interpreter_pool

Failed subtests:

  • test_free_reference - test.test_concurrent_futures.test_interpreter_pool.InterpreterPoolExecutorTest.test_free_reference

Test leaking resources:

  • test_interpreter_pool: memory blocks

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

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.14.itamaro-centos-aws.refleak.nogil/build/Lib/test/test_concurrent_futures/executor.py", line 220, in test_free_reference
    for _ in support.sleeping_retry(support.SHORT_TIMEOUT):
             ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.14.itamaro-centos-aws.refleak.nogil/build/Lib/test/support/__init__.py", line 2591, in sleeping_retry
    for _ in busy_retry(timeout, err_msg, error=error):
             ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.14.itamaro-centos-aws.refleak.nogil/build/Lib/test/support/__init__.py", line 2561, in busy_retry
    raise AssertionError(msg)
AssertionError: timeout (68.3 seconds)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants