Split line into words and put them in char array using strtok

Question

I have this simple line parser into tokens function... But something im missing.

int parse_line(char *line,char **words){

   int wordc=0;

   /* get the first token */
   char *word = strtok(line, " ");
   words[wordc]=(char*)malloc(256*sizeof(char));
   strcpy(words[wordc++],word );

   /* walk through other tokens */
    while( word != NULL ) {
        word = strtok(NULL, " ");
        words[wordc]=(char*)malloc(256*sizeof(char));
        strcpy(words[wordc++],word );
    }

    return wordc;
}

When i run it i get a segmentation fault! I give as first argument char[256] line and as second of course a char** words but i have first malloc memory for that one. like that

  char **words = (char **)malloc(256 * sizeof(char *));

main:
.
.
.
char buffer[256];
char **words = (char **)malloc(256 * sizeof(char *));
.
.
.
n = read(stdin, buffer, 255);
if (n < 0){
   perror("ERROR");
   break;
}

parse_line(buffer,words);

When program executes parse_line it exits with segmentation fault

Found where the seg fault occures. And it's on that line here:

strcpy(words[wordc++],word );

And specifically on the first strcpy. Before it even reaches the while loop

Answer 1

while( word != NULL ) {
    word = strtok(NULL, " ");
    words[wordc]=(char*)malloc(256*sizeof(char));
    strcpy(words[wordc++],word );
}

At the end of the line, word will always be set to NULL (as expected) and so strcpy(words[wordc++],word ) will be undefined behavior (likely a crash).

You need to reorganize the loop so you never try to copy a NULL string.

@jxh suggests this solution which fixes the issue of word being NULL in either of your strcpys.

/* get the first token */
char *word = strtok(line, " ");

while( word != NULL ) {
    words[wordc]=(char*)malloc(256*sizeof(char));
    strcpy(words[wordc++],word );
    word = strtok(NULL, " ");
}

I'd do this (uses less memory)

/* get the first token */
char *word = strtok(line, " ");

while( word != NULL ) {
    words[wordc++] = strdup(word);
    word = strtok(NULL, " ");
}

Answer 2

the following proposed code:

1. cleanly compiles
2. performs the desired functionality
3. properly checks for errors
4. displays the results to the user
5. fails to pass all allocated memory to free() so has lots of memory leaks

and now the proposed code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

// avoid 'magic' numbers in code
#define MAX_WORDS 256
#define MAX_LINE_LEN 256


int parse_line( char *line, char **words )
{
    int wordc=0;

    /* get the first token */
    char *token = strtok(line, " ");
    while( wordc < MAX_WORDS && token ) 
    {   
        words[wordc] = strdup( token );
        if( ! words[wordc] )
        {
            perror( "strdup failed" );
            exit( EXIT_FAILURE );
        }

        // implied else, strdup successful

        wordc++;

        // get next token
        token = strtok(NULL, " ");
    }

    return wordc;
}



int main( void )
{
    char buffer[ MAX_LINE LENGTH ];

    // fix another problem with OPs code
    char **words = calloc( MAX_WORDS, sizeof( char* ) );
    if( ! words )
    {
        perror( "calloc failed" );
        exit( EXIT_FAILURE );
    }

    // implied else, calloc successful

    // note: would be much better to use 'fgets()' rather than 'read()'
    ssize_t n = read( 0, buffer, sizeof( buffer ) );
    if (n <= 0)
    {
       perror("read failed");
       exit( EXIT_FAILURE );
    }

    // implied else, read successful

    // note: 'read()' does not NUL terminate the data
    buffer[ n ] = '\0';   

    int count = parse_line( buffer, words );

    for( int i = 0; i < count; i++ )
    {   
        printf( "%s\n", words[i] );
    } 
}

here is a typical run of the program:

hello old friend  <-- user entered line
hello
old
friend

Answer 3

Your answers are right ! BUT i had segF again BECAUSE OF READ!!!!! i didn't notice that when i run the program it didn't stop for reading from the input at read ! Instead it was passing it. What i did is i changed read to fgets and it worked !!! With also your changes! Can someone explain to me this???? Why it doesn't stop at read function??