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 12:04:21 UTC
Re: svn commit: r1875881 -
/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
On 3/30/20 3:18 PM, jorton@apache.org wrote:
> Author: jorton
> Date: Mon Mar 30 13:18:29 2020
> New Revision: 1875881
>
> URL: http://svn.apache.org/viewvc?rev=1875881&view=rev
> Log:
> * modules/ssl/ssl_engine_io.c: (ssl_io_filter_coalesce): Handle the
> case of a bucket which morphs to a bucket short enough to fit within
> the buffer without needing to split.
>
> Modified:
> httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
>
> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c?rev=1875881&r1=1875880&r2=1875881&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Mon Mar 30 13:18:29 2020
> @@ -1749,7 +1749,17 @@ static apr_status_t ssl_io_filter_coales
> }
> }
>
> - rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
> + /* If the read above made the bucket morph, it may now fit
> + * entirely within the buffer. Otherwise, split it so it does
> + * fit. */
> + if (e->length < COALESCE_BYTES
> + && e->length + buffered + bytes < COALESCE_BYTES) {
> + rv = APR_SUCCESS;
Hmm. If we had e->length == -1 above and the bucket read failed, e might still be the morphing bucket and hence e->length == -1.
I think all the code below assumes e->length >= 0 things can get off the rails.
How about the following patch (minus whitespace changes) to fix this:
Index: ssl_engine_io.c
===================================================================
--- ssl_engine_io.c (revision 1875997)
+++ ssl_engine_io.c (working copy)
@@ -1739,7 +1739,7 @@
&& bytes + buffered < COALESCE_BYTES
&& e != APR_BRIGADE_SENTINEL(bb)
&& !APR_BUCKET_IS_METADATA(e)) {
- apr_status_t rv;
+ apr_status_t rv = APR_SUCCESS;
/* For an indeterminate length bucket (PIPE/CGI/...), try a
* non-blocking read to have it morph into a HEAP. If the
@@ -1753,17 +1753,16 @@
if (rv != APR_SUCCESS && !APR_STATUS_IS_EAGAIN(rv)) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, f->c, APLOGNO(10232)
"coalesce failed to read from data bucket");
+ return AP_FILTER_ERROR;
}
}
+ if (rv == APR_SUCCESS) {
/* If the read above made the bucket morph, it may now fit
* entirely within the buffer. Otherwise, split it so it does
* fit. */
- if (e->length < COALESCE_BYTES
- && e->length + buffered + bytes < COALESCE_BYTES) {
- rv = APR_SUCCESS;
- }
- else {
+ if (e->length >= COALESCE_BYTES
+ || e->length + buffered + bytes >= COALESCE_BYTES) {
rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
}
@@ -1787,6 +1786,7 @@
return AP_FILTER_ERROR;
}
}
+ }
upto = e;
Regards
Rüdiger
Re: svn commit: r1875881 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Apr 1, 2020 at 2:39 PM Joe Orton <jo...@redhat.com> wrote:
>
> On Wed, Apr 01, 2020 at 02:04:21PM +0200, Ruediger Pluem wrote:
> > On 3/30/20 3:18 PM, jorton@apache.org wrote:
> > >
> > > - rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
> > > + /* If the read above made the bucket morph, it may now fit
> > > + * entirely within the buffer. Otherwise, split it so it does
> > > + * fit. */
> > > + if (e->length < COALESCE_BYTES
> > > + && e->length + buffered + bytes < COALESCE_BYTES) {
> > > + rv = APR_SUCCESS;
> >
> > Hmm. If we had e->length == -1 above and the bucket read failed, e might still be the morphing bucket and hence e->length == -1.
> > I think all the code below assumes e->length >= 0 things can get off the rails.
>
> Thanks a lot for the review... I tried to keep that as simple as
> possible but there are too many cases to cover. Yep, you're right.
>
> > How about the following patch (minus whitespace changes) to fix this:
>
> +1 that looks correct to me, please commit (or I can...)
+1, a morphing bucket that reads to another morphing bucket is way
more terrible than one returning EOF :)
Speaking of the latter, r1876000 ;)
Re: svn commit: r1875881 -
/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Apr 02, 2020 at 10:37:01AM +0200, Ruediger Pluem wrote:
> Sounds reasonable. Thanks for explaining. Do you fix <= vs < and > vs >= or should I?
http://svn.apache.org/viewvc?view=revision&revision=1876037
I hope I have tweaked this to death now, but more review definitely
welcome since I found another way to (prematurely?) optimize the logic
after trawling through some debug logs. Also really need some robust
tests for this since it's a pain to validate the behaviour across
various edge cases.
(and I am even more convinced this filter really should not exist and
the buffering could be done more efficiently in ssl_io_filter_output but
I will have to wait for a spare few days to try that...)
Regards, Joe
Re: svn commit: r1875881 -
/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 4/2/20 9:36 AM, Joe Orton wrote:
> On Thu, Apr 02, 2020 at 07:30:50AM +0200, Ruediger Pluem wrote:
>> On 4/1/20 11:44 PM, Mike Rumph wrote:
>>> I'm not very familiar with this code, so my questions might not make sense.
>>>
>>> But take a look at the following two code segments in ssl_io_filter_coalesce():
>>>
>>> for (e = APR_BRIGADE_FIRST(bb);
>>> e != APR_BRIGADE_SENTINEL(bb)
>>> && !APR_BUCKET_IS_METADATA(e)
>>> && e->length != (apr_size_t)-1
>>> && e->length < COALESCE_BYTES
>>> && (buffered + bytes + e->length) < COALESCE_BYTES;
>>>
>>> and
>>>
>>> if (rv == APR_SUCCESS) {
>>> /* If the read above made the bucket morph, it may now fit
>>> * entirely within the buffer. Otherwise, split it so it does
>>> * fit. */
>>> if (e->length >= COALESCE_BYTES
>>> || e->length + buffered + bytes >= COALESCE_BYTES) {
>>> rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
>>> }
>>>
>>> Does this mean that either buffered or bytes might be negative?
>>> Otherwise, testing both "e->length" and "e->length + buffered + bytes" against COALESCE_BYTES seems redundant.
>>
>> I guess they are some sort redundant. Joe?
>
> I was being paranoid about unsigned integer overflow when writing this.
> (e->length + X) may overflow, comparing e->length against COALESCE_BYTES
> in both cases prevents that since max(bytes + buffered)=COALESCE_BYTES.
>
> There is nothing preventing someone creating a bucket (e.g. FILE) with
> e->length = (apr_size_t)-2 and passing it through the filter stack
> AFAIK, so I think paranoia is reasonable.
>
> But now you made me read the code *again* I think those tests should be
> e->length <= COALESCE_BYTES and e->length > COALESCE_BYTES etc.
Sounds reasonable. Thanks for explaining. Do you fix <= vs < and > vs >= or should I?
Regards
Rüdiger
Re: svn commit: r1875881 -
/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Apr 02, 2020 at 07:30:50AM +0200, Ruediger Pluem wrote:
> On 4/1/20 11:44 PM, Mike Rumph wrote:
> > I'm not very familiar with this code, so my questions might not make sense.
> >
> > But take a look at the following two code segments in ssl_io_filter_coalesce():
> >
> > for (e = APR_BRIGADE_FIRST(bb);
> > e != APR_BRIGADE_SENTINEL(bb)
> > && !APR_BUCKET_IS_METADATA(e)
> > && e->length != (apr_size_t)-1
> > && e->length < COALESCE_BYTES
> > && (buffered + bytes + e->length) < COALESCE_BYTES;
> >
> > and
> >
> > if (rv == APR_SUCCESS) {
> > /* If the read above made the bucket morph, it may now fit
> > * entirely within the buffer. Otherwise, split it so it does
> > * fit. */
> > if (e->length >= COALESCE_BYTES
> > || e->length + buffered + bytes >= COALESCE_BYTES) {
> > rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
> > }
> >
> > Does this mean that either buffered or bytes might be negative?
> > Otherwise, testing both "e->length" and "e->length + buffered + bytes" against COALESCE_BYTES seems redundant.
>
> I guess they are some sort redundant. Joe?
I was being paranoid about unsigned integer overflow when writing this.
(e->length + X) may overflow, comparing e->length against COALESCE_BYTES
in both cases prevents that since max(bytes + buffered)=COALESCE_BYTES.
There is nothing preventing someone creating a bucket (e.g. FILE) with
e->length = (apr_size_t)-2 and passing it through the filter stack
AFAIK, so I think paranoia is reasonable.
But now you made me read the code *again* I think those tests should be
e->length <= COALESCE_BYTES and e->length > COALESCE_BYTES etc.
And I thought this would be a simple patch... Very much appreciate all
the review!
Regards, Joe
Re: svn commit: r1875881 -
/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 4/1/20 11:44 PM, Mike Rumph wrote:
> I'm not very familiar with this code, so my questions might not make sense.
>
> But take a look at the following two code segments in ssl_io_filter_coalesce():
>
> for (e = APR_BRIGADE_FIRST(bb);
> e != APR_BRIGADE_SENTINEL(bb)
> && !APR_BUCKET_IS_METADATA(e)
> && e->length != (apr_size_t)-1
> && e->length < COALESCE_BYTES
> && (buffered + bytes + e->length) < COALESCE_BYTES;
>
> and
>
> if (rv == APR_SUCCESS) {
> /* If the read above made the bucket morph, it may now fit
> * entirely within the buffer. Otherwise, split it so it does
> * fit. */
> if (e->length >= COALESCE_BYTES
> || e->length + buffered + bytes >= COALESCE_BYTES) {
> rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
> }
>
> Does this mean that either buffered or bytes might be negative?
> Otherwise, testing both "e->length" and "e->length + buffered + bytes" against COALESCE_BYTES seems redundant.
I guess they are some sort redundant. Joe?
Regards
Rüdiger
Re: svn commit: r1875881 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
Posted by Mike Rumph <mr...@gmail.com>.
I'm not very familiar with this code, so my questions might not make sense.
But take a look at the following two code segments in
ssl_io_filter_coalesce():
for (e = APR_BRIGADE_FIRST(bb);
e != APR_BRIGADE_SENTINEL(bb)
&& !APR_BUCKET_IS_METADATA(e)
&& e->length != (apr_size_t)-1
&& e->length < COALESCE_BYTES
&& (buffered + bytes + e->length) < COALESCE_BYTES;
and
if (rv == APR_SUCCESS) {
/* If the read above made the bucket morph, it may now fit
* entirely within the buffer. Otherwise, split it so it does
* fit. */
if (e->length >= COALESCE_BYTES
|| e->length + buffered + bytes >= COALESCE_BYTES) {
rv = apr_bucket_split(e, COALESCE_BYTES - (buffered +
bytes));
}
Does this mean that either buffered or bytes might be negative?
Otherwise, testing both "e->length" and "e->length + buffered + bytes"
against COALESCE_BYTES seems redundant.
Just one of these tests should be sufficient.
Also, is it ever possible for "COALESCE_BYTES - (buffered + bytes)" to be
negative?
If so, can the apr_bucket_split() function handle this properly?
Thanks,
Mike
On Wed, Apr 1, 2020 at 12:32 PM Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 4/1/20 2:38 PM, Joe Orton wrote:
> > On Wed, Apr 01, 2020 at 02:04:21PM +0200, Ruediger Pluem wrote:
> >> On 3/30/20 3:18 PM, jorton@apache.org wrote:
> >>>
> >>> - rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
> >>> + /* If the read above made the bucket morph, it may now fit
> >>> + * entirely within the buffer. Otherwise, split it so it does
> >>> + * fit. */
> >>> + if (e->length < COALESCE_BYTES
> >>> + && e->length + buffered + bytes < COALESCE_BYTES) {
> >>> + rv = APR_SUCCESS;
> >>
> >> Hmm. If we had e->length == -1 above and the bucket read failed, e
> might still be the morphing bucket and hence e->length == -1.
> >> I think all the code below assumes e->length >= 0 things can get off
> the rails.
> >
> > Thanks a lot for the review... I tried to keep that as simple as
> > possible but there are too many cases to cover. Yep, you're right.
> >
> >> How about the following patch (minus whitespace changes) to fix this:
> >
> > +1 that looks correct to me, please commit (or I can...)
>
> Committed as r1876014.
>
> Regards
>
> Rüdiger
>
Re: svn commit: r1875881 -
/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 4/1/20 2:38 PM, Joe Orton wrote:
> On Wed, Apr 01, 2020 at 02:04:21PM +0200, Ruediger Pluem wrote:
>> On 3/30/20 3:18 PM, jorton@apache.org wrote:
>>>
>>> - rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
>>> + /* If the read above made the bucket morph, it may now fit
>>> + * entirely within the buffer. Otherwise, split it so it does
>>> + * fit. */
>>> + if (e->length < COALESCE_BYTES
>>> + && e->length + buffered + bytes < COALESCE_BYTES) {
>>> + rv = APR_SUCCESS;
>>
>> Hmm. If we had e->length == -1 above and the bucket read failed, e might still be the morphing bucket and hence e->length == -1.
>> I think all the code below assumes e->length >= 0 things can get off the rails.
>
> Thanks a lot for the review... I tried to keep that as simple as
> possible but there are too many cases to cover. Yep, you're right.
>
>> How about the following patch (minus whitespace changes) to fix this:
>
> +1 that looks correct to me, please commit (or I can...)
Committed as r1876014.
Regards
Rüdiger
Re: svn commit: r1875881 -
/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Apr 01, 2020 at 02:04:21PM +0200, Ruediger Pluem wrote:
> On 3/30/20 3:18 PM, jorton@apache.org wrote:
> >
> > - rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
> > + /* If the read above made the bucket morph, it may now fit
> > + * entirely within the buffer. Otherwise, split it so it does
> > + * fit. */
> > + if (e->length < COALESCE_BYTES
> > + && e->length + buffered + bytes < COALESCE_BYTES) {
> > + rv = APR_SUCCESS;
>
> Hmm. If we had e->length == -1 above and the bucket read failed, e might still be the morphing bucket and hence e->length == -1.
> I think all the code below assumes e->length >= 0 things can get off the rails.
Thanks a lot for the review... I tried to keep that as simple as
possible but there are too many cases to cover. Yep, you're right.
> How about the following patch (minus whitespace changes) to fix this:
+1 that looks correct to me, please commit (or I can...)
> Index: ssl_engine_io.c
> ===================================================================
> --- ssl_engine_io.c (revision 1875997)
> +++ ssl_engine_io.c (working copy)
> @@ -1739,7 +1739,7 @@
> && bytes + buffered < COALESCE_BYTES
> && e != APR_BRIGADE_SENTINEL(bb)
> && !APR_BUCKET_IS_METADATA(e)) {
> - apr_status_t rv;
> + apr_status_t rv = APR_SUCCESS;
>
> /* For an indeterminate length bucket (PIPE/CGI/...), try a
> * non-blocking read to have it morph into a HEAP. If the
> @@ -1753,17 +1753,16 @@
> if (rv != APR_SUCCESS && !APR_STATUS_IS_EAGAIN(rv)) {
> ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, f->c, APLOGNO(10232)
> "coalesce failed to read from data bucket");
> + return AP_FILTER_ERROR;
> }
> }
>
> + if (rv == APR_SUCCESS) {
> /* If the read above made the bucket morph, it may now fit
> * entirely within the buffer. Otherwise, split it so it does
> * fit. */
> - if (e->length < COALESCE_BYTES
> - && e->length + buffered + bytes < COALESCE_BYTES) {
> - rv = APR_SUCCESS;
> - }
> - else {
> + if (e->length >= COALESCE_BYTES
> + || e->length + buffered + bytes >= COALESCE_BYTES) {
> rv = apr_bucket_split(e, COALESCE_BYTES - (buffered + bytes));
> }
>
> @@ -1787,6 +1786,7 @@
> return AP_FILTER_ERROR;
> }
> }
> + }
>
> upto = e;
>
>
> Regards
>
> Rüdiger
>