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 Thu, 13 Jan 2011 14:25:22 GMT
 

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

Mime
View raw message