C String Return Function Returns Garbage

Question

I'm having trouble return a string from a function. It prints out a garbage value in the main method. I saw a similar question on this forum but the results on that page didn't help me. I DO NOT want to pass another variable into the function. I want to be able to return the string value as is. How can I go about doing so?

char *LookupPath(char **argv, char **dir)
{
    /* String Name To Be Returned */
    char path_name[MAX_PATH_LEN] = {0};
    char *result = malloc(sizeof(path_name));
    int i;

    /* Check To See If File Name Is Already An Absolute Path Name */
    if(*argv[0] == '/') {

    }

    /* Look In Path Directories */
    for(i = 0; dir[i] != NULL; i++) {
        strncat(path_name, dir[i], sizeof(path_name));
        strncat(path_name, "/", sizeof(path_name));
        strncat(path_name, argv[0], sizeof(path_name));
        result = path_name;
        if(access(result, F_OK) == 0) {
            printf("result: %s\n", result);
            return result;
        }
        path_name[0] = '\0';
    }

    /* File Name Not Found In Any Path Variable */
    return NULL;
}

Your help is greatly appreciated!

You are not using `strncat` correctly. The last argument should be the maximum number of characters *to append*, not the maximum number of characters that the string should be. Consider using `snprintf` with a `"%s/%s"` format, etc. — dreamlax, Oct 22 '13 at 05:42

Yu Hao · Accepted Answer · 2013-10-22T05:43:48.510

4

result = path_name;

should be:

strcpy(result, path_name);

Or better, get rid of path_name and use result directly.

Note that you should remember to free result when it's not used, when returning it, free it in the function that calls it. Since you return NULL in failure, in that case, free it directly, or it's a memory leak.

And you are using strncat wrong, read the manual.

edited Oct 22 '13 at 05:43

answered Oct 22 '13 at 05:38

Yu Hao

119,891
44
235
294

score 2 · Answer 2 · edited May 23 '17 at 10:31

You cannot return a local array (like your path_name) from a function. That local array is inside the call frame, which gets popped on return.

As others replied, you should do

strncpy(result, path_name, MAX_PATH_LEN);

and document the convention that the caller should free the result.

BTW, your code is quite inefficient; you are allocating a rather large chunk of MAX_PATH_LEN (often 4096) for a string which often is much smaller.

If using GNU extensions you could simply use asprintf(3) (see this) or at least remove your malloc and return strdup(path_name); and use strdup(3) (which is standard, not requiring any GNU extension).

And learn how to use valgrind.

Plus he's leaking `result` (what it was originally assigned to, that is). — Peter Huene, Oct 22 '13 at 05:38

score 2 · Answer 3 · 2013-10-22T05:57:26.387

2

You are assigning values path_name more than it can actually hold.

    strncat(path_name, dir[i], sizeof(path_name));
    strncat(path_name, "/", sizeof(path_name));
    strncat(path_name, argv[0], sizeof(path_name));

should be:

    sprintf(path_name, "%s%s%s", dir[i],"/",argv[0]);

edited Oct 22 '13 at 05:57

answered Oct 22 '13 at 05:52

1

Not `%S` but `%s` and without spaces in the format control string. – Basile Starynkevitch Oct 22 '13 at 05:53
@BasileStarynkevitch: Thank you sir, for bringing it to notice – Oct 22 '13 at 06:01
2

If you know the size of the buffer, and if the function is available, you should use `snprintf` instead, this makes it more difficult to overrun the buffer. – dreamlax Oct 22 '13 at 09:42
Thank you! I ended up using this. – Jake Z Oct 22 '13 at 15:36
@JakeZ: Good for you. And, its not about what you use. It is said "In C there is *always* a better way to get it done!". The point is, you gotta learn what to use, where to use and why to do so.. – Oct 23 '13 at 04:53

score 1 · Answer 4 · answered Oct 22 '13 at 05:38

Because of this line:

result = path_name;

This reassigns result to point to the local variable path_name, which goes out of scope when the function returns. This also means you have a memory leak.

Instead of using the temporary path_name variable, write directly into result.

C String Return Function Returns Garbage

4 Answers4

Linked

Related