You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2020/02/14 09:08:14 UTC
[PATCH] event idle_spawn_rate initialization?
I've been playing with UBSan[1] which catches undefined behaviour, found a
couple of interesting things so far.
One is with event, I messed with the line numbers but the error is:
event.c:3620:13: runtime error: null pointer passed as argument 2, which is declared to never be null
from the memcpy() in this code:
https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3619
new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
memcpy(new_ptr, retained->idle_spawn_rate,
retained->mpm->num_buckets * sizeof(int));
retained->idle_spawn_rate = new_ptr;
retained->mpm->max_buckets = new_max;
At startup it seems retained->idle_spawn_rate is NULL, and you can't
(shouldn't?!) memcpy() from NULL. I am not sure what the right fix is
here, is that array supposed to be initialized to something other than
zero here, or somewhere else? Not obvious if the loop below will
initialize it properly:
https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3624
Is something like this correct? (also this could use apr_pmemdup rather
than palloc+memcpy now I think about it)
--- a/server/mpm/event/event.c
+++ b/server/mpm/event/event.c
@@ -3615,9 +3615,15 @@ static int event_open_logs(apr_pool_t * p, apr_pool_t * plog,
if (new_max < num_buckets) {
new_max = num_buckets;
}
- new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
- memcpy(new_ptr, retained->idle_spawn_rate,
- retained->mpm->num_buckets * sizeof(int));
+ if (retained->idle_spawn_rate) {
+ new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
+ memcpy(new_ptr, retained->idle_spawn_rate,
+ retained->mpm->num_buckets * sizeof(int));
+ }
+ else {
+ /* ### should initialize array to something other than 0?? */
+ new_ptr = apr_pcalloc(ap_pglobal, new_max * sizeof(int));
+ }
retained->idle_spawn_rate = new_ptr;
retained->mpm->max_buckets = new_max;
}
[1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
Re: [PATCH] event idle_spawn_rate initialization?
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Feb 14, 2020 at 11:33:50AM +0100, Ruediger Pluem wrote:
> On 02/14/2020 10:08 AM, Joe Orton wrote:
> > I've been playing with UBSan[1] which catches undefined behaviour, found a
> > couple of interesting things so far.
> >
> > One is with event, I messed with the line numbers but the error is:
> >
> > event.c:3620:13: runtime error: null pointer passed as argument 2, which is declared to never be null
> >
> > from the memcpy() in this code:
> > https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3619
> >
> > new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
> > memcpy(new_ptr, retained->idle_spawn_rate,
> > retained->mpm->num_buckets * sizeof(int));
>
> I guess the above only does not crash because retained->mpm->num_buckets is 0 at the same time.
> But this is probably something we should not rely on with all memcpy implementations.
Ahhhh, yes, that makes more sense now. Thanks a lot.
...
> I guess there is no need for two cases here. We should only avoid the call to memcpy
> if retained->idle_spawn_rate is NULL. The initialization happens then in the block starting
> at line 3624.
Gotcha. Done in r1874011.
Regards, Joe
Re: [PATCH] event idle_spawn_rate initialization?
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/14/2020 10:08 AM, Joe Orton wrote:
> I've been playing with UBSan[1] which catches undefined behaviour, found a
> couple of interesting things so far.
>
> One is with event, I messed with the line numbers but the error is:
>
> event.c:3620:13: runtime error: null pointer passed as argument 2, which is declared to never be null
>
> from the memcpy() in this code:
> https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3619
>
> new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
> memcpy(new_ptr, retained->idle_spawn_rate,
> retained->mpm->num_buckets * sizeof(int));
I guess the above only does not crash because retained->mpm->num_buckets is 0 at the same time.
But this is probably something we should not rely on with all memcpy implementations.
> retained->idle_spawn_rate = new_ptr;
> retained->mpm->max_buckets = new_max;
>
> At startup it seems retained->idle_spawn_rate is NULL, and you can't
> (shouldn't?!) memcpy() from NULL. I am not sure what the right fix is
> here, is that array supposed to be initialized to something other than
> zero here, or somewhere else? Not obvious if the loop below will
> initialize it properly:
I think stuff is correctly initialized in the block starting at line 3624.
>
> https://github.com/apache/httpd/blob/trunk/server/mpm/event/event.c#L3624
>
> Is something like this correct? (also this could use apr_pmemdup rather
> than palloc+memcpy now I think about it)
>
> --- a/server/mpm/event/event.c
> +++ b/server/mpm/event/event.c
> @@ -3615,9 +3615,15 @@ static int event_open_logs(apr_pool_t * p, apr_pool_t * plog,
> if (new_max < num_buckets) {
> new_max = num_buckets;
> }
> - new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
> - memcpy(new_ptr, retained->idle_spawn_rate,
> - retained->mpm->num_buckets * sizeof(int));
> + if (retained->idle_spawn_rate) {
> + new_ptr = (int *)apr_palloc(ap_pglobal, new_max * sizeof(int));
> + memcpy(new_ptr, retained->idle_spawn_rate,
> + retained->mpm->num_buckets * sizeof(int));
> + }
> + else {
> + /* ### should initialize array to something other than 0?? */
> + new_ptr = apr_pcalloc(ap_pglobal, new_max * sizeof(int));
> + }
I guess there is no need for two cases here. We should only avoid the call to memcpy
if retained->idle_spawn_rate is NULL. The initialization happens then in the block starting
at line 3624.
Regards
Rüdiger