Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > possible memory leak?

Reply
Thread Tools

possible memory leak?

 
 
Roman Mashak
Guest
Posts: n/a
 
      08-11-2005
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: http://www.velocityreviews.com/forums/(E-Mail Removed)


 
Reply With Quote
 
 
 
 
Peter Pichler
Guest
Posts: n/a
 
      08-11-2005
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

 
Reply With Quote
 
 
 
 
kernelxu@hotmail.com
Guest
Posts: n/a
 
      08-11-2005
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.

 
Reply With Quote
 
junky_fellow@yahoo.co.in
Guest
Posts: n/a
 
      08-11-2005

(E-Mail Removed) 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.

 
Reply With Quote
 
Roman Mashak
Guest
Posts: n/a
 
      08-11-2005
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: (E-Mail Removed)


 
Reply With Quote
 
Suman
Guest
Posts: n/a
 
      08-11-2005

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: (E-Mail Removed)

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

 
Reply With Quote
 
David Resnick
Guest
Posts: n/a
 
      08-11-2005

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

 
Reply With Quote
 
Peter Pichler
Guest
Posts: n/a
 
      08-11-2005
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: (E-Mail Removed)


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

 
Reply With Quote
 
Roman Mashak
Guest
Posts: n/a
 
      08-12-2005
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: (E-Mail Removed)

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: (E-Mail Removed)


 
Reply With Quote
 
Jack Klein
Guest
Posts: n/a
 
      08-12-2005
On Fri, 12 Aug 2005 11:12:49 +0900, "Roman Mashak" <(E-Mail Removed)>
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: (E-Mail Removed)
>
> 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: (E-Mail Removed)


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
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
efficient data loading with Python, is that possible possible? igor.tatarinov@gmail.com Python 10 12-14-2007 04:44 PM
is it possible to possible to create an iterator from a callback interace? aninnymouse@gmail.com C Programming 4 02-21-2006 02:10 PM
XML + XSD: Is it possible to get all possible Values for an Element? Markus Java 1 11-22-2005 02:51 PM
Authorization Manager, ASP.NET, possible memory leak John ASP .Net 0 12-15-2004 06:43 PM
Differences between Sony Memory Stick & memory Stick Pro vs Memory Stick Duo? zxcvar Digital Photography 3 11-28-2004 10:48 PM



Advertisments