You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/11/02 17:09:26 UTC
Re: svn commit: r1622450 - /httpd/httpd/trunk/support/ab.c
Hi,
On Thu, Sep 4, 2014 at 12:52 PM, <jk...@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)]...
> + *q++ = *p++;
> }
> *q = 0;
> }
>
Regards,
Yann.
Re: svn commit: r1622450 - /httpd/httpd/trunk/support/ab.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Nov 3, 2014 at 12:20 PM, Jan Kaluža <jk...@redhat.com> wrote:
> Am I missing something?
No it's me.
>
>>> q = servername;
>>> if (p) {
>>> p += 8;
>>> + /* -1 to not overwrite last '\0' byte */
>>> + while (*p > 32 && len++ < sizeof(servername) - 1)
>>> + *q++ = *p++;
>>> }
>>> *q = 0;
My (damaged-)brain has read the above as servername[len] = 0...
Sorry for the noise, got my +1 in STATUS now.
Thanks,
Yann.
Re: svn commit: r1622450 - /httpd/httpd/trunk/support/ab.c
Posted by Jan Kaluža <jk...@redhat.com>.
On 11/02/2014 05:09 PM, Yann Ylavic wrote:
> Hi,
>
> On Thu, Sep 4, 2014 at 12:52 PM, <jk...@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.
>