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
