Skip to content

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

Merged
arnaud-lb merged 1 commit intophp:PHP-8.2from
arnaud-lb:gh13921-2
Apr 16, 2024
Merged

Use return value of getpwuid_r(), not errno#13969
arnaud-lb merged 1 commit intophp:PHP-8.2from
arnaud-lb:gh13921-2

Conversation

@arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Apr 15, 2024 •
edited
Loading

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
ndossche
ndossche approved these changes Apr 15, 2024
View reviewed changes
Copy link
Member

@ndossche ndossche 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

arnaud-lb commented Apr 16, 2024

@dstogov indeed, I forgot to change the target!

@arnaud-lb arnaud-lb merged commit 32efc76 into php:PHP-8.2 Apr 16, 2024
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Apr 16, 2024
@arnaud-lb
Merge branch 'PHP-8.2' into PHP-8.3
c12fd08 
* 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
@arnaud-lb
Merge branch 'PHP-8.3'
af5db45 
* 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

iluuu1994 commented Apr 19, 2024

@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

devnexen commented Apr 19, 2024

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

iluuu1994 commented Apr 19, 2024

@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

arnaud-lb commented Apr 19, 2024

@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

iluuu1994 commented Apr 19, 2024

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

@arnaud-lb
Copy link
Member Author

arnaud-lb commented Apr 19, 2024

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

Reviewers

@iluuu1994 iluuu1994 iluuu1994 approved these changes

@ndossche ndossche ndossche approved these changes

Assignees

No one assigned

Labels

Extension: posix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@arnaud-lb @dstogov @iluuu1994 @devnexen @ndossche