Tokenizing user input in C (store in **arg)?

Question

I'm attempting to write a simple shell like interface, that takes in a users input (by char) and stores it via a pointer to a pointer* (exactly how argv works). Here's my code:

char input[100];
char **argvInput;
char ch;
int charLoop = 0;
int wordCount = 0;

argvInput = malloc(25 * sizeof(char *));

while((ch = getc(stdin))) {
    if ((ch == ' ' || ch == '\n') && charLoop != 0) {
        input[charLoop] = '\0';
        argvInput[wordCount] = malloc((charLoop + 1) * sizeof(char));
        argvInput[wordCount] = input;
        charLoop = 0;
        wordCount++;

        if (ch == '\n') {
            break;
        }

    } else if (ch != ' ' && ch != '\n') {
          input[charLoop] = ch;
          charLoop++;
        } else {
            break;
        }
    }

If I loop through argvInput via:

int i = 0;
for (i = 0; i < wordCount; i++)
    printf("Word %i: %s\n", i, argvInput[i]);

All of the values of argvInput[i] are whatever the last input assignment was. So if I type: "happy days are coming soon", the output of the loop is:

Word 0: soon
Word 1: soon
Word 2: soon
Word 3: soon
Word 4: soon

I'm at a loss. Clearly each loop is overwriting the previous value, but I'm staring at the screen, unable to figure out why...

Answer 1

This line is your bane:

    argvInput[wordCount] = input;

Doesn't matter that you allocate new space, if you're going to replace the pointer to it with another one (i.e. input).

Rather, use strncpy to extract parts of the input into argvInput[wordCount].

Answer 2

argvInput[wordCount] = input; is only making the pointer of argvInput[wordCount] point to the memory of input instead of copy the content of input into the new allocated memory. You should use memcpy or strcpy to correct your program.

After the pointer assignment the memory status looks like the image below. The memory allocated by malloc((charLoop + 1) * sizeof(char));, which are the grey ones in the graph, could not be accessed by your program anymore and this will lead to some memory leak issue. Please take care of that.

Answer 3

I suggest printing your argvInput pointers with %p, instead of %s, to identify this problem: printf("Word %i: %p\n", i, (void *) argvInput[i]);

What do you notice about the values it prints? How does this differ from the behaviour of argv? Try printing the pointers of argv: for (size_t x = 0; x < argc; x++) { printf("Word %zu: %p\n", x, (void *) argv[x]); }

Now that you've observed the problem, explaining it might become easier.

This code allocates memory, and stores a pointer to that memory in argvInput[wordCount]: argvInput[wordCount] = malloc((charLoop + 1) * sizeof(char)); (by the way, sizeof char is always 1 in C, so you're multiplying by 1 unnecessarily).

This code replaces that pointer to allocated memory with a pointer to input: argvInput[wordCount] = input; ... Hence, all of your items contain a pointer to the same array: input, and your allocated memory leaks because you lose reference to it. Clearly, this is the problematic line; It doesn't do what you initially thought it does.

It has been suggested that you replace your malloc call with a strdup call, and remove the problematic line. I don't like this suggestion, because strdup isn't in the C standard, and so it isn't required to exist.

strncpy will work, but it's unnecessarily complex. strcpy is guaranteed to work just as well because the destination array is allocated to be large enough to store the string. Hence, I recommend replacing the problematic line with strcpy(argvInput[wordCount], input);.

Another option that hasn't been explained in detail is strtok. It seems this is best left unexplored for now, because it would require too much modification to your code.

I have a bone to pick with this code: char ch; ch = getc(stdin); is wrong. getc returns an int for a reason: Any successful character read will be returned in the form of an unsigned char value, which can't possibly be negative. If getc encounters EOF or an error, it'll return a negative value. Once you assign the return value to ch, how do you differentiate between an error and a success?

Have you given any thought as to what happens if the first character is ' '? Currently, your code would break out of the loop. This seems like a bug, if your code is to mimic common argv parsing behaviours. Adapting this code to solve your problem might be a good idea:

for (int c = getc(stdin); c >= 0; c = getc(stdin)) {
    if (c == '\n') {
        /* Terminate your argv array and break out of the loop */
    }
    else if (c != ' ') {
        /* Copy c into input */
    }
    else if (charLoop != 0) {
        /* Allocate argvInput[wordCount] and copy input into it,
         * reset charLoop and increment wordCount */
    }
}