KEMBAR78
gh-127081: use re-entrant variants of `get{proto,serv}by{name,port}` by duaneg · Pull Request #132750 · python/cpython · GitHub
Skip to content

gh-127081: use re-entrant variants of get{proto,serv}by{name,port} #132750

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

gh-127081: use re-entrant variants of get{proto,serv}by{name,port} #132750

wants to merge 1 commit into from

Conversation

@duaneg
Copy link
Contributor

@duaneg duaneg commented Apr 20, 2025 •
edited by bedevere-app bot
Loading

Add configure tests and defines for getservbyname_r, getservbyport_r, and getprotobyname_r. Use these if available, otherwise fallback to the thread-unsafe variants.

Add a unit test to exercise getprotobyname, which is currently untested.

TODO:

  • Are there any platforms which define the unsafe variants but not the re-entrant ones? If not we can simplify the #ifdef hell somewhat.
  • Do the re-entrant functions have the same signature on all platforms?
  • These changes follow the existing code's practice: allocate a fixed-size (and overly large) buffer, and don't properly handle the error case if it is too small. Should this be fixed? If so should existing code also be fixed?

@duaneg
pythongh-127081: use re-entrant variants of get{proto,serv}by{name,port}
7447706 
Add configure tests and defines for getservbyname_r, getservbyport_r, and
getprotobyname_r. Use these if available, otherwise fallback to the
thread-unsafe variants.

Add a unit test to exercise getprotobyname, which is currently untested.

TODO:
 - Are there any platforms which define the unsafe variants but not the
   re-entrant ones? If not we can simplify the #ifdef hell somewhat.
 - Do the re-entrant functions have the same signature on all platforms?
 - These changes follow the existing code's practice: allocate a fixed-size
   (and overly large) buffer, and don't properly handle the error case if it is
   too small. Should this be fixed? If so should existing code also be fixed?
@bedevere-app bedevere-app bot mentioned this pull request Apr 20, 2025
Open
@picnixz picnixz changed the title gh-127081: use re-entrant variants of get{proto,serv}by{name,port} gh-127081: use re-entrant variants of get{proto,serv}by{name,port} Apr 20, 2025
@erlend-aasland erlend-aasland marked this pull request as draft September 18, 2025 16:23
@erlend-aasland
Copy link
Contributor

Converted to draft since the author still has a TODO list for this PR. Moreover, error handling is missing, so this is a work in progress.

@duaneg
Copy link
Contributor Author

duaneg commented Sep 22, 2025

With regards the TODO questions, I need some guidance from the maintainers...

  1. I suspect there aren't any (supported) platforms that define the original version but not the re-entrant one, so we can simplify configure and the #ifdef clutter by only checking for/using the latter. If the maintainers consider that a potentially worthwhile improvement I'll push an updated patch to be confirmed or otherwise via the buildbots. If it is considered too risky and/or not worth the code churn then fair enough: we can just drop this TODO.
  2. Triggering buildbot tests for this PR would answer the second question. That requires a core dev, though. Also, if we are going to try the #ifdef changes above I suppose it would be better to wait until after that.
  3. It seems like the answer to "should this be fixed" is "yes", which is great! 🙂 As mentioned, I was following the pattern of the existing code (in socket_gethostname, socket_gethostbyname_ex and socket_gethostbyaddr) that doesn't handle buffer overflows gracefully. Personally, I'd prefer to fix the existing code and have everything consistent, but I'd understand if the maintainers would prefer to minimise the scope of change. What approach would you like me to take here? Just add buffer overflow handling to the new code or also fix up the existing code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner will be requested when the pull request is marked ready for review AA-Turner is a code owner

@emmatyping emmatyping Awaiting requested review from emmatyping emmatyping will be requested when the pull request is marked ready for review emmatyping is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@duaneg @erlend-aasland