Dynamic allocation of array of strings fails. realloc() error

Question

I am trying to write a simple program that will read words from a file and print the number of occurrences of a particular word passed to it as argument.

For that, I use fscanf to read the words and copy them into an array of strings that is dynamically allocated.

For some reason, I get an error message.

Here is the code for the readFile function:

void readFile(char** buffer, char** argv){
    unsigned int i=0;
    FILE* file;
    file = fopen(argv[1], "r");
    do{
        buffer = realloc(buffer, sizeof(char*));
        buffer[i] = malloc(46);
    }while(fscanf(file, "%s", buffer[i++]));
    fclose(file);
}

And here is the main function :

int main(int argc, char** argv){
    char** buffer = NULL;
    readFile(buffer, argv);
    printf("%s\n", buffer[0]);
    return 0;
} 

I get the following error message :

realloc(): invalid next size
Aborted (core dumped)

I have looked at other threads on this topic but none of them seem to be of help. I could not apply whatever I learned there to my problem.

I used a debugger (VS Code with gdb). Data is written successfully into indices 0,1,2,3 of the buffer array but says error : Cannot access memory at address 0xfbad2488 for index 4 and pauses on exception.

Another thread on this topic suggests there might be a wild pointer somewhere. But I don't see one anywhere.

I have spent days trying to figure this out. Any help will be greatly appreciated.

Thanks.

Answer 1

Your algorithm is wrong on many fronts, including:

• buffer is passed by-value. Any modifications where buffer = ... is the assignment will mean nothing to the caller. In C, arguments are always pass-by-value (arrays included, but their "value" is a conversion to temporary pointer to first element, so you get a by-ref synonym there whether you want it or not).

• Your realloc usage is wrong. It should be expanding based on the iteration of the loop as a count multiplied by the size of a char *. You only have the latter, with no count multiplier. Therefore, you never allocate more than a single char * with that realloc call.

• Your loop termination condition is wrong. Your fscanf call should check for the expected number of arguments to be processed, which in your case is 1. Instead, you're looking for any non-zero value, which EOF is going to be when you hit it. Therefore, the loop never terminates.

• Your fscanf call is not protected from buffer overflow : You're allocating a static-sized string for each string read, but not limiting the %s format to the static size specified. This is a recipe for buffer-overflow.

• No IO functions are ever checked for success/failure : The following APIs could fail, yet you never check that possibility: fopen, fscanf, realloc, malloc. In failing to do so, you're violating Henry Spencer's 6th Commandment for C Programmers : "If a function be advertised to return an error code in the event of difficulties, thou shalt check for that code, yea, even though the checks triple the size of thy code and produce aches in thy typing fingers, for if thou thinkest ``it cannot happen to me'', the gods shall surely punish thee for thy arrogance."

• No mechanism for communicating the allocated string count to the caller : The caller of this function is expecting a resulting char**. Assuming you fix the first item in this list, you still have not provided the caller any means of knowing how long that pointer sequence is when readFile returns. An out-parameter and/or a formal structure is a possible solution to this. Or perhaps a terminating NULL pointer to indicate the list is finished.

• (Moderate) You never check argc : Instead, you just send argv directly to readFile, and assume the file name will be at argv[1] and always be valid. Don't do that. readFile should take either a FILE* or a single const char * file name, and act accordingly. It would be considerably more robust.

• (Minor) : Extra allocation : Even fixing the above items, you'll still leave one extra buffer allocation in your sequence; the one that failed to read. Not that it matter much in this case, as the caller has no idea how many strings were allocated in the first place (see previous item).

---

Shoring up all of the above would require a basic rewrite of nearly everything you have posted. In the end, the code would look so different, it's almost not worth trying to salvage what is here. Instead, look at what you have done, look at this list, and see where things went wrong. There's plenty to choose from.

Sample

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

#define STR_MAX_LEN     46

char ** readFile(const char *fname)
{
    char **strs = NULL;
    int len = 0;

    FILE *fp = fopen(fname, "r");
    if (fp != NULL)
    {
        do
        {
            // array expansion
            void *tmp = realloc(strs, (len+1) * sizeof *strs);
            if (tmp == NULL)
            {
                // failed. cleanup prior success
                perror("Failed to expand pointer array");
                for (int i=0; i<len; ++i)
                    free(strs[i]);
                free(strs);
                strs = NULL;
                break;
            }

            // allocation was good; save off new pointer
            strs = tmp;
            strs[len] = malloc( STR_MAX_LEN );
            if (strs[len] == NULL)
            {
                // failed. cleanup prior sucess
                perror("Failed to allocate string buffer");
                for (int i=0; i<len; ++i)
                    free(strs[i]);
                free(strs);
                strs = NULL;
                break;
            }

            if (fscanf(fp, "%45s", strs[len]) == 1)
            {
                ++len;
            }
            else
            {
                // read failed. we're leaving regardless. the last
                //  allocation is thrown out, but we terminate the list
                //  with a NULL to indicate end-of-list to the caller
                free(strs[len]);
                strs[len] = NULL;
                break;
            }

        } while (1);

        fclose(fp);
    }

    return strs;
}


int main(int argc, char *argv[])
{
    if (argc < 2)
        exit(EXIT_FAILURE);

    char **strs = readFile(argv[1]);
    if (strs)
    {
        // enumerate and free in the same loop
        for (char **pp = strs; *pp; ++pp)
        {
            puts(*pp);
            free(*pp);
        }

        // free the now-defunct pointer array
        free(strs);
    }

    return EXIT_SUCCESS;
}

