Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <CAP_GPNgkidUCDo4iB9kGdEQX=81usqinB9oF3Au8ZMe-PpxFCw@mail.gmail.com>
References: <20160824144322.8E4FD3A02E6@svn01-us-west.apache.org>
 <CABw-3YdcLSgjmGoOfqPEVL_=uTBzfQ7vAk79m_awOz_-95yBcg@mail.gmail.com> <CAP_GPNgkidUCDo4iB9kGdEQX=81usqinB9oF3Au8ZMe-PpxFCw@mail.gmail.com>
From: Ivan Zhakov <ivan@visualsvn.com>
Date: Fri, 26 Aug 2016 14:29:35 +0300
Message-ID: <CABw-3YeqfU8rTbtocBB0CSsFk8Jo0jLkaU36wkq4iYOVdKqcbg@mail.gmail.com>
Subject: Re: svn commit: r1757532 - /subversion/trunk/subversion/mod_dav_svn/repos.c
To: Evgeny Kotkov <evgeny.kotkov@visualsvn.com>
Cc: Subversion Development <dev@subversion.apache.org>
Content-Type: text/plain; charset=UTF-8
archived-at: Fri, 26 Aug 2016 11:30:03 -0000

On 26 August 2016 at 14:27, Evgeny Kotkov <evgeny.kotkov@visualsvn.com> wrote:
> Ivan Zhakov <ivan@visualsvn.com> writes:
>
>>> @@ -3266,15 +3264,13 @@ static svn_error_t *  __attribute__((war
>>>  close_filter(void *baton)
>>>  {
>>>    diff_ctx_t *dc = baton;
>>> -  apr_bucket_brigade *bb;
>>>    apr_bucket *bkt;
>>>    apr_status_t status;
>>>
>>>    /* done with the file. write an EOS bucket now. */
>>> -  bb = apr_brigade_create(dc->pool, dc->output->c->bucket_alloc);
>>>    bkt = apr_bucket_eos_create(dc->output->c->bucket_alloc);
>>> -  APR_BRIGADE_INSERT_TAIL(bb, bkt);
>>> -  if ((status = ap_pass_brigade(dc->output, bb)) != APR_SUCCESS)
>>> +  APR_BRIGADE_INSERT_TAIL(dc->bb, bkt);
>>> +  if ((status = ap_pass_brigade(dc->output, dc->bb)) != APR_SUCCESS)
>>>      return svn_error_create(status, NULL, "Could not write EOS to filter");
>>>
>> As far I understand apr_brigade_cleanup() should be called after
>> ap_pass_brigade(). So code should be something like:
>> [[[
>>  APR_BRIGADE_INSERT_TAIL(dc->bb, bkt);
>>  status = ap_pass_brigade(dc->output, dc->bb);
>>  apr_brigade_cleanup(dc->bb);
>>  if (status != APR_SUCCESS)
>>       return svn_error_create(status, NULL, "Could not write EOS to filter");
>> ]]]
>
> Thanks, I will add the missing call in trunk.
>
> While it's not strictly required here (this is the stream's close_fn(), and
> we always call apr_brigade_destroy() upon returning from the deliver()
> hook), this may not hold in the future.  Or someone might improperly use
> this code as a reference.
>
Ok, thanks! I added my +1 for backport anyway, since there is no
actual problem/memory leak in this particular case.


-- 
Ivan Zhakov