0

I'm getting back into C after a long break, and as always the problems lie in use of pointers and general memory management.

My specific problem arises from the use of realloc on a struct in order to dynamically grow it as an array as required. I've never really worked with struct arrays before so I do apologise in advance.

Note: I haven't gotten as far as adding any data to the struct data fields as of yet. I have also renamed all variables to generic names for the purpose of this example.

Here's my struct definition:

typedef struct a_struct
{
    char a;
    char b;
    int num;
} TheStruct;

I allocate the first struct via calloc:

TheStruct *new_structInstance(void)
{
    TheStruct *structInstance = NULL;

    structInstance = calloc(1, sizeof(TheStruct));

    return structInstance;
}

This is my attempt at reallocating the first struct into an array of them, which is where I'm almost certainly going wrong:

void add_structInstance_to_array(TheStruct *structInstance, int *numElements, 
int *bufferLen)
{
    if (*numElements == *bufferLen) {
        *bufferLen += GROWBY; // #define 16
        TheStruct *newstructInstance = realloc(structInstance, 
*bufferLen * sizeof(structInstance));
        if (newstructInstance == NULL) {
            free(structInstance);
            printf("Error: Memory could not be allocated.");
            exit(EXIT_FAILURE);
        }
        else if(structInstance != newstructInstance) {
            structInstance = newstructInstance;
        }
        newstructInstance = NULL;
    }
    *numElements += 1;
}

I free the array of structs thus:

void free_struct_array(TheStruct *structInstance, int *numElements)
{
    for(int i = *numElements-1; i >= 0; i--) {
        free(&structInstance[i]);
    }
    free(structInstance);
}

Note that the input text file input format read by fgets in main approximately:


char char int

And I drive the whole thing in main:

void main()
{
    TheStruct *structInstance = new_structInstance();
    int initialCreated = 0;

    int bufferLen = 0, numElements = 0;

    char buffer[BUFFER]; // #define 20

    FILE *file = fopen("input.txt", "r");

    while(fgets(buffer, sizeof(buffer), file) != NULL) {
        if(!initialCreated){
            initialCreated = 1;
        }
        else {
            add_structInstance_to_array(structInstance, &numElements, &bufferLen);
        }
    }

    // Free memory
    fclose(file);
    free_struct_array(structInstance, &numElements);
}

Compiling the code generates the following terminal output:

free(): double free detected in tcache 2 Aborted (core dumped)

