input validation and removal of unsafe numeric parsing#628
Conversation
|
@notroj look into this |
|
I'll be brutally honest here @metsw24-max - the effect of these comments putting pressure on me to merge your patches is that I will trust you less, and take more time to review your work, not to merge them faster. You're a new contributor here, and we definitely appreciate your PRs. We're under high workload at the moment, give us time. |
|
apologies for that i didn’t mean to put any pressure on the review process. I understand the workload and appreciate you taking the time to review. I will wait for your feedback. |
notroj
left a comment
notroj
left a comment
There was a problem hiding this comment.
Minor style nits otherwise LGTM
| @@ -1139,29 +1188,50 @@ static int balancer_process_balancer_worker(request_rec *r, proxy_server_conf *c | |||
| * on that # character, since the character == the flag | |||
| */ | |||
| if ((val = apr_table_get(params, "w_status_I"))) { | |||
| ap_proxy_set_wstatus(PROXY_WORKER_IGNORE_ERRORS_FLAG, atoi(val), wsel); | |||
| int ival; | |||
There was a problem hiding this comment.
coding style nit, declaring int ival in every single branch here is clearly making the code worse / harder to read, so move it up a context and just declare it once.
| @@ -26,6 +26,9 @@ | |||
| #include "apr_escape.h" | |||
| #include "mod_watchdog.h" | |||
|
|
|||
| #include <errno.h> | |||
| #include <stdlib.h> | |||
There was a problem hiding this comment.
<errno.h> should be inherited via APR headers already, are these really needed?
2 participants
This change hardens balancer-manager parameter handling by replacing permissive numeric parsing
atoi atofwith strict range-checked parsingIt ensures malformed or partially valid inputs are rejected instead of being silently coerced into valid values.