> -----Original Message-----
> From: Joe Orton
> Sent: Mittwoch, 12. Januar 2011 14:27
> To: [email protected]
> 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, [email protected] 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
