alelievr / libft-unit-test

453 stars 88 forks source link

split protected, but crashes #119

Open Akronae opened 2 years ago

Akronae commented 2 years ago

I protected my split function and it passes 1 protection test, but crashes on another one: image.

My code:

char    **ft_split(char const *s, char c)
{
    int     max_arr_size;
    char    **arr;
    int     arr_i;
    int     index_of_separator;
    char    *charset;

    if (!s)
        return (NULL);
    charset = ft_char_to_str(c);
    if (ft_strlen(charset) < 1)
        return (malloc(sizeof(0)));
    max_arr_size = ft_strlen(s) / ft_strlen(charset);
    arr = malloc(sizeof(int) * max_arr_size + sizeof(0));
    arr_i = -1;
    while (TRUE)
    {
        index_of_separator = str_index_of(s, charset);
        if (index_of_separator == 0)
        {
            s++;
            continue;
        }
        if (index_of_separator == INDEX_NOT_FOUND)
            break ;
        arr[++arr_i] = substr(s, 0, index_of_separator);
        s += index_of_separator;
    }
    if (s[0])
        arr[++arr_i] = (char *)s;
    arr[++arr_i] = NULL;
    return (arr);
}
vermillionblue commented 2 years ago

I have the exact same problem.

github-actions[bot] commented 1 year ago

Hello! Thanks for contributing to the libft unit test.

Note that this repository is not maintained by the owner anymore, instead there is a bot that will automatically merge any reviewed pull requests. If you feel like it, here are some links that can help you submiting a change in the code base::

ghost commented 1 year ago

Uninitialized memory, incorrect result (and not enough memory to store a NULL pointer either; see below re: sizeof(0))

malloc != calloc, the returned memory may contain anything:

if (ft_strlen(charset) < 1)
    return (malloc(sizeof(0)));

Example test where this would fail:

char    **result;
int i;

result = ft_split("whatever", '\0');
if (result != NULL) // true, since you returned memory allocated with malloc (unless there was a malloc failure)
{
    i = 0;
    while (result[i] != NULL)
    {
        // reading 8 bytes, but only allocated 4 bytes of memory, in *theory*, even just checking result[0] != NULL could crash
        // the allocated memory may contain anything, combined with the over-reading, result[0] != NULL will likely evaluate true
        printf("%s\n", result[0]); // reading unknown/unitialized memory until '\0' is found, very likely to read way past the end of said memory and crash
        i++;
    }
}

Example of something that would/should work instead:

if (ft_strlen(charset) < 1) // note: only happens when c == '\0'
{
    arr = malloc(2 * sizeof(char *)); // (on 64-bit systems) 8 bytes per pointer, two pointers in the returned array
    arr[0] = strdup(s); // or equivalent, splitting a string by the NUL terminator, said string should still be in the result of ft_split
    arr[1] = NULL;
    return (arr);
}

charset is a string containing a single char strlen(charset) is always either 1 or 0 (except when/if ft_char_to_str is broken, I guess) strlen(charset) is 0 if c (and therefore charset[0]) is '\0' this was already checked earlier in your code, so here, ft_strlen(charset) is always 1 --> pointless division (something / 1 == something):

max_arr_size = ft_strlen(s) / ft_strlen(charset);

sizeof(int) != sizeof(char *) (or any other pointer type) on 64-bit systems (typically) 4 bytes for int vs. 8 bytes for 64-bit pointers

arr = malloc(sizeof(int) * max_arr_size + sizeof(0));

despite the above mistake, you often have enough memory; simply because you over-allocate significantly for most cases: e.g. ft_split("0123456789\nABCDEF", '\n'); requires an array of three char *:

but you allocate strlen("0123456789\nABCDEF") (17) times sizeof(int) (typically 4) + sizeof(0) (== sizeof(int)): 72 bytes…


also, why sizeof(0)? are you trying to do sizeof(NULL)? sizeof(0): 0 is a literal constant of type int, so sizeof(0) == sizeof(int) (typically 4 bytes) sizeof(NULL): in C, NULL is 0 cast to a pointer type, so e.g. sizeof(NULL) == sizeof((void *)0) (8 bytes on 64-bit systems, not 4 like sizeof(0))