On 15 Mar 2016, at 1:09 PM, Yann Ylavic <ylavic....@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
—
httpd-register-poll3.patch
Description: Binary data
