Velocity Reviews

Velocity Reviews (http://www.velocityreviews.com/forums/index.php)
-   C Programming (http://www.velocityreviews.com/forums/f42-c-programming.html)
-   -   possible memory leak? (http://www.velocityreviews.com/forums/t439047-possible-memory-leak.html)

Roman Mashak 08-11-2005 05:57 AM

possible memory leak?
 
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: mrv@tusur.ru



Peter Pichler 08-11-2005 07:13 AM

Re: possible memory leak?
 
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


kernelxu@hotmail.com 08-11-2005 09:14 AM

Re: possible memory leak?
 
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.


junky_fellow@yahoo.co.in 08-11-2005 09:44 AM

Re: possible memory leak?
 

kernelxu@hotmail.com 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.


Roman Mashak 08-11-2005 01:44 PM

Re: possible memory leak?
 
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: mrv@tusur.ru



Suman 08-11-2005 02:05 PM

Re: possible memory leak?
 

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: mrv@tusur.ru

Does
http://www.eskimo.com/~scs/C-faq/q16.6.html
help?


David Resnick 08-11-2005 02:06 PM

Re: possible memory leak?
 

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


Peter Pichler 08-11-2005 08:53 PM

Re: possible memory leak?
 
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: mrv@tusur.ru


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


Roman Mashak 08-12-2005 02:12 AM

Re: possible memory leak?
 
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: mrv@tusur.ru

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: mrv@tusur.ru



Jack Klein 08-12-2005 02:30 AM

Re: possible memory leak?
 
On Fri, 12 Aug 2005 11:12:49 +0900, "Roman Mashak" <mrv@tusur.ru>
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: mrv@tusur.ru
>
> 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: mrv@tusur.ru


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/~a...FAQ-acllc.html


All times are GMT. The time now is 07:13 AM.

Powered by vBulletin®. Copyright ©2000 - 2014, vBulletin Solutions, Inc.
SEO by vBSEO ©2010, Crawlability, Inc.