Strange behaviour in assigning char**[i]-CodePudding

While debugging a simple shell application, I came across a strange bug when a command has >2 parameters. I tracked it down to this function (string[] is the command line, e.g. echo one two and sep is the character to split the string by, set to ' ' where it is called):

char **split(char string[], char *sep) {
    char *token = strtok(string, sep);
    char **argv = calloc(1, sizeof(char*));
    
    int i = 0;
    while (token != NULL) {
        argv = realloc(argv, sizeof(argv)   sizeof(char*));
        argv[i] = calloc(strlen(token), sizeof(char));
        strcpy(argv[i  ], token);
        token = strtok(NULL, sep);
    }
    argv[i] = NULL; // sets argv[0] even if i == 4
    return argv;
}

It runs fine with <=2 parameters, splitting a string into a char** and null terminating it. However with 3 parameters, argv[0] ends up getting set to null in at the end instead of the last element.

Am I doing something stupid or is there undefined behaviour?

edit: >=4 parameters causes crashes the program:

realloc(): invalid next size
signal: aborted (core dumped)

CodePudding user response：

The problem is on these two lines:

    argv = realloc(argv, sizeof(argv)   sizeof(char*));
    argv[i] = calloc(strlen(token), sizeof(char));

sizeof(argv) gives you the size of a pointer (usually 4 or 8), not the number of items it points to. So when you call realloc you're always asking for the same number of bytes, which happens to be enough for two array items. Then when you have more than 2 you write past the bounds of allocated memory triggering undefined behavior.

Since i is keeping track of the length of the array, use that when calling realloc, and you need to multiply by the element size instead of add.

    argv = realloc(argv, (i   1) * sizeof(char*));

On the next line, you allocate enough space for the characters in the string but not enough for the terminating null byte, so add 1 for that:

    argv[i] = calloc(strlen(token)   1, sizeof(char));

CodePudding user response：

You have three bugs in your code.

strlen() gives the string length without the null terminator. So you have to write calloc(strlen(token) 1, sizeof(char)); to make space for it.
sizeof(argv) returns the size of the pointer (not the number of elements) and is constant at compile time.
argv[i] = NULL; is out of bounds

Working code:

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

char **split(char string[], char *sep)
{
    char *token = strtok(string, sep);
    char **argv = calloc(1, sizeof(char *));
    int i = 0;
    while(token != NULL)
    {
        argv = realloc(argv, (i   1) * sizeof(char *));
        argv[i] = calloc(strlen(token)   1, sizeof(char));
        strcpy(argv[i  ], token);
        token = strtok(NULL, sep);
    }

    argv = realloc(argv, (i   1) * sizeof(char *));
    argv[i] = NULL;
    return argv;
}


int main(void)
{
    char s[] = "hello world test word";
    char **result = split(s, " ");

    printf("[0] = %s\n", result[0]);
    printf("[1] = %s\n", result[1]);
    printf("[2] = %s\n", result[2]);
    printf("[3] = %s\n", result[3]);

    return 1;

}

I recommend using gdb and valgrind for debugging such problems.