Skip to content

Commit

Permalink
Fix get/set priority - error handling for MacOS and extra tests
Browse files Browse the repository at this point in the history 
Closes GH-9044.
  • Loading branch information
@cmb69
jcm authored and cmb69 committed Jul 27, 2022
1 parent a398a2f commit 520bb2e
Show file tree
Hide file tree
Showing 7 changed files with 179 additions and 8 deletions.
3 changes: 3 additions & 0 deletions NEWS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ PHP NEWS
. Fixed bug GH-9090 (Support assigning function pointers in FFI). (Adam
Saponara)

- PCNTL:
. Fixed pcntl_(get|set)priority error handling for MacOS. (Juan Morales)

- Random:
. Fixed bug GH-9067 (random extension is not thread safe). (cmb)
. Fixed bug GH-9055 (segmentation fault if user engine throws). (timwolla)
Expand Down
37 changes: 35 additions & 2 deletions ext/pcntl/pcntl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1265,7 +1265,8 @@ PHP_FUNCTION(pcntl_getpriority)
/* needs to be cleared, since any returned value is valid */
errno = 0;

pri = getpriority(who, pid_is_null ? getpid() : pid);
pid = pid_is_null ? getpid() : pid;
pri = getpriority(who, pid);

if (errno) {
PCNTL_G(last_error) = errno;
Expand All @@ -1274,8 +1275,22 @@ PHP_FUNCTION(pcntl_getpriority)
php_error_docref(NULL, E_WARNING, "Error %d: No process was located using the given parameters", errno);
break;
case EINVAL:
#ifdef PRIO_DARWIN_BG
if (who != PRIO_PGRP && who != PRIO_USER && who != PRIO_PROCESS && who != PRIO_DARWIN_THREAD) {
zend_argument_value_error(2, "must be one of PRIO_PGRP, PRIO_USER, PRIO_PROCESS or PRIO_DARWIN_THREAD");
RETURN_THROWS();
} else if (who == PRIO_DARWIN_THREAD && pid != 0) {
zend_argument_value_error(1, "must be 0 (zero) if PRIO_DARWIN_THREAD is provided as second parameter");
RETURN_THROWS();
} else {
zend_argument_value_error(1, "is not a valid process, process group, or user ID");
RETURN_THROWS();
}
#else
zend_argument_value_error(2, "must be one of PRIO_PGRP, PRIO_USER, or PRIO_PROCESS");
RETURN_THROWS();
#endif

default:
php_error_docref(NULL, E_WARNING, "Unknown error %d has occurred", errno);
break;
Expand Down Expand Up @@ -1304,15 +1319,33 @@ PHP_FUNCTION(pcntl_setpriority)
Z_PARAM_LONG(who)
ZEND_PARSE_PARAMETERS_END();

if (setpriority(who, pid_is_null ? getpid() : pid, pri)) {
pid = pid_is_null ? getpid() : pid;

if (setpriority(who, pid, pri)) {
PCNTL_G(last_error) = errno;
switch (errno) {
case ESRCH:
php_error_docref(NULL, E_WARNING, "Error %d: No process was located using the given parameters", errno);
break;
case EINVAL:
#ifdef PRIO_DARWIN_BG
if (who != PRIO_PGRP && who != PRIO_USER && who != PRIO_PROCESS && who != PRIO_DARWIN_THREAD) {
zend_argument_value_error(3, "must be one of PRIO_PGRP, PRIO_USER, PRIO_PROCESS or PRIO_DARWIN_THREAD");
RETURN_THROWS();
} else if (who == PRIO_DARWIN_THREAD && pid != 0) {
zend_argument_value_error(2, "must be 0 (zero) if PRIO_DARWIN_THREAD is provided as second parameter");
RETURN_THROWS();
} else if (who == PRIO_DARWIN_THREAD && pid == 0 && (pri != 0 && pri != PRIO_DARWIN_BG)) {
zend_argument_value_error(1, "must be either 0 (zero) or PRIO_DARWIN_BG, for mode PRIO_DARWIN_THREAD");
RETURN_THROWS();
} else {
zend_argument_value_error(2, "is not a valid process, process group, or user ID");
RETURN_THROWS();
}
#else
zend_argument_value_error(3, "must be one of PRIO_PGRP, PRIO_USER, or PRIO_PROCESS");
RETURN_THROWS();
#endif
case EPERM:
php_error_docref(NULL, E_WARNING, "Error %d: A process was located, but neither its effective nor real user ID matched the effective user ID of the caller", errno);
break;
Expand Down
16 changes: 13 additions & 3 deletions ext/pcntl/tests/pcntl_getpriority_error.phpt
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
--TEST--
pcntl_getpriority() - Wrong process identifier
pcntl_getpriority() - Wrong mode passed and also for non existing process id provided
--EXTENSIONS--
pcntl
--SKIPIF--
Expand All @@ -8,16 +8,26 @@ pcntl
if (!function_exists('pcntl_getpriority')) {
die('skip pcntl_getpriority doesn\'t exist');
}

if (PHP_OS == "Darwin") {
die("skip This test is not for Darwin");
}

?>
--FILE--
<?php

try {
pcntl_getpriority(null, 42);
pcntl_getpriority(null, PRIO_PGRP + PRIO_USER + PRIO_PROCESS + 10);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

// Different behavior in MacOS than rest of operating systems
pcntl_getpriority(-1, PRIO_PROCESS);

?>
--EXPECT--
--EXPECTF--
pcntl_getpriority(): Argument #2 ($mode) must be one of PRIO_PGRP, PRIO_USER, or PRIO_PROCESS

Warning: pcntl_getpriority(): Error %d: No process was located using the given parameters in %s
43 changes: 43 additions & 0 deletions ext/pcntl/tests/pcntl_getpriority_error_darwin.phpt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
--TEST--
pcntl_getpriority() - Wrong mode passed and also for non existing process id provided
--EXTENSIONS--
pcntl
--SKIPIF--
<?php

if (!function_exists('pcntl_getpriority')) {
die('skip pcntl_getpriority doesn\'t exist');
}

if (PHP_OS !== "Darwin") {
die("skip This test only runs on Darwin");
}

?>
--FILE--
<?php

try {
pcntl_getpriority(null, (PRIO_PGRP + PRIO_USER + PRIO_PROCESS + 10));
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

try {
pcntl_getpriority(-1, PRIO_DARWIN_THREAD);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

try {
// Different behavior in MacOS than rest of operating systems
pcntl_getpriority(-1, PRIO_PROCESS);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

?>
--EXPECT--
pcntl_getpriority(): Argument #2 ($mode) must be one of PRIO_PGRP, PRIO_USER, PRIO_PROCESS or PRIO_DARWIN_THREAD
pcntl_getpriority(): Argument #1 ($process_id) must be 0 (zero) if PRIO_DARWIN_THREAD is provided as second parameter
pcntl_getpriority(): Argument #1 ($process_id) is not a valid process, process group, or user ID
15 changes: 12 additions & 3 deletions ext/pcntl/tests/pcntl_setpriority_error.phpt
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
--TEST--
pcntl_setpriority() - Wrong process identifier
pcntl_setpriority() - Check for errors
--EXTENSIONS--
pcntl
--SKIPIF--
Expand All @@ -8,16 +8,25 @@ pcntl
if (!function_exists('pcntl_setpriority')) {
die('skip pcntl_setpriority doesn\'t exist');
}

if (PHP_OS == "Darwin") {
die("skip This test is not for Darwin");
}

?>
--FILE--
<?php

try {
pcntl_setpriority(0, null, 42);
$result = pcntl_setpriority(0, null, (PRIO_PGRP + PRIO_USER + PRIO_PROCESS + 10));
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

pcntl_setpriority(0, -123);

?>
--EXPECT--
--EXPECTF--
pcntl_setpriority(): Argument #3 ($mode) must be one of PRIO_PGRP, PRIO_USER, or PRIO_PROCESS

Warning: pcntl_setpriority(): Error 3: No process was located using the given parameters in %s
46 changes: 46 additions & 0 deletions ext/pcntl/tests/pcntl_setpriority_error_darwin.phpt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
--TEST--
pcntl_setpriority() - Check for errors
--EXTENSIONS--
pcntl
--SKIPIF--
<?php

if (!function_exists('pcntl_setpriority')) {
die('skip pcntl_setpriority doesn\'t exist');
}

if (PHP_OS !== "Darwin") {
die("skip This test only runs on Darwin");
}

?>
--FILE--
<?php

try {
pcntl_setpriority(0, null, (PRIO_PGRP + PRIO_USER + PRIO_PROCESS + 10));
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

try {
pcntl_setpriority(0, -1, PRIO_DARWIN_THREAD);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

try {
pcntl_setpriority(0, -123);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

pcntl_setpriority(-1000, 1);

?>
--EXPECTF--
pcntl_setpriority(): Argument #3 ($mode) must be one of PRIO_PGRP, PRIO_USER, PRIO_PROCESS or PRIO_DARWIN_THREAD
pcntl_setpriority(): Argument #2 ($process_id) must be 0 (zero) if PRIO_DARWIN_THREAD is provided as second parameter
pcntl_setpriority(): Argument #2 ($process_id) is not a valid process, process group, or user ID

Warning: pcntl_setpriority(): Error 1: A process was located, but neither its effective nor real user ID matched the effective user ID of the caller in %s
27 changes: 27 additions & 0 deletions ext/pcntl/tests/pcntl_setpriority_error_linux.phpt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
--TEST--
pcntl_setpriority() - Check for errors
--EXTENSIONS--
pcntl
--SKIPIF--
<?php

if (!function_exists('pcntl_setpriority')) {
die('skip pcntl_setpriority doesn\'t exist');
}

if (PHP_OS !== "Linux") {
die("skip This test only runs on Linux");
}

?>
--FILE--
<?php

pcntl_setpriority(-1000, 1);
pcntl_setpriority(-1000, 0);

?>
--EXPECTF--
Warning: pcntl_setpriority(): Error 1: A process was located, but neither its effective nor real user ID matched the effective user ID of the caller in %s

Warning: pcntl_setpriority(): Error 13: Only a super user may attempt to increase the process priority in %s on line %d

13 comments on commit 520bb2e

@drupol
Copy link
Contributor

@drupol drupol commented on 520bb2e Aug 7, 2022

Choose a reason for hiding this comment

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

Since this change, we cannot build php8.2-beta2. See fossar/nix-phps#134

It seems that the issue comes from the new test introduced in pcntl_setpriority_error_linux.phpt.

@cmb69
Copy link
Member

@cmb69 cmb69 commented on 520bb2e Aug 7, 2022

Choose a reason for hiding this comment

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

Can you please provide the diff of the test (passing --show-diff to the test runner might be sufficient to get it).

@drupol
Copy link
Contributor

@drupol drupol commented on 520bb2e Aug 7, 2022

Choose a reason for hiding this comment

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

make test --show-diff ?

@cmb69
Copy link
Member

@cmb69 cmb69 commented on 520bb2e Aug 8, 2022

Choose a reason for hiding this comment

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

Not quite. If you're now doing make test, then you'd need to do make test TESTS=--show-diff.

@drupol
Copy link
Contributor

@drupol drupol commented on 520bb2e Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here you go, the full build log of the pcntl extension.

test.log

The diff is:

========DIFF========
+ Warning: pcntl_setpriority(): Error 13: Only a super user may attempt to increase the process priority in /build/php-8.2.0beta2/ext/pcntl/tests/pcntl_setpriority_error_linux.php on line 3
- Warning: pcntl_setpriority(): Error 1: A process was located, but neither its effective nor real user ID matched the effective user ID of the caller in %s
     
     Warning: pcntl_setpriority(): Error 13: Only a super user may attempt to increase the process priority in %s on line %d
========DONE========

@cmb69
Copy link
Member

@cmb69 cmb69 commented on 520bb2e Aug 8, 2022

Choose a reason for hiding this comment

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

Thanks! So the diff is basically:

+ Warning: pcntl_setpriority(): Error 13: Only a super user may attempt to increase the process priority in /build/php-8.2.0beta2/ext/pcntl/tests/pcntl_setpriority_error_linux.php on line 3
- Warning: pcntl_setpriority(): Error 1: A process was located, but neither its effective nor real user ID matched the effective user ID of the caller in %s

Looks like we need to fix the test case (the same issue is likely to happen, if we stop running CI as root).

@juan-morales, any ideas how to solve this?

@juan-morales
Copy link
Contributor

Choose a reason for hiding this comment

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

I am looking on this @cmb69 @drupol

@juan-morales
Copy link
Contributor

Choose a reason for hiding this comment

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

@drupol can you tell me under which user is your CI running?

I tried all kind of ideas trying to replicate your output but I could not.

Locally for me works as expected, using latest version of php's repo and also using tag php-8.2.0beta2 (used by you as far as I saw), and works as expected.

According to your output ...

pcntl_setpriority(-1000, 1); //<--- This one did not fail as expected, so the user running the test was able to change priority of process ID 1, in linux, belongs to root user (init process)
pcntl_setpriority(-1000, 0); //<---- This one is the only one that failed as expected

Did you try running the CI as non-root user? if user is not root, how about the privileges used? sudo? I will be waiting for your response.

regards!

@drupol
Copy link
Contributor

@drupol drupol commented on 520bb2e Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi,

Thanks for investigating!

After further investigations on our side, we noticed that the variable '$USER` was empty while running the tests. Then @jtojnar had the idea to turn the sandboxing mechanism off in the Nix build system, and it worked.

I guess the issue is on our side now, unless you need more details.

@juan-morales
Copy link
Contributor

Choose a reason for hiding this comment

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

Very happy to know that there is a solution for this.

Have a good day.

@drupol
Copy link
Contributor

@drupol drupol commented on 520bb2e Aug 8, 2022

Choose a reason for hiding this comment

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

Question:
Do you think it would be possible to skip tests in environments that doesn't meet the requirements?

@juan-morales
Copy link
Contributor

@juan-morales juan-morales commented on 520bb2e Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

From my side... No problem at all, I can write the code, then is up to the main contributors to approve it or not.

I will write it tomorrow because now I Am travelling

@drupol
Copy link
Contributor

@drupol drupol commented on 520bb2e Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Excellent news. Looking forward to testing the patch and the next beta3 :)

Please sign in to comment.