httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Graham Leggett <minf...@sharp.fm>
Subject Re: Proposed change to mpm_register_socket_callback(): apr_socket_t -> apr_pollfd_t
Date Tue, 15 Mar 2016 19:28:17 GMT
On 15 Mar 2016, at 1:09 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

>> I just gave it a try and it was a lot more elegant than I thought.
>> 
>> You create an array the precise size you need, and off you go.
> 
> That's indeed elegant enough, but I think I prefer the plain array
> solution, still :p
> 
> Don't you like?
> {
>    apr_pollfd_t pfds[3];
>    /* or */
>    apr_pollfd_t *pfds = apr_pcalloc(p, 3 * apr_pollfd_t);
> 
>    pfds[0].desc_type = APR_POLL_SOCKET;
>    pfds[0].desc.s = s0;
>    ...
>    pfds[1].desc_type = APR_POLL_SOCKET;
>    pfds[1].desc.s = s1;
>    ...
>    pfds[2].desc_type = APR_NO_DESC;
> 
>    ap_mpm_register_poll_callback_timeout(pfds, pool, ...);
> }

The trouble with the above is that because of the pool cleanup we now have, pfds[3] needs
to live as long as pool p. In your example it does, but there is nothing stopping someone
trying to allocate pfds[3] on the stack and then returning. Later the cleanup is run, and
boom - difficult to debug crash or weird behaviour.

With the array you’re guaranteed the memory is allocated from a pool, which means the pool
cleanup will always be safe.

What we should also do is drop the apr_pool_t *p parameter and read it from apr_header_array_t’s
pool instead. This will be a further check to stop the caller from doing anything pathological,
as we definitely know the cleanup and the array belong to each other, and our API becomes
simpler still.

Attached patch does this.

Regards,
Graham
—


Mime
View raw message