httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Plüm, Rüdiger, VF-Group" <ruediger.pl...@vodafone.com>
Subject RE: svn commit: r1051468 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_engine_io.c
Date Wed, 12 Jan 2011 14:29:45 GMT
 

> -----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

Mime
View raw message