On 9/10/21 2:55 PM, Yann Ylavic wrote:
> On Fri, Sep 10, 2021 at 1:32 PM Yann Ylavic <ylavic....@gmail.com> wrote:
>>
>> On Fri, Sep 10, 2021 at 11:12 AM Ruediger Pluem <rpl...@apache.org> wrote:
>>>
>>> On 9/10/21 11:04 AM, Ruediger Pluem wrote:
>>>>
>>>> On 9/10/21 8:55 AM, Joe Orton wrote:
>>>>> On Fri, Sep 10, 2021 at 12:51:53AM -0000, yla...@apache.org wrote:
>>>>>>
>>>>>> apr_file_setaside: don't blindly kill the old cleanup and file
>>>>>> descriptor.
>>>>>>
>>>>>> There is no cleanup with APR_FOPEN_NOCLEANUP, so apr_pool_cleanup_kill()
>>>>>> can
>>>>>> go in the !(old_file->flags & APR_FOPEN_NOCLEANUP) block.
>>>>>>
>>>>>> The file descriptor can't be invalidated either, the file may be split in
>>>>>> multiple buckets and setting aside one shouldn't invalidate the others.
>>>>>
>>>>> Interesting. So is the API claim that:
>>>>>
>>>>> * @remark After calling this function, old_file may not be used
>>>>>
>>>>> not really correct, or needs to be weakened?
>>>>
>>>> I think it is correct, for the reason you state below and the reason I
>>>> mentioned,
>>>> but I guess we probably need to review in httpd land if we still use the
>>>> old_file
>>>> afterwards e.g. when buckets have been split.
>>
>> The fix still belongs in APR though, being able to split and setaside
>> file buckets is broken for now.
>>
>>>
>>> I think the old code with setting the file descriptor of old_file to -1,
>>> should reveal
>>> in a clear way if we still use the old file afterwards. Keeping the file
>>> descriptor may
>>> lead to subtle failures some time later that are hard to debug.
>>
>> OK, the semantics of apr_file_setaside() are to take ownership
>> (invalidate the old fd) and we probably can't change that.
>>
>> So we need to fix file_bucket_setaside() somehow, like this?
>
> Well, this rather:
>
> Index: buckets/apr_buckets_file.c
> ===================================================================
> --- buckets/apr_buckets_file.c (revision 1893196)
> +++ buckets/apr_buckets_file.c (working copy)
> @@ -18,6 +18,7 @@
> #include "apr_general.h"
> #include "apr_file_io.h"
> #include "apr_buckets.h"
> +#include "apr_strings.h" /* for apr_pmemdup() */
>
> #if APR_HAS_MMAP
> #include "apr_mmap.h"
> @@ -218,16 +219,34 @@ static apr_status_t file_bucket_setaside(apr_bucke
> apr_file_t *fd = NULL;
> apr_file_t *f = a->fd;
> apr_pool_t *curpool = apr_file_pool_get(f);
> + apr_status_t rv;
>
> if (apr_pool_is_ancestor(curpool, reqpool)) {
> return APR_SUCCESS;
> }
>
> + /* If the file is shared/split this bucket can't take exclusive ownership
> + * with apr_file_setaside() (which invalidates the current/old a->fd,) so
> + * we need to dup in this case.
> + */
> + if (a->refcount.refcount > 1) {
> + rv = apr_file_dup(&fd, f, reqpool);
> + if (rv != APR_SUCCESS) {
> + return rv;
> + }
> + a->refcount.refcount--;
> + a = data->data = apr_pmemdup(reqpool, a, sizeof(*a));
Don't you need to create the memory for a / data->data via
apr_bucket_alloc(sizeof(*a), data->list)
like we do in apr_bucket_file_make for this data structure?
Something like
a = apr_bucket_alloc(sizeof(*a), data->list);
*a = *(data->data); // or memcpy(a, data->data, sizeof(*a))
data->data = a;
Otherwise looks good.
And you would need to bring back
old_file->filedes = -1
as a partial revert of r1893204?
Regards
Rüdiger
