Skip to content

gh-106505: don't lookup own hostname in getfqdn() - #106506

Open
bluelhf wants to merge 9 commits into
python:mainfrom
bluelhf:main
Open

gh-106505: don't lookup own hostname in getfqdn()#106506
bluelhf wants to merge 9 commits into
python:mainfrom
bluelhf:main

Conversation

@bluelhf

@bluelhf bluelhf commented Jul 7, 2023

Copy link
Copy Markdown
  • return gethostname() immediately instead of resolving it

@ghost

ghost commented Jul 7, 2023

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@bluelhf bluelhf changed the title gh-106505 fix(socket): don't lookup own hostname in getfqdn() gh-106505: don't lookup own hostname in getfqdn() Jul 7, 2023

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it covered by existing tests? If no, please add tests.

Also, please correct the NEWS entry. The sentence should start with a title character and end with a period.

@bluelhf

bluelhf commented Feb 2, 2024

Copy link
Copy Markdown
Author

@serhiy-storchaka Thanks, I'll fix the NEWS entry. For the tests, though, can you give any advice on how I could implement the test, since the correct result depends on the OS configuration? Is it OK to just call gethostname() and check that the function returns that hostname for empty string and the specified loopback addresses?

@serhiy-storchaka

Copy link
Copy Markdown
Member

I do not know. Does this change affect any of our buildbots?

I would write a PR containing test that compares the result of the old implementation of getfqdn('') with gethostname() and signal if they are different. Also print any interesting details. Then modify some file in Lib/test/libregrtest so that only this test (or only this test file) be run. And run it an buildbots. If there are some interesting results, we can use it to create a test. And it may expose some flaw in this PR, although it does not guaranee this.

NEWS entries must start with a title case letter and end with a period — the entry was also rewritten to match the usual wording of entries
@bluelhf

bluelhf commented Mar 6, 2024

Copy link
Copy Markdown
Author

I do not know. Does this change affect any of our buildbots?

I would write a PR containing test that compares the result of the old implementation of getfqdn('') with gethostname() and signal if they are different. Also print any interesting details. Then modify some file in Lib/test/libregrtest so that only this test (or only this test file) be run. And run it an buildbots. If there are some interesting results, we can use it to create a test. And it may expose some flaw in this PR, although it does not guaranee this.

In many cases, and I assume for all Python buildbots, such tests would pass, as gethostbyaddr would throw an error when passing an invalid address (the computer's hostname), which would cause the hostname to be returned as-is. Sometimes, though, the hostname resolution may take an extended period of time (which is how I originally found this issue, with python -m http.server taking upwards of 10 seconds to start), and since it's incorrect to resolve a hostname as if it were an address anyway, it would be better to just return the hostname without any resolution (as documented!)

For tests, I could write some that mock gethostbyaddr to ensure that it isn't called when an empty name, "::" or "0.0.0.0" are passed to getfqdn()?

@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 13, 2026
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants