Search code examples
cfgetsstrtok

Get rid of newline from fgets - keep getting seg fault


I'm trying to parse inputs from the command line but keep getting a seg fault in my C program. Any thoughts on where the issue is? Right now I want to get rid of the \n at the end of the user input.

void assign_to_array(char *command){
    char *in;
    int len;
    char *pos;

    char **info = calloc(10,sizeof(char*));

    for(int i=0;i<10;i++){
        info[i]=calloc(50,sizeof(char));
    }

    in = strtok(command," ");
    if((pos=strchr(in,'\n')) != NULL)
        *pos='\0';

    while (in != NULL){
        in = strtok(NULL," ");
        if((pos=strchr(in,'\n')) != NULL)
                *pos='\0';
    }

    for(int i=0;i<3;i++)
        free(info[i]);
    free(info);
}

Solution

  • There are a few things that could be checked to be sure not to segfault. Here's the main thing:

    while (in != NULL){
        in = strtok(NULL," ");
        if((pos=strchr(in,'\n')) != NULL) *pos='\0';
    }
    

    When you do this you're checking that 'in' is not pointing to NULL at the enter of the while loop. But you do modify the memory area pointed by 'in' INSIDE the loop and you use it right after that. Here is one solution:

    in = strtok(NULL," ");
    while (in != NULL)
    {
        if((pos=strchr(in,'\n')) != NULL) *pos='\0';
        in = strtok(NULL," ");
    }
    

    So that 'in' is checked EVERY TIME before it is used.

    Next few things:

    You're using calloc(3) without checking even once that the return value is not NULL. You should ALWAYS check return values. What if your machine has run out of memory? You'll just crash...

    char **info = calloc(10,sizeof(char*));
    if (info == NULL) return;
    
    for(int i=0;i<10;i++){
        info[i]=calloc(50,sizeof(char));
        if (info[i] == NULL)
          return;
    }
    

    You can't return an error value since the function assign_to_array is 'void' typed. Still I suggest that this function should return an int or something to avoid any side effect.

    Also, you're alloc'ing an array of 10 strings each with 50 characters (which you're not using actually...). At the end you're only freeing the first 3 strings.

    If this array is every time equal to 10 strings of 50 characters each I would suggest not to calloc but simply to create it on the stack (like this char info[10][50];) so that you don't have to bother freeing it at the end or callocating it at the beginning. The drawback of this technique is that you won't be able to return the array but you don't seem to do.

    I hope I've been helpful.