Resolving filepath to executable in C

Question

I am currently tasked with a homework assignment to write a primitive shell in C, and am having difficulty implementing the shell feature which constructs a path to a given requested program. e.g. Transforming user input of wc to /usr/bin/wc.

Getenv() is working fine to get the value of $PATH. Using code supplied by my instructors, I've also parsed this value into individual 'tokens,' where a token is defined: typedef char *tok_t

My question is how can I fix this implementation of the following function, which seeks to return the absolute path to a given filename if found, and NULL otherwise.

The main issue here is concatenating a tok_t and a char* to produce the full pathname.

char *resolve_path(char *filename) {
    printf("trying to resolve path...\n");
    char *path_var = getenv("PATH");
    tok_t *path_list = get_toks(path_var);
    //fprint_tok(stdout, path_list);
    char *path;
    for (int i = 0; path_list[i]; i++) {
       path = (char*) malloc(PATH_MAX);
       strcat(path, *path_list[i]);
       strcat(path, filename);
       printf("Trying... %s\n", path);
       if (file_exists(path)) {
           return path;
       }
       free(path);
    }
    return NULL;
}

Should I bother with malloc() and strcat(), or is there some better way of implementing this? Currently getting segfaults and warnings about the type compatibility in use of strcat().

What is `tok_t`? If you do want to use `malloc`, you don't need to do it each time through the loop.. — John3136, Sep 16 '15 at 02:38

score 2 · Accepted Answer · edited May 23 '17 at 12:03

You do need to use malloc() since you are returning the resulting path from the function (a pointer to an automatic array created in this function will not be valid after the function returns). You do need to use strcat() or similar in order to produce a single contiguous char * to pass to file_exists().

There are a few issues with your code, however:

Do not explicitly cast void * to other types in C - at best, it is unnecessary (I'm talking about casting the return value of your allocation, in this case).
Check to see if malloc() fails.
You don't need to call malloc() and free() inside your loop - just once (each) outside the loop is sufficient.
If tok_t is a char * then path_list is a char **, so no need to dereference path_list[i] when you pass it to strncpy()/strncat() as that would merely be a char, when they expect strings. This is the likely cause of your statement "Currently getting segfaults and warnings about the type compatibility in use of strcat()."
You need to set the first character of path to NULL before your first call to strcat(), or better, use strncpy(), in which case you will want to set the last character of path to NULL after you're done.
Use strncat() with PATH_MAX - strlen(path) because otherwise you could overflow path.

Here's an example:

char *resolve_path(const char *filename) {
    printf("trying to resolve path...\n");
    char *path_var = getenv("PATH");
    tok_t *path_list = get_toks(path_var);
    char *path = malloc(PATH_MAX+1); // See, no cast, and the +1 is for the NULL
    if (!path) {
        return NULL; // Check for failure
    }
    // strncpy/strncat won't null-terminate path if we run out of space
    path[PATH_MAX] = NULL;
    for (int i = 0; path_list[i]; i++) {
       // this could be done more efficiently with memcpy/hand-coding/etc
       strncpy(path, path_list[i], PATH_MAX); // don't dereference it
       strncat(path, filename, PATH_MAX - strlen(path));
       printf("Trying... %s\n", path);
       if (file_exists(path)) {
           return path;
       }
    }
    free(path);
    return NULL;
}

Thank you for the suggestions, I was able to get the function working with this implementation minus the malloc(), instead declaring path as char path[PATH_MAX + 1]. — theRook, Sep 16 '15 at 03:09
@theRook Please don't do that, as you are returning a pointer to an array on the stack when you do that. You need to malloc() if you want to safely keep the string around after the function returns. It may have worked *this time* or *for now* but it's unsafe. I accidentally suggested you could do such a thing initially but please re-read my answer. — Iskar Jarak, Sep 16 '15 at 03:12

score 0 · Answer 2 · edited May 23 '17 at 12:22

I'm expounding on @Iskar_Jarak's answer because using that code as-is still results in a segmentation fault.

Because tok_t is just a typedef for char*, it sort of muddles your code. get_toks() is (without typedef's) technically just returning a char**.

That means this line:

strncpy(path, *path_list[i], PATH_MAX);

should actually be

strncpy(path, path_list[i], PATH_MAX);

Because you shouldn't dereference path_list[i]. If you do, you pass a char tostrncpy when it takes a char*. That's why you're getting that warning and a seg fault.

So, for what it's worth, here're my corrections to your code:

char *resolve_path(const char *filename) {
    printf("trying to resolve path...\n");
    char *path_var = getenv("PATH");
    tok_t *path_list = get_toks(path_var);
    char *path = malloc(PATH_MAX+1);
    if (path == NULL) {
        return NULL; // malloc failed
    }
    path[PATH_MAX] = '\0'; // null terminate path
    for (int i = 0; path_list[i]; i++) {
       size_t len =  strlen(path_list[i]); // get the length of this path
       strncpy(path, path_list[i], PATH_MAX); // copy path_list to path
       path[len] = '/'; // add seperator
       strncat(path, filename, PATH_MAX - len - 1); // -1 because of separator
       printf("Trying... %s\n", path);
       if (file_exists(path)) {
           return path;
       }
    }
    free(path);
    return NULL;
}

Oh, I missed that. I've updated my answer. Note that a) it should result in a compiler warning (the classic *makes pointer from integer without a cast*) and b) it won't necessarily result in a segmentation fault, but at the very least using the resulting pointer is undefined behaviour. — Iskar Jarak, Sep 16 '15 at 04:43

Resolving filepath to executable in C

2 Answers2