Skip to content
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

Fix usage of reentrant functions in ext/posix #13921

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Apr 9, 2024

  • It's not necessarily an error when sysconf(_SC_GETPW_R_SIZE_MAX) returns -1, as specified by posix (and the musl implementation always returns -1). Pick an initial buffer size in this case.
  • Reentrant variants return an error number an may not set errno
  • Implement retry logic for ttyname_r()
  • Fix retry logic for getpwnam_r() (pw would be NULL after the first try)
  • Test retry logic by setting the initial buffer size to 1 in debug builds

Tests for these functions would fail under musl due to sysconf(_SC_GETPW_R_SIZE_MAX) returning -1 (which is allowed by the standard). The test suite passed on other platforms, but these functions could fail under unusual conditions. Setting buflen to 1 ensures that the retry logic is tested.

- It's not necessarily an error of sysconf(_SC_GETPW_R_SIZE_MAX) returns -1, as
  specified by posix (and the musl implementation always returns -1). Pick an
  initial buffer size in this case.
- Reentrant variants return an error number an may not set errno
- Implement retry logic for ttyname_r()
- Fix retry logic for getpwnam_r() (pw would be NULL after the first try)
- Test retry logic by setting the initial buffer size to 1 in debug builds
@devnexen
Copy link
Member

devnexen commented Apr 9, 2024

I think a somewhat similar PR was started earlier but maybe I confuse.

@arnaud-lb arnaud-lb mentioned this pull request Apr 9, 2024
@arnaud-lb
Copy link
Member Author

Indeed, there appears to be some overlap with #13922

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

Looking good. Do you really want to keep the debug part ? :)

@arnaud-lb
Copy link
Member Author

Thank you!

Yes, this ensures that the retry code paths are tested. This doesn't negatively impact the duration of the test suite.

@arnaud-lb arnaud-lb merged commit 66809c0 into php:PHP-8.2 Apr 11, 2024
8 checks passed
arnaud-lb added a commit that referenced this pull request Apr 11, 2024
* PHP-8.2:
  [ci skip] NEWS
  Fix usage of reentrant functions in ext/posix (#13921)
arnaud-lb added a commit that referenced this pull request Apr 11, 2024
* PHP-8.3:
  [ci skip] NEWS
  [ci skip] NEWS
  Fix usage of reentrant functions in ext/posix (#13921)
@iluuu1994
Copy link
Member

Is this responsible for the failures in nightly? https://github.com/php/php-src/actions/runs/8682420096/job/23806945143

@arnaud-lb
Copy link
Member Author

It's possible. I will check!

@dstogov
Copy link
Member

dstogov commented Apr 16, 2024

The nightly failures are definitely related to this patch.
See https://github.com/php/php-src/actions/runs/8655545925/job/23734685655
ext/posix/tests/posix_getpwnam_basic_01.phpt and ext/posix/tests/posix_getpwuid_basic.phpt are failed only on MAC and only with DEBUG_ZTS build. It seems getgrnam_r() "test retry logic" doesn't work properly.

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.

4 participants