possible memory leak?

[Roman Mashak]

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: (e-mail address removed)

 

[Peter Pichler]

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 hope I have managed to clear at least some of the confusion.

Peter

 

[kernelxu]

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]

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]

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 address removed)

 

[Suman]

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 address removed)

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

 

[David Resnick]

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]

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

 

[Roman Mashak]

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 address 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 address removed)

 

[Jack Klein]

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 address 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 address 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);

 

[Keith Thompson]

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

 

[Roman Mashak]

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: (e-mail address removed)

 

[Peter Pichler]

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

 

[CBFalconer]

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.