Output (run against /usr/share/dict/words)

A
a
aa
aal
aalii
aam
Aani
aardvark
aardwolf
Aaron
Aaronic
Aaronical
Aaronite
Aaronitic
Aaru
Ab
aba
Ababdeh
Ababua
abac
abaca
......
zymotechny
zymotic
zymotically
zymotize
zymotoxic
zymurgy
Zyrenian
Zyrian
Zyryan
zythem
Zythia
zythum
Zyzomys
Zyzzogeton

---

Improvements

The secondary malloc in this code is completely pointless. You're using a fixed length word maximum size, so you could easily retool you array to be a pointer to use this:

char (*strs)[STR_MAX_LEN]

and simply eliminate the per-string malloc code entirely. That does leave the problem of how to tell the caller how many strings were allocated. In the prior version we used a NULL pointer to indicate end-of-list. In this version we can simply use a zero-length string. Doing this makes the declaration of readFile rather odd looking, but for returning a pointer-to-array-of-size-N, its' correct. See below:

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

#define STR_MAX_LEN     46

char (*readFile(const char *fname))[STR_MAX_LEN]
{
    char (*strs)[STR_MAX_LEN] = NULL;
    int len = 0;

    FILE *fp = fopen(fname, "r");
    if (fp != NULL)
    {
        do
        {
            // array expansion
            void *tmp = realloc(strs, (len+1) * sizeof *strs);
            if (tmp == NULL)
            {
                // failed. cleanup prior success
                perror("Failed to expand pointer array");
                free(strs);
                strs = NULL;
                break;
            }

            // allocation was good; save off new pointer
            strs = tmp;

            if (fscanf(fp, "%45s", strs[len]) == 1)
            {
                ++len;
            }
            else
            {
                // read failed. make the final string zero-length
                strs[len][0] = 0;
                break;
            }

        } while (1);

        fclose(fp);
    }

    return strs;
}


int main(int argc, char *argv[])
{
    if (argc < 2)
        exit(EXIT_FAILURE);

    char (*strs)[STR_MAX_LEN] = readFile(argv[1]);
    if (strs)
    {
        // enumerate and free in the same loop
        for (char (*s)[STR_MAX_LEN] = strs; (*s)[0]; ++s)
            puts(*s);
        free(strs);
    }

    return EXIT_SUCCESS;
}

The output is the same as before.

---

Another Improvement: Geometric Growth

With a few simple changes we can significantly cut down on the realloc invokes (we're currently doing one per string added) by only doing them in a double-size growth pattern. If each time we reallocate, we double the size of the prior allocation, we will make more and more space available for reading larger numbers of strings before the next allocation:

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

#define STR_MAX_LEN     46

char (*readFile(const char *fname))[STR_MAX_LEN]
{
    char (*strs)[STR_MAX_LEN] = NULL;
    int len = 0;
    int capacity = 0;

    FILE *fp = fopen(fname, "r");
    if (fp != NULL)
    {
        do
        {
            if (len == capacity)
            {
                printf("Expanding capacity to %d\n", (2 * capacity + 1));

                void *tmp = realloc(strs, (2 * capacity + 1) * sizeof *strs);
                if (tmp == NULL)
                {
                    // failed. cleanup prior success
                    perror("Failed to expand string array");
                    free(strs);
                    strs = NULL;
                    break;
                }

                // save the new string pointer and capacity
                strs = tmp;
                capacity = 2 * capacity + 1;
            }

            if (fscanf(fp, "%45s", strs[len]) == 1)
            {
                ++len;
            }
            else
            {
                // read failed. make the final string zero-length
                strs[len][0] = 0;
                break;
            }

        } while (1);

        // shrink if needed. remember to retain the final empty string
        if (strs && (len+1) < capacity)
        {
            printf("Shrinking capacity to %d\n", len);
            void *tmp = realloc(strs, (len+1) * sizeof *strs);
            if (tmp)
                strs = tmp;
        }

        fclose(fp);
    }

    return strs;
}


int main(int argc, char *argv[])
{
    if (argc < 2)
        exit(EXIT_FAILURE);

    char (*strs)[STR_MAX_LEN] = readFile(argv[1]);
    if (strs)
    {
        // enumerate and free in the same loop
        for (char (*s)[STR_MAX_LEN] = strs; (*s)[0]; ++s)
            puts(*s);

        // free the now-defunct pointer array
        free(strs);
    }

    return EXIT_SUCCESS;
}

Output

The output is the same as before, but I added instrumentation to show when expansion happens to illustrate the expansions and final shrinking. I'll leave out the rest of the output (which is over 200k lines of words)

Expanding capacity to 1
Expanding capacity to 3
Expanding capacity to 7
Expanding capacity to 15
Expanding capacity to 31
Expanding capacity to 63
Expanding capacity to 127
Expanding capacity to 255
Expanding capacity to 511
Expanding capacity to 1023
Expanding capacity to 2047
Expanding capacity to 4095
Expanding capacity to 8191
Expanding capacity to 16383
Expanding capacity to 32767
Expanding capacity to 65535
Expanding capacity to 131071
Expanding capacity to 262143
Shrinking capacity to 235886