CODEWITHSUNDEEP
cgdbvalgrind
user3022048
user3022048

Reputation: 49

Segfault in C program, malloc call

I am writing a program that takes a list of path ( environmental variable), splits the paths and prints it. When compiling it I get a segfault. The following is my output on GDB :

Program received signal SIGSEGV, Segmentation fault.
0x0000000000400eb0 in dest (name=0x7fffffffbce0 "PATH") at executables.c:100
100     dest[i] = malloc(srclen+1);

On valgrind: 

==21574== 1 errors in context 2 of 3:
==21574== Use of uninitialised value of size 8
==21574==    at 0x400EB0: dest (executables.c:100)
==21574==    by 0x400B5B: main (main.c:9)

This is my function: 

char** dest(char *name){
    int i=0;
    char *vp;
    const char s[2]=":";
    char *token;
    char **dest;
    name[strlen(name)-1]='\0';
    vp=getenv(name);
    if(vp == NULL){
        exit(1);
    }
    token =strtok(vp,s);
    while( token != NULL ){
        size_t srclen = strlen(token);
        dest[i] = malloc(srclen+1);
        strcpy(dest[i], token);
        token = strtok(NULL, s);
        i++;
    }
    dest[i]=NULL;
    return dest;
}

And this is my main:

#include "executables.h"
int main(int argc, char **argv){
    char *path;
    char name[BUFSIZ];
    printf("enter name of environment variable:\n");
    fgets(name,BUFSIZ,stdin);
    char **p=dest(name);
    int j=0;
    while(p[j]!=NULL){
        printf("%s\n",p[j]);
        j++;
    }
    return(0);
}

Upvotes: 1

Views: 158

Answers (3)

Gerhardh
Gerhardh

Reputation: 12404

From the manpage of getenv:

Notes ... As typically implemented, getenv() returns a pointer to a string within the environment list. The caller must take care not to modify this string, since that would change the environment of the process.

Your code violates that rule:

vp=getenv(name);
...
token =strtok(vp,s);

This is an illegal memory write operation.

Upvotes: 1

clearlight
clearlight

Reputation: 12625

Use strdup(). Saves steps (accounts for '\0' too). You have to allocate some memory before hand for the approach you're using. Otherwise you might want a linked list and allocate packets instead of using the array pattern. When you say dest[i] = <ptr value> you're indexing to an offset of unallocated memory and storing something there, so it's a segvio.

#include <string.h>
#define MAXTOKENS  10000

char **get_dest(char *name) {
    // Since dest has to be exposed/persist beyond this function all
    // need dynamically allocate (malloc()) rather than stack allocate
    // of the form of: char *dest[MAXTOKENS]. 
    char *dest = malloc(MAXTOKENS * sizeof (char *));  // <--- need to allocate storage for the pointers
    char *vp;
    if ((vp = getenv(name)) == NULL)
        exit(-1); // -1 is err exit on UNIX, 0 is success

    int i = 0;
    char *token = strtok(vp, ":");
    while (token != NULL) {
        dest[i] = strdup(token); // <=== strdup()
        token = strtok(NULL, ":");
        i++;
    }

//  dest[i] = NULL;  // Why are you setting this to NULL after adding token?
    return dest;

}

It's better if main() takes care of passing a proper null-terminated string to the get_dest() function because main is where the finicky fgets() is handled. Generally you want to do things locally where it makes the most sense and is most relevant. If you ever took your get_dest() function and used it somewhere where the strings were not read by fgets() it would just be a wasted step to overwrite the terminator there. So by initializing the char array to zeroes before fgets() you don't have to worry about setting the trailing byte to '\0'.

And finally probably not good to have your function name dest the same name as the variable it returns dest. In some situations having multiple symbols in your program with the same name can get you into trouble.

#include "executables.h"
int main(int argc, char **argv) {
    char *path;
    char name[BUFSIZ] = { 0 };  // You could initialize it to zero this way
    printf("enter name of environment variable:\n");
//  bzero(name, BUFSIZ); //... or you could initialize it to zero this way then
    fgets(name, BUFSIZ, stdin);
    char **p = get_dest(name);
    int j = 0;
    while(p[j] != NULL) {
        printf("%s\n", p[j]);
        j++;
        free(p[j]);  // like malloc(), strdup'd() strings must be free'd when done
    }
    free(p);
    return 0;
}

Upvotes: 2

synchronizer
synchronizer

Reputation: 2075

    dest[i] = malloc(srclen + 1);

You need to allocate memory for the pointer to char pointers (dest) as well as each char pointer stored in dest. In the code you provided, neither step is taken.

Upvotes: 2

Related Questions