Segmentation fault: 11 while trying to parse string

Question

I'm trying to parse an input string into a command string and an array of arguments strings.

I'm having some issue using strtok and strcpy, I think that my command string is not being null terminated properly which is leading to the seg fault.

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

#define delims " \t\r\n"

int main() {
    char input[] = "ls -a -f";
    char *buffer;
    char command[256];
    char arguments[256][256];
    int i = 0, j;

    buffer = strtok(input, delims);

    strcpy(command, buffer);

    printf("Command: %s\n\r", command);

    while (buffer != NULL)
    {
        buffer = strtok(NULL, delims);

        strcpy(arguments[++i], buffer);
    }

    buffer = strtok(NULL, delims);

    for (j = 0; j < i; ++i)
    {
        printf("Argument[%d]: %s", j, arguments[j]);
    }

    return 0;
}

Current Output:

Command: ls
Segmentation fault: 11

Expected Output:

Command: ls
Argument[0]: -a
Argument[1]: -f

I don't pretend to be very good with C, so any pointers in the right direction would be extremely helpful.

That's not how to structure the loop. Change while (buffer != NULL) to while (strtok(NULL, delims)) — bruceg, Mar 02 '16 at 23:37
`strcpy(arguments[++i], buffer);` -->> `strcpy(arguments[i++], buffer);` — wildplasser, Mar 02 '16 at 23:42
`while (buffer != NULL) { buffer = strtok(NULL, delims); strcpy(arguments[++i], buffer); } buffer = strtok(NULL, delims);` --> `while ((buffer = strtok(NULL, delims)) != NULL) { strcpy(arguments[i++], buffer); }` — BLUEPIXY, Mar 02 '16 at 23:58

score 3 · Answer 1 · answered Mar 03 '16 at 00:01

Your problem likely revolves around the line strcpy(arguments[++i], buffer);. You are incrementing i, and then using it as an array index. The first round through the loop will copy into array index 1. When you print from the loop, you start at index 0. Since you don't initialize the arrays, they're full of garbage and bad things happen when you try to print index 0 (full of garbage) as a string.

Two suggestions to fix this: First, move expressions with side effects (like ++i) to a line of their own. This makes things simpler and eliminates any order-of-operations gotchas. Second, print out the arguments as soon as you read them instead of looping back through everything a second time. Since you're just printing the values, this would mean you wouldn't need an entire array to store all of the arguments. You'd only need enough buffer to store the current argument long enough to print it.

score 1 · Answer 2 · answered Mar 03 '16 at 00:27

the following code:

compiles cleanly
removes unneeded local variables
outputs the proper items, then quits
defines magic numbers with meaningful names
uses NUL terminated array for the delimiters for strtok()
used the 'typical' name for the returned value of strtok()
always checks the returned value from strtok()

and now the code:

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


#define MAX_CMD_LEN (256)
#define MAX_ARGS    (256)
#define MAX_ARG_LEN (256)

int main( void )
{
    char input[] = "ls -a -f";
    char *token;
    char command[ MAX_CMD_LEN ] = {'\0'};
    char arguments[ MAX_ARGS ][ MAX_ARG_LEN ] = {{'\0'}};

    if ( NULL != (token = strtok(input, " \t\r\n" )) )
        strcpy(command, token);

    printf("Command: %s\n\r", command);

    size_t i = 0;
    while (i<MAX_ARGS && NULL != (token = strtok( NULL, " \t\r\n" ) ) )
    {
        strcpy(arguments[ i ], token);
        i++;
    }


    for( i=0; *arguments[i]; i++ )
    {
        printf("Argument[%lu]: %s\n", i, arguments[i]);
    }

    return 0;
} // end function: main

Segmentation fault: 11 while trying to parse string

2 Answers2