httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Kalu┼ża <jkal...@redhat.com>
Subject Re: svn commit: r1622450 - /httpd/httpd/trunk/support/ab.c
Date Mon, 03 Nov 2014 11:20:24 GMT
On 11/02/2014 05:09 PM, Yann Ylavic wrote:
> Hi,
>
> On Thu, Sep 4, 2014 at 12:52 PM,  <jkaluza@apache.org> wrote:
>> Author: jkaluza
>> Date: Thu Sep  4 10:52:24 2014
>> New Revision: 1622450
>>
>> URL: http://svn.apache.org/r1622450
>> Log:
>> ab: increase request and response header size to 8192 bytes,
>> fix potential buffer-overflow in Server: header handling.
>>
>> Modified:
>>      httpd/httpd/trunk/support/ab.c
>>
>> Modified: httpd/httpd/trunk/support/ab.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/support/ab.c?rev=1622450&r1=1622449&r2=1622450&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/support/ab.c (original)
>> +++ httpd/httpd/trunk/support/ab.c Thu Sep  4 10:52:24 2014
> [snip]
>> @@ -1516,12 +1516,14 @@ static void read_connection(struct conne
>>                    * this is first time, extract some interesting info
>>                    */
>>                   char *p, *q;
>> +                size_t len = 0;
>>                   p = strstr(c->cbuff, "Server:");
>>                   q = servername;
>>                   if (p) {
>>                       p += 8;
>> -                    while (*p > 32)
>> -                    *q++ = *p++;
>> +                    /* -1 to not overwrite last '\0' byte */
>> +                    while (*p > 32 && len++ < sizeof(servername) -
1)
>
> Maybe ++len above (instead of len++) since we need to leave room for
> the final '\0' below?
> Otherwise we may still overflow when writing it to
> servername[sizeof(servername)]...

I think technically that code is OK. It writes "sizeof(servername) - 1" 
characters to servername and keeps the last byte for zero. It could be 
rewritten as "++len < sizeof(servername)", but the result is the same 
and since gcc optimizes that, it even generates the same code.

Just to be really sure, I wrote following test code:

#include <stdio.h>
#include <stdlib.h>

#define BUFF_SIZE 10

int main(int argc, char **argv) {
	char *servername = malloc(BUFF_SIZE);
	char original[] = "Something_longer_than_10_bytes";
	char *p = original, *q = servername;
	size_t len = 0;
	while (*p > 32 && len++ < BUFF_SIZE - 1)
		*q++ = *p++;
	*q = 0;
	printf("'%s'\n", servername);
	return 0;
}

Running that in valgrind looks OK too.

$ gcc test.c
$ valgrind -q ./a.out
'Something'
$

Am I missing something?

Regards,
Jan Kaluza

>> +                        *q++ = *p++;
>>                   }
>>                   *q = 0;
>>               }
>>
>
> Regards,
> Yann.
>


Mime
View raw message