0

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]);
    }
}
Ruslan Ver
  • 127
  • 4
  • 12
  • `list = (char**)malloc((sizeof(char*)));` There is no `list[1]` you only allocate memory for one pointer. – Gerhardh Aug 02 '22 at 15:57
  • `if (*idx == 0) list = NULL; list[*idx - 1][...]=...` That won't fly. First of all, `list=NULL` will not be visible after leaving that function. Second, assigning something to `NULL[-1][...]=...` doesn't look like a great idea. There should be an `else` I guess... – Gerhardh Aug 02 '22 at 16:00
  • Idx is never negative, and i’m freeing the list in the end of the function, adding an edit for that. – Ruslan Ver Aug 02 '22 at 16:04
  • @Gerhardh so for the first message, first allocation what should be the fix, I don't know how much words there will be actually... – Ruslan Ver Aug 02 '22 at 16:05
  • 1
    I do not mean a negative `idx`. But if it is 0 as you check in that `if`, then `idx-1` will be negative. And the pointer is set to `NULL` in the previous line. – Gerhardh Aug 02 '22 at 16:07
  • 1
    If you don't know the number in advance, a) start with a value that is guaranteed to be large enough and shrink afterwards or b) start with some decent/random size and increase if it is not sufficient. `realloc` is your friend. – Gerhardh Aug 02 '22 at 16:09
  • Another issue: You allocate a new `list` within your `while` loop but you only free it once after that loop. All allocations except the last one will be lost and cause memory leaks. – Gerhardh Aug 02 '22 at 16:11
  • Why are you working with a standard that is probably considerably older than you are? You should be using C11 (C18 even, though they're essentially the same); you should not be use a relic from the last millennium. Using C90 will teach you bad habits. It is not as stringent as modern C. – Jonathan Leffler Aug 02 '22 at 16:17
  • @Gerhardh I have a function that freeing it, I didn't wanted to make this question too long but ok, added an edit – Ruslan Ver Aug 02 '22 at 16:18
  • `list[i] = NULL; free(list[i]);` You constantly tend to setting pointers to `NULL` before you use them. – Gerhardh Aug 02 '22 at 16:18
  • @JonathanLeffler Tell that to my University hahaha – Ruslan Ver Aug 02 '22 at 16:19
  • 1
    The line `strncpy(list[*idx], token, strlen(token));` ensures that the byte arrays are not null terminated. You should be using `strcpy()` after ensuring you've allocated enough space (which you have). Using `malloc()` does not ensure that the memory is zero-initialized — using `calloc()` would, but is unnecessary if you use `strcpy()`. – Jonathan Leffler Aug 02 '22 at 16:19
  • 1
    Time to worry about the standard of education they are offering you. They should have updated their courses over the last 5 years; they should not still be teaching the material from yesteryear. – Jonathan Leffler Aug 02 '22 at 16:20
  • When Jonathan Leffler wrote "last 5 years", he probably actually meant "20 years". Already C99 introduced lots of improvements. Not adapting to that version (or a newer one) during last 22 years should be treated as refusing to do their job and what they are paid for. – Gerhardh Aug 02 '22 at 17:01

1 Answers1

1

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;
}
Gerhardh
  • 11,688
  • 4
  • 17
  • 39