Prevent potential buffer overflow for large value of php_cli_server_workers_max #9000
Conversation
|
Thank you for the PR! I don't like the silent fall back to 1 worker, though. Can't we use |
|
I agree here in the sense it s not correct to hide it, you can possibly catch out of range from the value before allocation (like nginx does to catch silly values for its worker processes) or as @cmb69 said, either way need clarity from user's perspective. |
|
If I understand it correctly, replacing |
sapi/cli/php_cli_server.c
Outdated
| php_cli_server_workers = calloc( | ||
| php_cli_server_workers_max, sizeof(pid_t)); | ||
| php_cli_server_workers = safe_emalloc( | ||
| php_cli_server_workers_max, sizeof(pid_t), 0); |
There was a problem hiding this comment.
Two keys here:
1/ > and clear the memory afterwards.
since you re moving from calloc.
2/ You might need a change for free(php_cli_server_workers).
There was a problem hiding this comment.
Ok. I have replaced it with efree.
There was a problem hiding this comment.
Indeed. Don t forget the first point :)
There was a problem hiding this comment.
ecalloc is a safe version of calloc that checks for multiply overflow. I think it can be used here.
There was a problem hiding this comment.
Alright, @cmb69 will have a last look in case I missed something. Nice work.
yiyuaner
force-pushed
the
master
branch
2 times, most recently
from
2822fb3 to
8cb3735
Compare
sapi/cli/php_cli_server.c
Outdated
| @@ -2421,7 +2421,7 @@ static void php_cli_server_startup_workers(void) { | |||
| if (php_cli_server_workers_max > 1) { | |||
| zend_long php_cli_server_worker; | |||
|
|
|||
| php_cli_server_workers = calloc( | |||
| php_cli_server_workers = ecalloc( | |||
There was a problem hiding this comment.
ecalloc() and friends allocate memory which is freed at the end of each request. That would cause issues here. Instead you'd need pecalloc() with the third argument being 1 (and use pefree() instead of efree() above). Note that the Zend memory allocation functions are infallible, i.e. they never return NULL (but instead terminate the process), so the following NULL check is superfluous.
Fixes issue 8989.