Re: svn commit: r1864868 - /httpd/httpd/trunk/server/core_filters.c

[Joe Orton]

On Sat, Aug 10, 2019 at 11:58:44AM +0200, Marion & Christophe JAILLET wrote:
> Hi all,
> 
> I would appreciate some other eyes on the patch below.
> I guess that that the fix is correct, but I don't know the possible
> implication of the fix.
> 
> As said in the commit description, -1 seems to be a valid length, but I
> don't know if such buckets can happen here.

(apr_size_t)-1 is the right way to compare here.  I don't know of any 
case where indeterminate-length buckets will be present in input 
filtering, so I'd expect we never hit the -1 case in practice.  But the 
change is right and it's a good find!

Re: svn commit: r1864868 - /httpd/httpd/trunk/server/core_filters.c

[Marion & Christophe JAILLET]

Hi all,

I would appreciate some other eyes on the patch below.
I guess that that the fix is correct, but I don't know the possible 
implication of the fix.


As said in the commit description, -1 seems to be a valid length, but I 
don't know if such buckets can happen here.


So this patch can change the behavior of the code and trigger a path 
that was never executed before.

Comments from s.o. with deeper understanding of filters are welcomed.


Should the commit description be tweaked, please do so.

CJ


Le 10/08/2019 à 11:52, jaillet...@apache.org a écrit :

Author: jailletc36
Date: Sat Aug 10 09:52:34 2019
New Revision: 1864868

URL: http://svn.apache.org/viewvc?rev=1864868=rev
Log:
Fix a signed/unsigned comparison that can never match.

-1 is a valid length value (for socket, pipe and cgi buckets for example)
All path I've checked cast the -1 to (apr_size_t) in order for the comparison 
to work. So do it as well here.

This has been like that in trunk since r708144, about 11 years ago, so I assume 
that it is not really an issue.

Spotted by gcc 9.1 and -Wextra

Modified:
 httpd/httpd/trunk/server/core_filters.c

Modified: httpd/httpd/trunk/server/core_filters.c
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core_filters.c?rev=1864868=1864867=1864868=diff
==
--- httpd/httpd/trunk/server/core_filters.c (original)
+++ httpd/httpd/trunk/server/core_filters.c Sat Aug 10 09:52:34 2019
@@ -277,7 +277,7 @@ apr_status_t ap_core_input_filter(ap_fil
  while ((len < readbytes) && (rv == APR_SUCCESS)
 && (e != APR_BRIGADE_SENTINEL(ctx->bb))) {
  /* Check for the availability of buckets with known length */
-if (e->length != -1) {
+if (e->length != (apr_size_t)-1) {
  len += e->length;
  e = APR_BUCKET_NEXT(e);
  }