parse line from file to list in c c90-CodePudding

I'm working with c90 on linux. I have a strange bug when I want to end a string, let idx be the index, so when I get to the last index I want the list[idx] to be NULL. example:

list[0] actually "hello"
list[1] actually "world\n"
list[2] sometimes is "" or NULL

so when I put NULL to the the end of the list its deletes one of the other words..

for: list[2] = NULL;

unexpectedly list[0] turns NULL but list[1] still "world\n" and list[2] of course NULL.

I wrote this function:

void function()
{

    char buffer[BUFF_LEN];
    char** list = NULL;
    int list_len = 0;
    while (fgets(buffer, BUFF_LEN, fptr))
    {
        list = (char**)malloc((sizeof(char*)));
        get_input(buffer, list, &list_len);
        /*
        some other code
        */
    }
    free_list(list, list_len); /*free the array of strings words*/
}

and wrote also the get_input because I work with c90

void get_input(char* line, char** list, int *idx)
{
    char * token;
    *idx = 0;
    token = strtok(line, " "); /*extract the first token*/
    /* loop through the string to extract all other tokens */
    while (token != NULL)
    {
        if (token && token[0] == '\t')
            memmove(token, token   1, strlen(token));
        printf("%s\n", token); 
        list[*idx] = (char *)malloc(strlen(token) 1);
        strncpy(list[*idx], token, strlen(token));
        token = strtok(NULL, " ");   /*get every token*/
        (*idx)  ;
    }
    if (*idx == 0)
        list = NULL;
    list[*idx - 1][strcspn(list[*idx - 1], "\n")] = 0; /* remove the "\n" */
    list[*idx] = NULL; /* to know when the list ends */
}

the free function:

void free_list(char** list, int list_len)
{
    int i;
    for(i= list_len - 1; i >= 0; i--)
    {
        list[i] = NULL;
        free(list[i]);
    }
}

CodePudding user response：

You have multiple issues.

void function()
{
    char buffer[BUFF_LEN];
    char** list = NULL;
    int list_len = 0;
    while (fgets(buffer, BUFF_LEN, fptr))
    {
        list = (char**)malloc((sizeof(char*)));
        get_input(buffer, list, &list_len);
        /*
        some other code
        */
    }
    free_list(list, list_len); /*free the array of strings words*/
}

You only allocate memory for 1 pointer.
You only free the pointers in the last list.
You never free the memory for list ifself.
You should not cast the return value of malloc and friends.

This should be changed like this:

void function()
{
    char buffer[BUFF_LEN];
    char** list = NULL;
    int list_len = 0;
    while (fgets(buffer, BUFF_LEN, fptr))
    {
        list = malloc((sizeof(char*)));
        get_input(buffer, &list, &list_len);
        /*
        some other code
        */
        free_list(list); /*free the array of strings words*/
        free(list);
    }
}

The freeing function is also broken:

void free_list(char** list, int list_len)
{
    int i;
    for( i= list_len - 1; i >= 0; i--)
    {
        list[i] = NULL;
        free(list[i]);
    }
}

You set the pointer within list to NULL before you free it. This causes a memory leak as the memory is not really freed.
You don't really need the length as you have added a sentinel. But that is not an error.
There is also no need to free the pointers backwards.

After cleanup the function could look like this:

void free_list(char** list)
{
    while (list[i])
    {
        free(list[i]);
        i  ;
    }
}

Now the biggest part:

void get_input(char* line, char** list, int *idx)
{
    char * token;
    *idx = 0;
    token = strtok(line, " "); /*extract the first token*/
    /* loop through the string to extract all other tokens */
    while (token != NULL)
    {
        if (token && token[0] == '\t')
            memmove(token, token   1, strlen(token));
        printf("%s\n", token); 
        list[*idx] = (char *)malloc(strlen(token) 1);
        strncpy(list[*idx], token, strlen(token));
        token = strtok(NULL, " ");   /*get every token*/
        (*idx)  ;
    }
    if (*idx == 0)
        list = NULL;
    list[*idx - 1][strcspn(list[*idx - 1], "\n")] = 0; /* remove the "\n" */
    list[*idx] = NULL; /* to know when the list ends */
}

You do not care about memory for the pointers in your list. That means you store the pointers in memory that you are not allowed to touch. By doing this you invoke undefined behaviour.
You must realloc the memory and for that you must be able to modify the passed pointer.
You should not cast the return values of malloc and friends.
You access illegal index values if *idx==0
You call strncpy with the length of the string without space for the 0 byte. That will cause the copy to be not nul terminated. Also there is no need to use strncpy over strcpy as you have reserved enough memory.

void get_input(char* line, char*** list, int *idx)
{
    char *token;
    char **list_local = *list; // Make things easier by avoiding one * within the function.
    *idx = 0;
    token = strtok(line, " "); /*extract the first token*/

    /* loop through the string to extract all other tokens */
    while (token != NULL)
    {
        if (token[0] == '\t')  // No need to check for `token` again
            memmove(token, token   1, strlen(token));

        printf("%s\n", token); 

        list_local[*idx] = malloc(strlen(token) 1);
        strcpy(list_local[*idx], token);
        token = strtok(NULL, " ");   /*get every token*/
        (*idx)  ;

        /* Increase array size to hold 1 more entry. */
        /* That new element already includes memory for the sentinel NULL */
        {
            char ** temp = realloc(list_local, sizeof(char*) * (*idx));
            if (temp != NULL)            
                list_local = temp;
            // TODO: error handling ...
        }

    }
    if (*idx != 0)
    {
        list_local[*idx - 1][strcspn(list_local[*idx - 1], "\n")] = 0; /* remove the "\n" */
    }

    list_local[*idx] = NULL; /* to know when the list ends */
    *list = list_local;
}