possible memory leak?

Discussion in 'C Programming' started by Roman Mashak, Aug 11, 2005.

  1. Roman Mashak

    Roman Mashak Guest

    Hello, All!

    I have this small piece of code, where segmentation fault happenes only upon
    runnin code. No problems during debug (JFI I'm using gdb-6.3):

    ----
    struct host_info {
    char *host;
    int port;
    char *path;
    int is_ftp;
    char *user;
    };

    ....

    int parse_url(char *url, struct host_info *h)
    {
    char *cp, *sp, *up, *pp,*ptr;

    if (strncmp(url, "ftp://", 6) == 0) {
    h->port = 21;
    h->host = url + 6;
    h->is_ftp = 1;
    } else {
    return -1;
    }

    sp = strchr(h->host, '/');
    if (sp) {
    *sp++ = '\0'; /* XXX */
    h->path = sp;

    } else
    h->path = strdup("");

    up = strrchr(h->host, '@');
    if (up != NULL) {
    h->user = h->host;
    *up++ = '\0';
    h->host = up;
    } else
    h->user = NULL;

    pp = h->host;

    cp = strchr(pp, ':');
    if (cp != NULL) {
    *cp++ = '\0';
    h->port = htons(atoi(cp));
    }

    return 0;
    }
    -----

    The problem happenes at 'XXX' mark. I also examined source code with
    'splint', which gave me some hints:

    ---
    Implicitly only storage h->path (type char *) not released before assignment
    (sp aliases h->host): h->path = sp A memory leak has been detected.
    Only-qualified storage is not released before the last reference to it is
    lost.

    Implicitly only storage h->path (type char *) not released before
    assignment: h->path = strdup("")
    ---

    I'm still confused. What can be the problem?

    Thank you.

    With best regards, Roman Mashak. E-mail:
    Roman Mashak, Aug 11, 2005
    #1
    1. Advertising

  2. Roman Mashak wrote:

    > Hello, All!
    >
    > I have this small piece of code, where segmentation fault happenes only upon
    > runnin code. No problems during debug (JFI I'm using gdb-6.3):

    ....
    > int parse_url(char *url, struct host_info *h)
    > {
    > char *cp, *sp, *up, *pp,*ptr;

    ....
    > sp = strchr(h->host, '/');
    > if (sp) {
    > *sp++ = '\0'; /* XXX */
    > h->path = sp;
    > } else
    > h->path = strdup("");

    ....

    In the branch above, you assign two different things to h->path,
    depending on sp. One is derived from a function argument and the other
    one is an allocated memory. This is begging for problems. Depending on
    your code design, a few things could happen.

    First, since I could not see you freeing the memory allocated by
    strdup(), you have a chance of memory leak. If you were to free the
    memory, then you face the danger of freeing something you had not
    allocated if sp is assigned to h->path. The three possible solutions are
    to always allocate, never allocate or remember whether you have
    allocated or not.

    Also, depending on how you call the function, your line marked with XXX
    may cause a problem if your url (the first function argument) is a
    string literal. Your XXX line modifies it, which is a clear example of
    undefined behaviour.

    You might try changing the offending bit to something like:

    h->path = strdup(h->host);
    /* Remember to check that strdup() succeeded! */
    sp = strchr(h->path, '/');
    if (sp) {
    *sp = '\0'; /* XXX */
    } else
    *h->path = '\0';

    (keeping in mind that strdup() is non-standard and hence out of the
    scope of this newsgroup). Repeat the same for h->user and h->port.
    Remember to free the memory afterwards.

    Alternatively, redesign your code so that it does not need copying the
    strings all the time. (Possibly by making only one copy and working on
    it instead of the function argument.)

    > The problem happenes at 'XXX' mark. I also examined source code with
    > 'splint', which gave me some hints:
    >
    > ---
    > Implicitly only storage h->path (type char *) not released before assignment
    > (sp aliases h->host): h->path = sp A memory leak has been detected.
    > Only-qualified storage is not released before the last reference to it is
    > lost.
    >
    > Implicitly only storage h->path (type char *) not released before
    > assignment: h->path = strdup("")
    > ---
    >
    > I'm still confused. What can be the problem?


    I hope I have managed to clear at least some of the confusion.

    Peter
    Peter Pichler, Aug 11, 2005
    #2
    1. Advertising

  3. Roman Mashak

    Guest

    I have never seen you applied memory units for your struct " *h " ,
    and you assign a constant to an variable which has no room. there will
    be a segment fault.
    , Aug 11, 2005
    #3
  4. Roman Mashak

    Guest

    wrote:
    > I have never seen you applied memory units for your struct " *h " ,
    > and you assign a constant to an variable which has no room. there will
    > be a segment fault.


    The way the argument "h" is passed to function parse_url(), indicates
    that the caller of this function should have already allocated
    memory for "struct host_info *h". Note that the type of "h" is
    "struct host_info *" and not "struct host_info **".

    Even if you allocate the memory in function parse_url(), its of no
    use because its no longer accessible after you return.
    , Aug 11, 2005
    #4
  5. Roman Mashak

    Roman Mashak Guest

    Thanks to all for replies.

    Unfortunately, I still have the problem.
    [OT]
    The curious thing is this code is taken from 'busybox' package (sure it's OT
    here) where it works fine (and I didn't change anything). I also explored
    'busybox' code and didn't find any allocation of memory, even I traced with
    debugger and it all works fine there.
    [/OT]

    What's wrong with the code like this (code in original post gets dumped at
    *sp++ = '\0' statement, so I decided to simplify a little):

    char *s = "abcde"; /* s point to 'a' character */
    *s = 'A'; /* replace 'a' with 'A' */
    s++; /* move pointer to 'b' */

    With best regards, Roman Mashak. E-mail:
    Roman Mashak, Aug 11, 2005
    #5
  6. Roman Mashak

    Suman Guest

    Roman Mashak wrote:
    > Thanks to all for replies.
    >
    > Unfortunately, I still have the problem.
    > [OT]
    > The curious thing is this code is taken from 'busybox' package (sure it's OT
    > here) where it works fine (and I didn't change anything). I also explored
    > 'busybox' code and didn't find any allocation of memory, even I traced with
    > debugger and it all works fine there.
    > [/OT]
    >
    > What's wrong with the code like this (code in original post gets dumped at
    > *sp++ = '\0' statement, so I decided to simplify a little):
    >
    > char *s = "abcde"; /* s point to 'a' character */
    > *s = 'A'; /* replace 'a' with 'A' */
    > s++; /* move pointer to 'b' */
    >
    > With best regards, Roman Mashak. E-mail:

    Does
    http://www.eskimo.com/~scs/C-faq/q16.6.html
    help?
    Suman, Aug 11, 2005
    #6
  7. Roman Mashak wrote:
    > Thanks to all for replies.
    >
    >
    > What's wrong with the code like this (code in original post gets dumped at
    > *sp++ = '\0' statement, so I decided to simplify a little):
    >
    > char *s = "abcde"; /* s point to 'a' character */
    > *s = 'A'; /* replace 'a' with 'A' */
    > s++; /* move pointer to 'b' */
    >


    http://www.eskimo.com/~scs/C-faq/q1.32.html

    You shouldn't modify string literals, such as
    "abcde". Make a copy first, or do something like this:

    char s[] = "abcde";

    -David
    David Resnick, Aug 11, 2005
    #7
  8. Roman Mashak wrote:

    > What's wrong with the code like this (code in original post gets dumped at
    > *sp++ = '\0' statement, so I decided to simplify a little):
    >
    > char *s = "abcde"; /* s point to 'a' character */
    > *s = 'A'; /* replace 'a' with 'A' */
    > s++; /* move pointer to 'b' */
    >
    > With best regards, Roman Mashak. E-mail:


    The problem here is trying to change the contents of a string literal.
    For historical reasons, a type of "abcde" is char*, but it should be
    treated as const char*. Changing the contents may "work" in some
    implementations, but it is undefined. Imagine for example that all
    string literals are stored in read-only memory. Trying to change them
    may result in ignoring the change or in a run-time error on fussier systems.

    Peter
    Peter Pichler, Aug 11, 2005
    #8
  9. Roman Mashak

    Roman Mashak Guest

    Hello, Peter!
    You wrote on Thu, 11 Aug 2005 21:53:38 +0100:

    ??>> What's wrong with the code like this (code in original post gets
    ??>> dumped at *sp++ = '\0' statement, so I decided to simplify a little):
    ??>> char *s = "abcde"; /* s point to 'a' character */
    ??>> *s = 'A'; /* replace 'a' with 'A' */
    ??>> s++; /* move pointer to 'b' */
    ??>>
    ??>> With best regards, Roman Mashak. E-mail:

    PP> The problem here is trying to change the contents of a string literal.
    PP> For historical reasons, a type of "abcde" is char*, but it should be
    PP> treated as const char*. Changing the contents may "work" in some
    PP> implementations, but it is undefined. Imagine for example that all
    PP> string literals are stored in read-only memory. Trying to change them
    PP> may result in ignoring the change or in a run-time error on fussier
    PP> systems.
    Why is it not assumed that array (char s[] = "abcd") can be also allocated
    in ROM? My compiler has option allowing to treat string literals as writable
    (but it's not safe).

    Let's consider my original peice of code (here I put full version that
    crashes):
    -----
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include <netinet/in.h>

    struct host_info {
    char *host;
    int port;
    char *path;
    int is_ftp;
    char *user;
    };

    int parse_url(char *url, struct host_info *h)
    {
    char *cp, *sp, *up, *pp;
    char *host, *path;

    if (strncmp(url, "ftp://", 6) == 0) {
    host = url + 6;
    h->port = 21;
    h->host = url + 6;
    h->is_ftp = 1;
    } else {
    return -1;
    }

    sp = strchr(h->host, '/');
    if (sp) {
    *sp++ = '\0'; /* XXX */
    h->path = sp;
    } else
    h->path = strdup("");

    up = strrchr(host, '@');
    if (up != NULL) {
    h->user = h->host;
    *up++ = '\0';
    h->host = up;
    } else
    h->user = NULL;

    pp = h->host;

    cp = strchr(pp, ':');
    if (cp != NULL) {
    *cp++ = '\0';
    h->port = htons(atoi(cp));
    }

    return 0;
    }

    int main(void)
    {
    struct host_info *hh;
    hh = (struct host_info*)malloc(sizeof(struct host_info));

    int rc = parse_url("ftp://192.168.11.197/pub/test.txt", 0);
    printf("return status rc=%d\n", rc);

    return 0;
    }
    -----

    As I stated earlier, segfault happens at XXX mark. If I'm right, then
    compiler treats that line as follows:
    1) evaluate *sp
    2) put '\0' into memory area pointed by 'sp'
    3) increment pointer value

    So, I suspect crash occurs at second step.
    [OT]
    By the way, no faults happen while debug in GDB. Basically debuggers are
    supposed to reveal such kinds of problems.
    [/OT]

    With best regards, Roman Mashak. E-mail:
    Roman Mashak, Aug 12, 2005
    #9
  10. Roman Mashak

    Jack Klein Guest

    On Fri, 12 Aug 2005 11:12:49 +0900, "Roman Mashak" <>
    wrote in comp.lang.c:

    > Hello, Peter!
    > You wrote on Thu, 11 Aug 2005 21:53:38 +0100:
    >
    > ??>> What's wrong with the code like this (code in original post gets
    > ??>> dumped at *sp++ = '\0' statement, so I decided to simplify a little):
    > ??>> char *s = "abcde"; /* s point to 'a' character */
    > ??>> *s = 'A'; /* replace 'a' with 'A' */
    > ??>> s++; /* move pointer to 'b' */
    > ??>>
    > ??>> With best regards, Roman Mashak. E-mail:
    >
    > PP> The problem here is trying to change the contents of a string literal.
    > PP> For historical reasons, a type of "abcde" is char*, but it should be
    > PP> treated as const char*. Changing the contents may "work" in some
    > PP> implementations, but it is undefined. Imagine for example that all
    > PP> string literals are stored in read-only memory. Trying to change them
    > PP> may result in ignoring the change or in a run-time error on fussier
    > PP> systems.
    > Why is it not assumed that array (char s[] = "abcd") can be also allocated
    > in ROM? My compiler has option allowing to treat string literals as writable
    > (but it's not safe).
    >
    > Let's consider my original peice of code (here I put full version that
    > crashes):
    > -----
    > #include <stdio.h>
    > #include <stdlib.h>
    > #include <string.h>
    > #include <netinet/in.h>
    >
    > struct host_info {
    > char *host;
    > int port;
    > char *path;
    > int is_ftp;
    > char *user;
    > };
    >
    > int parse_url(char *url, struct host_info *h)
    > {
    > char *cp, *sp, *up, *pp;
    > char *host, *path;
    >
    > if (strncmp(url, "ftp://", 6) == 0) {
    > host = url + 6;
    > h->port = 21;
    > h->host = url + 6;
    > h->is_ftp = 1;
    > } else {
    > return -1;
    > }
    >
    > sp = strchr(h->host, '/');
    > if (sp) {
    > *sp++ = '\0'; /* XXX */
    > h->path = sp;
    > } else
    > h->path = strdup("");
    >
    > up = strrchr(host, '@');
    > if (up != NULL) {
    > h->user = h->host;
    > *up++ = '\0';
    > h->host = up;
    > } else
    > h->user = NULL;
    >
    > pp = h->host;
    >
    > cp = strchr(pp, ':');
    > if (cp != NULL) {
    > *cp++ = '\0';
    > h->port = htons(atoi(cp));
    > }
    >
    > return 0;
    > }
    >
    > int main(void)
    > {
    > struct host_info *hh;
    > hh = (struct host_info*)malloc(sizeof(struct host_info));
    >
    > int rc = parse_url("ftp://192.168.11.197/pub/test.txt", 0);


    This is EXACTLY what three people just told you, and even posted links
    to the specific FAQ page. You are passing a string literal to a
    function that tries to modify it. Modifying a string literal is
    undefined behavior. One possibility of undefined behavior is a
    program "crash".

    > printf("return status rc=%d\n", rc);
    >
    > return 0;
    > }
    > -----
    >
    > As I stated earlier, segfault happens at XXX mark. If I'm right, then
    > compiler treats that line as follows:
    > 1) evaluate *sp
    > 2) put '\0' into memory area pointed by 'sp'
    > 3) increment pointer value
    >
    > So, I suspect crash occurs at second step.
    > [OT]
    > By the way, no faults happen while debug in GDB. Basically debuggers are
    > supposed to reveal such kinds of problems.
    > [/OT]
    >
    > With best regards, Roman Mashak. E-mail:


    At the top of main(), add this definition:

    char url_to_parse [] = "ftp://192.168.11.197/pub/test.txt";

    ....then change the function call to:

    int rc = parse_url(url_to_parse, 0);

    --
    Jack Klein
    Home: http://JK-Technology.Com
    FAQs for
    comp.lang.c http://www.eskimo.com/~scs/C-faq/top.html
    comp.lang.c++ http://www.parashift.com/c -faq-lite/
    alt.comp.lang.learn.c-c++
    http://www.contrib.andrew.cmu.edu/~ajo/docs/FAQ-acllc.html
    Jack Klein, Aug 12, 2005
    #10
  11. "Roman Mashak" <> writes:
    [...]
    > Why is it not assumed that array (char s[] = "abcd") can be also allocated
    > in ROM? My compiler has option allowing to treat string literals as writable
    > (but it's not safe).


    Because char s[] = "abcd" creates an array of 5 characters with the
    initial value {'a', 'b', 'c', 'd', '\0'}. The array is not const, and
    it's not a string literal; it's perfectly legal to modify it. (A
    string literal in an initializer doesn't create the same kind of
    not-const-but-don't-touch-it array that a string literal in most other
    contexts create.)

    --
    Keith Thompson (The_Other_Keith) <http://www.ghoti.net/~kst>
    San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
    We must do something. This is something. Therefore, we must do this.
    Keith Thompson, Aug 12, 2005
    #11
  12. Roman Mashak

    Roman Mashak Guest

    Hello, Jack!
    You wrote on Thu, 11 Aug 2005 21:30:05 -0500:

    JK> This is EXACTLY what three people just told you, and even posted links
    JK> to the specific FAQ page. You are passing a string literal to a
    JK> function that tries to modify it. Modifying a string literal is
    JK> undefined behavior. One possibility of undefined behavior is a
    JK> program "crash".
    I've read the links carefully but didn't confronted with my code properly.
    My mistake, thanks a lot!
    So, as I understand, the most safe practice is to avoid string literals? (as
    well as 'strcpy' as claimed in another thread)

    With best regards, Roman Mashak. E-mail:
    Roman Mashak, Aug 12, 2005
    #12
  13. Roman Mashak wrote:

    > So, as I understand, the most safe practice is to avoid string literals? (as
    > well as 'strcpy' as claimed in another thread)


    Avoid *modifying* string literals. String literals as such are difficult
    to avoid, just like numeric literals. There's nothing wrong with
    strcpy(), provided you know how to use it.

    Peter
    Peter Pichler, Aug 12, 2005
    #13
  14. Roman Mashak

    CBFalconer Guest

    Peter Pichler wrote:
    > Roman Mashak wrote:
    >
    >> So, as I understand, the most safe practice is to avoid string
    >> literals? (as well as 'strcpy' as claimed in another thread)

    >
    > Avoid *modifying* string literals. String literals as such are
    > difficult to avoid, just like numeric literals. There's nothing
    > wrong with strcpy(), provided you know how to use it.


    If you are using gcc, use the -Wwrite-strings option. This will
    effectively declare all those strings as const, and you will get a
    compile time error if you try to write to them.

    --
    Chuck F () ()
    Available for consulting/temporary embedded and systems.
    <http://cbfalconer.home.att.net> USE worldnet address!
    CBFalconer, Aug 12, 2005
    #14
    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. John
    Replies:
    0
    Views:
    388
  2. Tuvas

    Possible memory leak?

    Tuvas, Jan 24, 2006, in forum: Python
    Replies:
    29
    Views:
    765
    Tuvas
    Jan 30, 2006
  3. Pep

    Possible memory leak?

    Pep, Jul 21, 2008, in forum: C++
    Replies:
    11
    Views:
    948
    Matthias Buelow
    Jul 23, 2008
  4. John
    Replies:
    1
    Views:
    115
    Joe Kaplan \(MVP - ADSI\)
    Dec 15, 2004
  5. hemant
    Replies:
    3
    Views:
    165
    hemant
    Jun 26, 2008
Loading...

Share This Page