possibly memory leak?

[auto.ptr]

option[0].....option[312]......
|--> p?

 

[auto.ptr]

option[0].....option[312]......
|--> p?

 

[auto.ptr]

struct dhcp_hdr {
/* ... various fields here*/
unsigned char options[312];
}
p = dhcph->options + 4;
*(++p),p--,(*p)?(*p,p+=2,p+=len)p=p+2+len) .

 

[auto.ptr]

} while ( p != end );

 

[Anand]

Hello, All!

In this piece of code I get 'segmentation fault' periodically: [...]
do {
len = *(++p); /* save option length */
p--; len = *p;
if ( *p == 190 || *p == 191 ) {
opt = *p;
p += 2;

assert( (p+len) <= end); // include <assert.h>, you could also
// do a check
if ((p+len) <= end)
{

strncpy(tmp, (char *)p, len);
printf("option %u found, value is %s\n", opt, tmp);

p += len;

}
else
{
printf("Not enough buffer\n");
break;
}

}
else
p = (p + 2) + len; /* move pointer to the next tag */
} while ( p != end );

------------^
(p < end)
[...]

1) Bounds check p != end check may be wrong and should be changed to
(p < end)
Unless you're always guaranteed that p stops at end, not just passes
by it. (I am always paranoid and would prefer the first one.)

2) Do the bounds check just after the pointer modification, or
just before the access.. (I always do the first one)
p += ...; // after p+=2, p+= len, p += (2+len);
if (p >= end) break; // do this check.

 

[Marc Boyer]

struct dhcp_hdr {
/* ... various fields here*/
unsigned char options[312];
}
...
struct dhcp_hdr *dhcph;
unsigned char *p, *end, opt;
char *tmp = NULL;
unsigned int len=0;
...
p = dhcph->options + 4;
end = dhcph->options + sizeof(dhcph->options);

tmp = malloc(100);

Why ? You are sure that this is sufficient ?

do {
len = *(++p); /* save option length */
p--;

Why not
len=p[1]
or
len=*(p+1)

if ( *p == 190 || *p == 191 ) {

Some magic number I assume.

opt = *p;
p += 2;
strncpy(tmp, (char *)p, len);

You are sure that the option is nul-terminated and that
strlen(p)<len ?

printf("option %u found, value is %s\n", opt, tmp);

p += len;
}
else
p = (p + 2) + len; /* move pointer to the next tag */
} while ( p != end );
...

Seems like 'p' points to illegal chunk of memory at some time and I'm unable
to find this properly. Where can it be?

Its impossible to known without any information on the value of
the unsigned chars in the dhcph->options array. Perhaps could you
add some assertions in your code, like
assert(p < end );
assert(strlen(p)<len);
assert(len<100);
etc...

Marc Boyer

 

[Roman Mashak]

Hello, All!

In this piece of code I get 'segmentation fault' periodically:

....
struct dhcp_hdr {
/* ... various fields here*/
unsigned char options[312];
}
....
struct dhcp_hdr *dhcph;
unsigned char *p, *end, opt;
char *tmp = NULL;
unsigned int len=0;
....
p = dhcph->options + 4;
end = dhcph->options + sizeof(dhcph->options);

tmp = malloc(100);

do {
len = *(++p); /* save option length */
p--;
if ( *p == 190 || *p == 191 ) {
opt = *p;
p += 2;
strncpy(tmp, (char *)p, len);
printf("option %u found, value is %s\n", opt, tmp);

p += len;
}
else
p = (p + 2) + len; /* move pointer to the next tag */
} while ( p != end );
....

Seems like 'p' points to illegal chunk of memory at some time and I'm unable
to find this properly. Where can it be?

With best regards, Roman Mashak. E-mail: (e-mail address removed)

 

[Roman Mashak]

Hello, Marc!
You wrote on Tue, 15 Nov 2005 08:47:50 +0000 (UTC):

[skip]
??>> ...
??>>
??>> Seems like 'p' points to illegal chunk of memory at some time and I'm
??>> unable to find this properly. Where can it be?

MB> Its impossible to known without any information on the value of
MB> the unsigned chars in the dhcph->options array. Perhaps could you
MB> add some assertions in your code, like
MB> assert(p < end );
MB> assert(strlen(p)<len);
MB> assert(len<100);
MB> etc...
Thanks to everybody for replies. The problem was with the bound checking in
'while' loop:

while (p != end)

changed into

while ( p < end )

ans also a few clumsy spots were fixed.

With best regards, Roman Mashak. E-mail: (e-mail address removed)

 

[Old Wolf]

Hello, All!

In this piece of code I get 'segmentation fault' periodically:

Your code is not very robust when faced with malicious data.

struct dhcp_hdr {
/* ... various fields here*/
unsigned char options[312];
}
struct dhcp_hdr *dhcph;
unsigned char *p, *end, opt;
char *tmp = NULL;
unsigned int len=0;
...
p = dhcph->options + 4;
end = dhcph->options + sizeof(dhcph->options);

tmp = malloc(100);

do {
len = *(++p);
p--;

Please replace those two lines with:
len = p[1];

if ( *p == 190 || *p == 191 ) {
opt = *p;
p += 2;
strncpy(tmp, (char *)p, len);

If (len > 100) then you cause a buffer overflow. Check this first.
If (end - p) < len, and there isn't a 0-terminator in there,
then you read past the end of the buffer. Fix this.

strncpy is generally a poor function to use, because once you
have added enough code to stop overflows, it would have been
easier to just do a memcpy or some other copy function.

printf("option %u found, value is %s\n", opt, tmp);

p += len;
}
else
p = (p + 2) + len; /* move pointer to the next tag */

Again, check that you aren't pointing p past the end here.
If you correctly checked it earlier, then it wouldn't be
necessary to re-check. I suggest that when you read 'len',
you check that it's < 100 and that 2 + len < (end - p), and
if not, assume the rest of the input is corrupt.

} while ( p != end );

You commented that changing this to (p < end) fixed the
problem. This is the ambulance at the bottom of the cliff.
You would still be pointing past the end of your buffer,
which is a no-no. It's essential you fix the underlying
cause of the overflow.

 

[Thad Smith]

In this piece of code I get 'segmentation fault' periodically:

...
struct dhcp_hdr {
/* ... various fields here*/
unsigned char options[312];
}
...
struct dhcp_hdr *dhcph;
unsigned char *p, *end, opt;
char *tmp = NULL;
unsigned int len=0;
...
p = dhcph->options + 4;
end = dhcph->options + sizeof(dhcph->options);

At this point dhcph is either a null pointer or undefined and the
results of assigning p and end are undefined. You need to assign a
value to dhcph.