possibly memory leak?

Discussion in 'C Programming' started by auto.ptr, Nov 15, 2005.

  1. auto.ptr

    auto.ptr Guest

    option[0].....option[312]......
    |--> p?
     
    auto.ptr, Nov 15, 2005
    #1
    1. Advertising

  2. auto.ptr

    auto.ptr Guest

    option[0].....option[312]......
    |--> p?
     
    auto.ptr, Nov 15, 2005
    #2
    1. Advertising

  3. auto.ptr

    auto.ptr Guest

    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) .
    --------------------------------------------
    }while( p < end ) trying...
     
    auto.ptr, Nov 15, 2005
    #3
  4. auto.ptr

    auto.ptr Guest

    } while ( p != end );

    >> ------- }while( p < end ); try to this ?
     
    auto.ptr, Nov 15, 2005
    #4
  5. auto.ptr

    Anand Guest

    Roman Mashak wrote:
    > 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.
     
    Anand, Nov 15, 2005
    #5
  6. auto.ptr

    Marc Boyer Guest

    Le 15-11-2005, Roman Mashak <> a écrit :
    > 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
     
    Marc Boyer, Nov 15, 2005
    #6
  7. auto.ptr

    Roman Mashak Guest

    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:
     
    Roman Mashak, Nov 15, 2005
    #7
  8. auto.ptr

    Roman Mashak Guest

    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:
     
    Roman Mashak, Nov 15, 2005
    #8
  9. auto.ptr

    Old Wolf Guest

    Roman Mashak wrote:

    > 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.
     
    Old Wolf, Nov 15, 2005
    #9
  10. auto.ptr

    Thad Smith Guest

    Roman Mashak wrote:

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

    --
    Thad
     
    Thad Smith, Nov 16, 2005
    #10
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. s.subbarayan

    Dynamic memory allocation and memory leak...

    s.subbarayan, Mar 18, 2005, in forum: C Programming
    Replies:
    10
    Views:
    764
    Eric Sosman
    Mar 22, 2005
  2. Richard Heathfield

    Leak or no leak ??

    Richard Heathfield, Jul 10, 2006, in forum: C Programming
    Replies:
    4
    Views:
    381
    Richard Heathfield
    Jul 10, 2006
  3. hamishd
    Replies:
    5
    Views:
    347
    Dan Bloomquist
    Nov 28, 2006
  4. cham
    Replies:
    5
    Views:
    794
  5. Mark Probert
    Replies:
    4
    Views:
    357
    Mark Probert
    Feb 9, 2005
Loading...

Share This Page