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

Use return value of getpwuid_r(), not errno #13969

Merged
merged 1 commit into from
Apr 16, 2024
Merged

Conversation

arnaud-lb
Copy link
Member

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

getpwuid_r() and other reentrant functions used in ext/posix return an error number and may not set errno. #13921 updated ext/posix to use the return value, but one change was forgotten.

#13921 also sets the buffer len to 1 by default in debug builds, which allowed to detect this issue on Mac OS.

@arnaud-lb arnaud-lb marked this pull request as ready for review April 15, 2024 15:23
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Indeed! nice one.
I checked other reentrant ones in this file but this seems to be the only mistake indeed.

@dstogov
Copy link
Member

dstogov commented Apr 16, 2024

Please target this to PHP-8.2

@arnaud-lb arnaud-lb changed the base branch from master to PHP-8.2 April 16, 2024 11:47
@arnaud-lb
Copy link
Member Author

@dstogov indeed, I forgot to change the target!

@arnaud-lb arnaud-lb merged commit 32efc76 into php:PHP-8.2 Apr 16, 2024
10 checks passed
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Apr 16, 2024
* PHP-8.2:
  [ci skip] NEWS
  fix: zend-max-execution-timers with negative or high timeout value (php#13942)
  Use return value of getpwuid_r(), not errno (php#13969)
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Apr 16, 2024
* PHP-8.3:
  [ci skip] NEWS
  [ci skip] NEWS
  fix: zend-max-execution-timers with negative or high timeout value (php#13942)
  Use return value of getpwuid_r(), not errno (php#13969)
@iluuu1994
Copy link
Member

@arnaud-lb It seems we still get some failures on macOS:

https://github.com/php/php-src/actions/runs/8746998656/job/24004796765

========DIFF========
--
       string(%d) "%S"
       ["members"]=>
     %a
008+     [0]=>
009+     string(4) "root"
010+     [1]=>
011+     string(6) "runner"
012+   }
       ["gid"]=>
       int(20)
     }
     bool(false)
017+ 
018+ Termsig=11
========DONE========
FAIL Test posix_getgrnam() function : basic functionality [ext/posix/tests/posix_getgrnam_basic.phpt] 

@devnexen
Copy link
Member

Speaking of macos @iluuu1994 I was unable to reproduce for days the phar module segfault occurring only on intel arch, I have a arm64 version.

@iluuu1994
Copy link
Member

@devnexen Ok, that's unfortunate. Thank you nonetheless. I will try to add ASAN to CI for macOS today, maybe that will help with identification of the issues.

@arnaud-lb
Copy link
Member Author

@iluuu1994

@arnaud-lb It seems we still get some failures on macOS:

https://github.com/php/php-src/actions/runs/8746998656/job/24004796765

I wasn't able to reproduce this one in various configurations including in Valgrind/ASAN. Please let me know if it fails again, I will take an other look.

@iluuu1994
Copy link
Member

@arnaud-lb Did you try a release build? That's what seems to be failing on CI (NTS).

@arnaud-lb
Copy link
Member Author

Yes, I actually tried both but no luck

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.

5 participants