You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2020/04/01 07:24:27 UTC
Re: svn commit: r1874775 - /httpd/httpd/trunk/test/README.travis
On 3/31/20 6:31 PM, Yann Ylavic wrote:
> On Tue, Mar 31, 2020 at 7:11 AM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 3/31/20 1:19 AM, Yann Ylavic wrote:
>>> Index: server/core_filters.c
>>> ===================================================================
>>> --- server/core_filters.c (revision 1875881)
>>> +++ server/core_filters.c (working copy)
>>> @@ -543,6 +543,12 @@ static apr_status_t send_brigade_nonblocking(apr_s
>>>
>>> rv = apr_bucket_read(bucket, &data, &length, APR_BLOCK_READ);
>>> }
>>> + if (APR_STATUS_IS_EOF(rv)) {
>>> + /* Morphing bucket exhausted, next. */
>>> + apr_bucket_delete(bucket);
>>> + rv = APR_SUCCESS;
>>> + continue;
>>> + }
>>> if (rv != APR_SUCCESS) {
>>> goto cleanup;
>>> }
>>
>>
>> How is the above related to the issue here? I guess this is something probably all callers to apr_bucket_read need to take care,
>> of, correct?
>
> Since the core output filter can now have to handle morphing buckets
> (not retained by ap_request_core_filter() anymore), I thought it was
> related..
> Agreed that all apr_bucket_read() users should do that.
I have checked socket, pipe and cgi buckets and none of them seem to return EOF. In case they hit EOF they replace themselves with
an immortal bucket of length 0. So above seems just an additional safety if a (future) morphing bucket behaves differently and
returns EOF, but with the current code that path should not be hit really.
Regards
Rüdiger
Re: svn commit: r1874775 - /httpd/httpd/trunk/test/README.travis
Posted by Ruediger Pluem <rp...@apache.org>.
On 4/1/20 11:02 AM, Joe Orton wrote:
> On Wed, Apr 01, 2020 at 10:06:20AM +0200, Ruediger Pluem wrote:
>>
>>
>> On 4/1/20 9:53 AM, Joe Orton wrote:
>>> On Wed, Apr 01, 2020 at 09:24:27AM +0200, Ruediger Pluem wrote:
>>>> I have checked socket, pipe and cgi buckets and none of them seem to return EOF. In case they hit EOF they replace themselves with
>>>> an immortal bucket of length 0. So above seems just an additional safety if a (future) morphing bucket behaves differently and
>>>> returns EOF, but with the current code that path should not be hit really.
>>>
>>> Yeah, the "read till EOF" is an implementation detail for those bucket
>>> types, I would very strongly argue if they ever exposed EOF on read() it
>>> would be a bucket type bug. It could quite possibly obscure a bug
>>> elsewhere if filters ignored EOF on read() so I don't think that should
>>> be recommended.
>>
>> So you recommend that part of the patch to be reverted?
>
> Thinking about it more, the most likely way to hit that condition is a
> FILE bucket where the underlying file is truncated simultaneous to it
> being read (i.e. it's shorter than when the bucket was created). That
> will definitely result in apr_bucket_read() returning EOF, and I think
> should definitely be treated as an error rather than silently ignored.
>
> TL;DR -> yes, or am I missing something?
>
I think you are correct. This should be an error as it can only happen in error cases.
Regards
Rüdiger
Re: svn commit: r1874775 - /httpd/httpd/trunk/test/README.travis
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Apr 01, 2020 at 10:06:20AM +0200, Ruediger Pluem wrote:
>
>
> On 4/1/20 9:53 AM, Joe Orton wrote:
> > On Wed, Apr 01, 2020 at 09:24:27AM +0200, Ruediger Pluem wrote:
> >> I have checked socket, pipe and cgi buckets and none of them seem to return EOF. In case they hit EOF they replace themselves with
> >> an immortal bucket of length 0. So above seems just an additional safety if a (future) morphing bucket behaves differently and
> >> returns EOF, but with the current code that path should not be hit really.
> >
> > Yeah, the "read till EOF" is an implementation detail for those bucket
> > types, I would very strongly argue if they ever exposed EOF on read() it
> > would be a bucket type bug. It could quite possibly obscure a bug
> > elsewhere if filters ignored EOF on read() so I don't think that should
> > be recommended.
>
> So you recommend that part of the patch to be reverted?
Thinking about it more, the most likely way to hit that condition is a
FILE bucket where the underlying file is truncated simultaneous to it
being read (i.e. it's shorter than when the bucket was created). That
will definitely result in apr_bucket_read() returning EOF, and I think
should definitely be treated as an error rather than silently ignored.
TL;DR -> yes, or am I missing something?
Re: svn commit: r1874775 - /httpd/httpd/trunk/test/README.travis
Posted by Ruediger Pluem <rp...@apache.org>.
On 4/1/20 9:53 AM, Joe Orton wrote:
> On Wed, Apr 01, 2020 at 09:24:27AM +0200, Ruediger Pluem wrote:
>> I have checked socket, pipe and cgi buckets and none of them seem to return EOF. In case they hit EOF they replace themselves with
>> an immortal bucket of length 0. So above seems just an additional safety if a (future) morphing bucket behaves differently and
>> returns EOF, but with the current code that path should not be hit really.
>
> Yeah, the "read till EOF" is an implementation detail for those bucket
> types, I would very strongly argue if they ever exposed EOF on read() it
> would be a bucket type bug. It could quite possibly obscure a bug
> elsewhere if filters ignored EOF on read() so I don't think that should
> be recommended.
So you recommend that part of the patch to be reverted?
Regards
Rüdiger
Re: svn commit: r1874775 - /httpd/httpd/trunk/test/README.travis
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Apr 01, 2020 at 09:24:27AM +0200, Ruediger Pluem wrote:
> I have checked socket, pipe and cgi buckets and none of them seem to return EOF. In case they hit EOF they replace themselves with
> an immortal bucket of length 0. So above seems just an additional safety if a (future) morphing bucket behaves differently and
> returns EOF, but with the current code that path should not be hit really.
Yeah, the "read till EOF" is an implementation detail for those bucket
types, I would very strongly argue if they ever exposed EOF on read() it
would be a bucket type bug. It could quite possibly obscure a bug
elsewhere if filters ignored EOF on read() so I don't think that should
be recommended.