what's wrong with this path combine function?
 

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

Re: what's wrong with this path combine function?
 

On Jan 23, 1:50*pm, Alia K <alia_kho...@yahoo.com> wrote:
>
> * * * * 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).

Re: what's wrong with this path combine function?
 

On 2011-01-23, Alia K <alia_khouri@yahoo.com> wrote:
> 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.

Re: what's wrong with this path combine function?
 

On Jan 23, 4:50*am, Alia K <alia_kho...@yahoo.com> wrote:
> 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.

Re: what's wrong with this path combine function?
 

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

Re: what's wrong with this path combine function?
 

On 1/23/2011 6:50 AM, Alia K wrote:
> 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

Re: what's wrong with this path combine function?
 

tperk <da.perk@gmail.com> writes:

> On 1/23/2011 6:50 AM, Alia K wrote:
<snip>
>> 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].

--
Ben.