-
Notifications
You must be signed in to change notification settings - Fork 8k
Prevent potential buffer overflow for large value of php_cli_server_workers_max #9000
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
Conversation
Thank you for the PR! I don't like the silent fall back to 1 worker, though. Can't we use safe_emalloc() instead of calloc() (fails hard if unsuccessful), and clear the memory afterwards?
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.
c6b9a02 to
266ccf8
Compare
If I understand it correctly, replacing calloc with safe_emalloc can resolve this. When integer overflow happens, an error message is printed and the program aborts.
sapi/cli/php_cli_server.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I have replaced it with efree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Don t forget the first point :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, @cmb69 will have a last look in case I missed something. Nice work.
2822fb3 to
8cb3735
Compare
sapi/cli/php_cli_server.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8cb3735 to
56c320c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Fixes issue 8989.