what's wrong with this path combine function?


A

Alia K

I followed some of the advice on this page (http://efreedom.com/
Question/1-3142365/Combine-Directory-File-Path-C) to rewrite a path
combine functions as below. Even though it seems to work, I suspect
I'm doing something wrong because (1) I am not including the
possibility of the "/" length in malloc and (2) I am probably causing
a memory leak because the pointer returned by malloc is not freed.

Now, because it seems to work ok (no complaints with gcc or clang -
Wall) I'm unsure whether it's a good function or not (-:

Any advice would be appreciated. Please find the function below:

<code>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

char *combine(const char *path1, const char *path2)
{
char *destination = malloc(
strlen(path1) + strlen(path2));

if (path1 == NULL && path2 == NULL) {
strcpy(destination, "");
} else if (path2 == NULL || !path2[0]) {
strcpy(destination, path1);
} else if (path1 == NULL || !path1[0]) {
strcpy(destination, path2);
} else {
char directory_separator[] = "/";

const char last_char = path1[strlen(path1) - 1];
strcpy(destination, path1);

if (last_char != directory_separator[0])
strcat(destination, directory_separator);
strcat(destination, path2);
}
return destination;
}

int main(int argc, char **argv)
{
printf("res: %s\n", combine("path1", "path2"));
return EXIT_SUCCESS;
}
</code>

Thank you.

AK
 
Ad

Advertisements

M

Malcolm McLean

        char *destination = malloc(
            strlen(path1) + strlen(path2));
You need
strlen(path1) + strlen(path2) + strlen(path_separator) + 1

(maximum length of the components plus 1 for the nul).
 
I

Ike Naar

char *destination = malloc(
strlen(path1) + strlen(path2));

This will blow up if path1 or path2 is a null pointer.

You also need to allocate an extra byte for the path separator,
plus one for the null character that terminates the string.

char *destination = malloc( (path1==NULL ? 0 : strlen(path1))
+ 1 /* for path separator */
+ (path2==NULL ? 0 : strlen(path2))
+ 1 /* for string terminator */
);

malloc can fail and return NULL; you should check for this.
 
P

PKM

I followed some of the advice on this page (http://efreedom.com/
Question/1-3142365/Combine-Directory-File-Path-C) to rewrite a path
combine functions as below. Even though it seems to work, I suspect
I'm doing something wrong because (1) I am not including the
possibility of the "/" length in malloc and (2) I am probably causing
a memory leak because the pointer returned by malloc is not freed.

Now, because it seems to work ok (no complaints with gcc or clang -
Wall) I'm unsure whether it's a good function or not (-:

I count 12 bytes that need to be allocated: "path1/path2" is eleven
characters,
and one for the terminating null. I've never gotten a compiler to
complain
about this; it's usually a run-time error if something goes wrong. In
this case,
it worked for some reason (on my system too), but to be safe, I would
allocate
those two extra bytes.

As for memory leaks, well, yes, I usually try to remember to free
those chunks of
memory. The system is supposed to do that when the program exits, and
so for a simple
piece of code like the one you give it won't cause a problem. What
happens to me, though,
is that I will start with something simple like that, just to test
some feature, and then
build directly on to that as I develop more of a program. If I don't
put some free() calls
in early in the process, it can be hard to find them later.

If you run this program with valgrind -v, you'll see some cryptic
error messages about
your memory allocation. If you then malloc the two missing bytes,
those messages will go away.
 
A

Alia K

Thank you all for the helpful responses which are well-appreciated. I
will certainly add valgrind to my tool chest as it seems to pick up
these kinds of things.

Best,

AK
 
T

tperk

I followed some of the advice on this page (http://efreedom.com/
Question/1-3142365/Combine-Directory-File-Path-C) to rewrite a path
combine functions as below. Even though it seems to work, I suspect
I'm doing something wrong because (1) I am not including the
possibility of the "/" length in malloc and (2) I am probably causing
a memory leak because the pointer returned by malloc is not freed.

Now, because it seems to work ok (no complaints with gcc or clang -
Wall) I'm unsure whether it's a good function or not (-:

Any advice would be appreciated. Please find the function below:

<code>
#include<stdlib.h>
#include<stdio.h>
#include<string.h>

char *combine(const char *path1, const char *path2)
{
char *destination = malloc(
strlen(path1) + strlen(path2));

if (path1 == NULL&& path2 == NULL) {
strcpy(destination, "");
} else if (path2 == NULL || !path2[0]) {
strcpy(destination, path1);
} else if (path1 == NULL || !path1[0]) {
strcpy(destination, path2);
} else {
char directory_separator[] = "/";

const char last_char = path1[strlen(path1) - 1];
strcpy(destination, path1);

if (last_char != directory_separator[0])
strcat(destination, directory_separator);
strcat(destination, path2);
}
return destination;
}

int main(int argc, char **argv)
{
printf("res: %s\n", combine("path1", "path2"));
return EXIT_SUCCESS;
}
</code>

Thank you.

AK
>char *destination = malloc(
> strlen(path1) + strlen(path2));
Add one for the NUL-terminator character.
i.e., strlen(path1) + strlen(path2) + 1
 
Ad

Advertisements

B

Ben Bacarisse

tperk said:
On 1/23/2011 6:50 AM, Alia K wrote:
char *combine(const char *path1, const char *path2)
{
char *destination = malloc(
strlen(path1) + strlen(path2));

if (path1 == NULL&& path2 == NULL) {
strcpy(destination, "");
} else if (path2 == NULL || !path2[0]) {
strcpy(destination, path1);
} else if (path1 == NULL || !path1[0]) {
strcpy(destination, path2);
} else {
char directory_separator[] = "/";

const char last_char = path1[strlen(path1) - 1];
strcpy(destination, path1);

if (last_char != directory_separator[0])
strcat(destination, directory_separator);
strcat(destination, path2);
}
return destination;
}

int main(int argc, char **argv)
{
printf("res: %s\n", combine("path1", "path2"));
return EXIT_SUCCESS;
}
</code>

Thank you.

AK
char *destination = malloc(
strlen(path1) + strlen(path2));
Add one for the NUL-terminator character.
i.e., strlen(path1) + strlen(path2) + 1

For the record, another +1 is needed for the path separator. If the
allocation can be done later this allocation can be conditional on
last_char != directory_separator[0].
 

Ask a Question

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

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Top