You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rp...@apache.org on 2010/12/21 12:43:42 UTC
svn commit: r1051468 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_io.c
Author: rpluem
Date: Tue Dec 21 11:43:42 2010
New Revision: 1051468
URL: http://svn.apache.org/viewvc?rev=1051468&view=rev
Log:
* Do not drop contents of incomplete lines, but safe them for the next
round of reading.
PR: 50481
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1051468&r1=1051467&r2=1051468&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Tue Dec 21 11:43:42 2010
@@ -2,6 +2,9 @@
Changes with Apache 2.3.11
+ *) mod_ssl: Correctly read full lines in input filter when the line is
+ incomplete during first read. PR 50481. [Ruediger Pluem]
+
*) mod_authz_core: Add AuthzSendForbiddenOnFailure directive to allow
sending '403 FORBIDDEN' instead of '401 UNAUTHORIZED' if authorization
fails for an authenticated user. PR 40721. [Stefan Fritsch]
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=1051468&r1=1051467&r2=1051468&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Tue Dec 21 11:43:42 2010
@@ -756,6 +756,10 @@ static apr_status_t ssl_io_input_getline
status = ssl_io_input_read(inctx, buf + offset, &tmplen);
if (status != APR_SUCCESS) {
+ if (APR_STATUS_IS_EAGAIN(status) && (*len > 0)) {
+ /* Safe the part of the line we already got */
+ char_buffer_write(&inctx->cbuf, buf, *len);
+ }
return status;
}
@@ -782,6 +786,10 @@ static apr_status_t ssl_io_input_getline
*len = bytes;
}
+ else {
+ /* Safe the part of the line we already got */
+ char_buffer_write(&inctx->cbuf, buf, *len);
+ }
return APR_SUCCESS;
}
RE: svn commit: r1051468 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_engine_io.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Joe Orton
> Sent: Freitag, 14. Januar 2011 15:42
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1051468 - in /httpd/httpd/trunk:
> CHANGES modules/ssl/ssl_engine_io.c
>
> On Thu, Jan 13, 2011 at 03:25:22PM +0100, "Plüm, Rüdiger,
> VF-Group" wrote:
> > Should I commit the patch below now to resolve the issue and address
> > your point?
>
> Once again we are struggling with the ill-defined filtering API :(
>
> You're proposing here to make the _GETLINE call return a
> partial read in
> a case where no LF is found. I think most existing callers would
> probably cope fine with that, but the wording in util_filter.h is
> explicit:
>
> * (If a potential line is too long or no CRLF is found,
> the
> * filter may return partial data).
>
>
> i.e. it does not make mention of non-blocking mode.
>
> So I think it would be best to revert the second half of the
> patch and
> leave the first half as-is, i.e. patch as below - if this
> makes sense to
> you also?
Makes sense. Committed as r1059037.
>
> I've been trying to make a test case which tickles this issue
> but have
> so far not managed to get a non-blocking GETLINE call with
> the filter in
> the right state.
Have a look at the test case provided in the PR50481. This might be helpful.
Regards
Rüdiger
Re: svn commit: r1051468 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_io.c
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Jan 13, 2011 at 03:25:22PM +0100, "Plüm, Rüdiger, VF-Group" wrote:
> Should I commit the patch below now to resolve the issue and address
> your point?
Once again we are struggling with the ill-defined filtering API :(
You're proposing here to make the _GETLINE call return a partial read in
a case where no LF is found. I think most existing callers would
probably cope fine with that, but the wording in util_filter.h is
explicit:
* (If a potential line is too long or no CRLF is found, the
* filter may return partial data).
i.e. it does not make mention of non-blocking mode.
So I think it would be best to revert the second half of the patch and
leave the first half as-is, i.e. patch as below - if this makes sense to
you also?
I've been trying to make a test case which tickles this issue but have
so far not managed to get a non-blocking GETLINE call with the filter in
the right state.
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1054976)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -786,10 +786,6 @@
*len = bytes;
}
- else {
- /* Safe the part of the line we already got */
- char_buffer_write(&inctx->cbuf, buf, *len);
- }
return APR_SUCCESS;
}
RE: svn commit: r1051468 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_engine_io.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: "Plüm, Rüdiger, VF-Group"
> Sent: Mittwoch, 12. Januar 2011 16:05
> To: dev@httpd.apache.org
> Subject: RE: svn commit: r1051468 - in /httpd/httpd/trunk:
> CHANGES modules/ssl/ssl_engine_io.c
>
>
>
> > -----Original Message-----
> > From: Joe Orton
> > Sent: Mittwoch, 12. Januar 2011 15:51
> > To: dev@httpd.apache.org
> > Subject: Re: svn commit: r1051468 - in /httpd/httpd/trunk:
> > CHANGES modules/ssl/ssl_engine_io.c
> >
> > On Wed, Jan 12, 2011 at 03:29:45PM +0100, "Plüm, Rüdiger,
> > VF-Group" wrote:
> > >
> > > I guess the second call to char_buffer_write can happen in
> > the situation
> > > you describe above, but IMHO it can also happen if we have
> > a non blocking read
> > > and we just did not get enough data.
> >
> > How? If no LF is ever found the loop continues until the
> > buffer is full
> > with tmplen == 0. In the case of read failure/EAGAIN, the
> > code returns
> > from the function early and never falls out of the loop.
> >
> > (This is all non-obvious because of the structure of the
> > code; it might
> > read more clearly if the if (pos) bit was moved inside the
> > loop before
> > the break.)
>
> Ok. I guess you are correct. I thought about a situation similar
> to ap_get_brigade where a non blocking read can return APR_SUCCESS
> and an empty brigade but ssl_io_input_read returning APR_SUCCESS
> and tmplen == 0 would lead to an endless loop in the while loop.
> Besides this leading to another bug of a spinning httpd process
> my code would never be triggered in this case.
> So what is the way forward now?
> My second propsal to return just what we have?
Should I commit the patch below now to resolve the issue and address
your point?
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1058133)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -757,8 +757,7 @@
if (status != APR_SUCCESS) {
if (APR_STATUS_IS_EAGAIN(status) && (*len > 0)) {
- /* Save the part of the line we already got */
- char_buffer_write(&inctx->cbuf, buf, *len);
+ return APR_SUCCESS;
}
return status;
}
@@ -786,10 +785,6 @@
*len = bytes;
}
- else {
- /* Save the part of the line we already got */
- char_buffer_write(&inctx->cbuf, buf, *len);
- }
return APR_SUCCESS;
}
which would result in the following complete patch (r105148, r1058133, Patch above):
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1051467)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -756,6 +756,9 @@
status = ssl_io_input_read(inctx, buf + offset, &tmplen);
if (status != APR_SUCCESS) {
+ if (APR_STATUS_IS_EAGAIN(status) && (*len > 0)) {
+ return APR_SUCCESS;
+ }
return status;
}
Regards
Rüdiger
RE: svn commit: r1051468 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_engine_io.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Joe Orton
> Sent: Mittwoch, 12. Januar 2011 15:51
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1051468 - in /httpd/httpd/trunk:
> CHANGES modules/ssl/ssl_engine_io.c
>
> On Wed, Jan 12, 2011 at 03:29:45PM +0100, "Plüm, Rüdiger,
> VF-Group" wrote:
> > > In that case the correct behaviour of the input filter is
> to return a
> > > partial read with APR_SUCCESS (per AP_MODE_GETLINE semantics
> > > described
> > > in util_filter.h). So the data must *not* also be
> buffered in that
> > > case, and the behaviour was correct before.
> >
> > I guess the second call to char_buffer_write can happen in
> the situation
> > you describe above, but IMHO it can also happen if we have
> a non blocking read
> > and we just did not get enough data.
>
> How? If no LF is ever found the loop continues until the
> buffer is full
> with tmplen == 0. In the case of read failure/EAGAIN, the
> code returns
> from the function early and never falls out of the loop.
>
> (This is all non-obvious because of the structure of the
> code; it might
> read more clearly if the if (pos) bit was moved inside the
> loop before
> the break.)
Ok. I guess you are correct. I thought about a situation similar
to ap_get_brigade where a non blocking read can return APR_SUCCESS
and an empty brigade but ssl_io_input_read returning APR_SUCCESS
and tmplen == 0 would lead to an endless loop in the while loop.
Besides this leading to another bug of a spinning httpd process
my code would never be triggered in this case.
So what is the way forward now?
My second propsal to return just what we have?
Regards
Rüdiger
Re: svn commit: r1051468 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_io.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jan 12, 2011 at 03:29:45PM +0100, "Plüm, Rüdiger, VF-Group" wrote:
> > In that case the correct behaviour of the input filter is to return a
> > partial read with APR_SUCCESS (per AP_MODE_GETLINE semantics
> > described
> > in util_filter.h). So the data must *not* also be buffered in that
> > case, and the behaviour was correct before.
>
> I guess the second call to char_buffer_write can happen in the situation
> you describe above, but IMHO it can also happen if we have a non blocking read
> and we just did not get enough data.
How? If no LF is ever found the loop continues until the buffer is full
with tmplen == 0. In the case of read failure/EAGAIN, the code returns
from the function early and never falls out of the loop.
(This is all non-obvious because of the structure of the code; it might
read more clearly if the if (pos) bit was moved inside the loop before
the break.)
Regards, Joe
RE: svn commit: r1051468 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_engine_io.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Joe Orton
> Sent: Mittwoch, 12. Januar 2011 14:27
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1051468 - in /httpd/httpd/trunk:
> CHANGES modules/ssl/ssl_engine_io.c
>
> On Tue, Dec 21, 2010 at 11:43:42AM -0000, rpluem@apache.org wrote:
> > URL: http://svn.apache.org/viewvc?rev=1051468&view=rev
> > Log:
> > * Do not drop contents of incomplete lines, but safe them
> for the next
> > round of reading.
> >
> > PR: 50481
> ...
> > --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Tue Dec
> 21 11:43:42 2010
> > @@ -756,6 +756,10 @@ static apr_status_t ssl_io_input_getline
> > status = ssl_io_input_read(inctx, buf + offset, &tmplen);
> >
> > if (status != APR_SUCCESS) {
> > + if (APR_STATUS_IS_EAGAIN(status) && (*len > 0)) {
> > + /* Safe the part of the line we already got */
> > + char_buffer_write(&inctx->cbuf, buf, *len);
> > + }
> > return status;
> > }
> >
> > @@ -782,6 +786,10 @@ static apr_status_t ssl_io_input_getline
> >
> > *len = bytes;
> > }
> > + else {
> > + /* Safe the part of the line we already got */
> > + char_buffer_write(&inctx->cbuf, buf, *len);
> > + }
>
> Just saw the backport proposal... apologies if I am not understanding
> this correctly:
>
> The only case affected by the second chunk of this change is
> where the
> buffer is full when trying to read a line - i.e. line longer than
> buffer. Right?
No. The issue occurs if we do a non blocking read and no full line is available
for reading (first call to char_buffer_write). In this case the already read data
was not returned but silently dropped.
>
> In that case the correct behaviour of the input filter is to return a
> partial read with APR_SUCCESS (per AP_MODE_GETLINE semantics
> described
> in util_filter.h). So the data must *not* also be buffered in that
> case, and the behaviour was correct before.
I guess the second call to char_buffer_write can happen in the situation
you describe above, but IMHO it can also happen if we have a non blocking read
and we just did not get enough data. I guess the following patch can resolve this:
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1058133)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -787,8 +787,16 @@
*len = bytes;
}
else {
- /* Save the part of the line we already got */
- char_buffer_write(&inctx->cbuf, buf, *len);
+ /*
+ * Check if the buffer is not full but we found no LF.
+ * This indicates that no more data was available during a non
+ * blocking read.
+ */
+ if (offset < buflen) {
+ /* Save the part of the line we already got */
+ char_buffer_write(&inctx->cbuf, buf, *len);
+ return APR_EAGAIN;
+ }
}
return APR_SUCCESS;
OTOH another possible fix for the original problem should be to return just what we have:
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1058133)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -757,8 +757,7 @@
if (status != APR_SUCCESS) {
if (APR_STATUS_IS_EAGAIN(status) && (*len > 0)) {
- /* Save the part of the line we already got */
- char_buffer_write(&inctx->cbuf, buf, *len);
+ return APR_SUCCESS;
}
return status;
}
@@ -786,10 +785,6 @@
*len = bytes;
}
- else {
- /* Save the part of the line we already got */
- char_buffer_write(&inctx->cbuf, buf, *len);
- }
return APR_SUCCESS;
}
>
> Was that part of the patch necessary to fix to the observed problem?
> The first part looks fine though s/safe/save or s/safe/buffer
> if I may
> nitpick the language ;)
Language part fixed as r1058133. I seem to confuse safe and save
more often in recent times :-)
Regards
Rüdiger
Re: svn commit: r1051468 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_io.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Dec 21, 2010 at 11:43:42AM -0000, rpluem@apache.org wrote:
> URL: http://svn.apache.org/viewvc?rev=1051468&view=rev
> Log:
> * Do not drop contents of incomplete lines, but safe them for the next
> round of reading.
>
> PR: 50481
...
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Tue Dec 21 11:43:42 2010
> @@ -756,6 +756,10 @@ static apr_status_t ssl_io_input_getline
> status = ssl_io_input_read(inctx, buf + offset, &tmplen);
>
> if (status != APR_SUCCESS) {
> + if (APR_STATUS_IS_EAGAIN(status) && (*len > 0)) {
> + /* Safe the part of the line we already got */
> + char_buffer_write(&inctx->cbuf, buf, *len);
> + }
> return status;
> }
>
> @@ -782,6 +786,10 @@ static apr_status_t ssl_io_input_getline
>
> *len = bytes;
> }
> + else {
> + /* Safe the part of the line we already got */
> + char_buffer_write(&inctx->cbuf, buf, *len);
> + }
Just saw the backport proposal... apologies if I am not understanding
this correctly:
The only case affected by the second chunk of this change is where the
buffer is full when trying to read a line - i.e. line longer than
buffer. Right?
In that case the correct behaviour of the input filter is to return a
partial read with APR_SUCCESS (per AP_MODE_GETLINE semantics described
in util_filter.h). So the data must *not* also be buffered in that
case, and the behaviour was correct before.
Was that part of the patch necessary to fix to the observed problem?
The first part looks fine though s/safe/save or s/safe/buffer if I may
nitpick the language ;)
Regards, Joe