Valgrind outputs the following (I've skipped more verbose output as it includes my filesystem folder structure):

==102002== LEAK SUMMARY:
==102002==    definitely lost: 128 bytes in 1 blocks
==102002==    indirectly lost: 0 bytes in 0 blocks
==102002==      possibly lost: 0 bytes in 0 blocks
==102002==    still reachable: 472 bytes in 1 blocks
==102002==         suppressed: 0 bytes in 0 blocks
==102002==
==102002== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

If there is any way I can output more useful information without exposing my file structure, please let me know.

I tried an "easier" version of this example, instead having a single struct and running realloc on an array within the struct, e.g.:

typedef struct a_struct
{
   int *arr;
}

The above worked fine. Modifying the example to use structs as the array elements instead, a bit like a linked list I suppose, is my intention, so that I can access the struct array such as:

a_struct[element].datatype = something;
int getInt = a_struct[element].inttype;

So I suppose the question is, is it possible using my approach above, and if so, where am I going wrong and is there a better approach?

Lawro
  • 3
  • 4

2 Answers2

0

You not only free the individual elements, you also free the first element again.

This code is highly problematic:

void free_struct_array(TheStruct *structInstance, int *numElements)
{
    for(int i = *numElements-1; i >= 0; i--) {
        free(&structInstance[i]);
    }
    free(structInstance);
}

This should be a singular array of structs, it should be one allocation you can just free() in one shot. If it was an array of pointers it'd be TheStruct**.

Remember how you allocated:

structInstance = calloc(1, sizeof(TheStruct));

Your free() calls should pair up exactly with those.

Reminder: Never, ever, call free() on something you did not explicitly allocate1

1 Or that was allocated on your behalf by a malloc() family allocator, as some libraries do.

tadman
  • 208,517
  • 23
  • 234
  • 262
  • Not calling the `free_struct_array` function, or any other call to `free()` doesn't change the error outcome I have, but I do see your point. Regarding the freeing of the struct array, I take it that means a `realloc` on a specific block only ever counts as a single `free()` for the entire block, as opposed to `malloc` or `calloc` which need a `free()` for every single case? – Lawro Dec 21 '22 at 22:59
  • Here's a typical life-cycle: `calloc()`, then `realloc()` as many times as necessary, then `free()` *once and once only*. A reallocation requires a valid pointer and returns a valid pointer (if successful) so there's no need to release memory. That's handled for you as part of the contract. – tadman Dec 21 '22 at 23:58
  • You can't `free()` on arbitrary pointers, you can *only* call `free()` on pointers given to you by tools like `calloc()`, and that *exact pointer* alone. Close enough does not count. Being in the allocation does not count. It must be the allocation's pointer. – tadman Dec 21 '22 at 23:58
  • One of the big challenges when writing C is keeping track of what responsibilities your various functions have when it comes to allocations, and what responsibilities are implied by calling them if they are handing off "ownership" of allocations. That `add_structInstance_to_array` decides to summarily `free()` something is a huge problem, that's outside the scope of that function's responsibility and you will have no idea after calling that function if it has or hasn't assumed ownership of that pointer you just ate. – tadman Dec 22 '22 at 00:00
  • Thanks, I understand the `free()` situation now! Regarding the `free()` in `add_structInstance_to_array`, I only free the new pointer if the memory fails to allocate and exit the program, unless I'm missing something? – Lawro Dec 22 '22 at 00:29
  • If you're allocating trivial amounts of memory (< 1GB) on a modern machine, it's fairly safe to assume the allocation will succeed, especially for non-mission-critical applications. If you're working in a very restrictive embedded environment with extremely limited memory, and where crashing is an unacceptable outcome, you will need to be far, far more careful, but that can be a lot of work. I'd say for cases like this just presume the allocation succeeds and move on with life. If it crashes, it crashes. – tadman Dec 22 '22 at 00:35
  • Even on a low-end piece of hardware like a Raspberry Pi, you still have ~1GB to work with, so tiny programs like this allocating several MB should never reasonably fail. Note that this advice to ignore it does not apply if you're doing things that could conceivably use way too much memory, like allocating a huge matrix, or reading in a file completely into memory, though these are exceptional cases. – tadman Dec 22 '22 at 00:36
  • What I'm saying really is don't worry about failed allocation or reallocation calls. Focus almost entirely on these things: Not leaking memory, not having a double-free, not having use-after free, and not using uninitialized values. That's already a *ton* of stuff to keep track of! Those are things that *will* have severe consequences, not some theoretical allocation failure that'll likely never happen. – tadman Dec 22 '22 at 00:38
  • RIght, but if I remove all of the allocation error checking and any kind of explicit `free()` in the program, I still get the same double free error, despite not freeing anything. – Lawro Dec 22 '22 at 10:10
0

Aha! Thanks to @tadman, I dug deeper into freeing allocated struct pointers inadvertently through going out of scope / not returning the pointer to the struct.

I fixed the problem by modifying my add_structInstance_to_array to return the pointer to the reallocated struct:

TheStruct *add_structInstance_to_array(TheStruct *structInstance, int *numElements, int *bufferLen)
{
    if (*numElements == *bufferLen) {
        *bufferLen += GROWBY; // #define 16
        TheStruct *newstructInstance = realloc(structInstance, 
*bufferLen * sizeof(structInstance));
        if (newstructInstance == NULL) {
            free(structInstance);
            printf("Error: Memory could not be allocated.");
            exit(EXIT_FAILURE);
        }
        else if(structInstance != newstructInstance) {
            structInstance = newstructInstance;
        }
        newstructInstance = NULL;
    }
    *numElements += 1;
    
    return structInstance;
}

To further agree with @tadman, I could also get rid of the free_struct_array function that would loop through individual struct elements, and instead simply free the struct array once in main.

Lawro
  • 3
  • 4