Fix error case memory bug in GetHostName()

[jowlyzhang]

Fix the error handling in GetHostName for non EFAULT, non EINVAL error. Current handling will cause stack overflow when non null-terminated c style string is in name, e.g. ENAMETOOLONG, when the name buffer is not big enough and the host name is truncated.

Test Plan:

COMPILE_WITH_ASAN=1 make all check

[facebook-github-bot]

@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[pdillinger]

Thanks for the fix! I suggest a more clear and direct summary like "Fix error case memory bug in GetHostName()". In case you were taking inspiration from some of the soft language in my PR #11538, that is kind of a special case where it's hard to promise what is actually safe usage during static destruction and we nevertheless need to make reasonable accommodations to avoid bugs or crashes, as there might not be a clear path to guaranteed safe usage.

In this case, the old code was completely unsafe (on an error handling path).

Rant: It would be nice to deprecate and get rid of this API with its C-style calling conventions, but that's probably more work than it's worth. Back to the important things.

[jowlyzhang]

Thanks for the fix! I suggest a more clear and direct summary like "Fix error case memory bug in GetHostName()". In case you were taking inspiration from some of the soft language in my PR #11538, that is kind of a special case where it's hard to promise what is actually safe usage during static destruction and we nevertheless need to make reasonable accommodations to avoid bugs or crashes, as there might not be a clear path to guaranteed safe usage.

In this case, the old code was completely unsafe (on an error handling path).

Rant: It would be nice to deprecate and get rid of this API with its C-style calling conventions, but that's probably more work than it's worth. Back to the important things.

Thanks Peter for the review and the suggested summary. You are right that this is not just a good-to-have improvement, but an absolutely necessary fix for an incorrectly handled error case.

[facebook-github-bot]

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@jowlyzhang merged this pull request in 6649978.