free() on char* recognized as invalid by valgrind

Question

I am attempting to free char* pointers that exist within a char** array, but valgrind is determining this operation as invalid.

Here is a simple example of what I'm doing:

struct building{
  int propertyPrice;
  int totalArea;
  char** floors;
}

int main(){

  struct building* B = malloc(sizeof(struct building));

  for(size_t i=0;i<10;i++)
    B->floors[i] = malloc(20 * sizeof(char*));

}

Here's where I'm confused - I can access the elements with the following loop:

for(size_t i=0;i<10;i++)
    printf("%s", B->floors[i]); //assume that a valid string exists at each floors[i]!

I am attempting to free the allocated memory as follows:

for(size_t i=0;i<10;i++)
    free(B->floors[i]);

free(B);

Please excuse the lack of malloc() check against NULL, I'm cutting that out for brevity.

Valgrind determines that the operation free(B->floors[i]) is Invalid free() / delete / delete[] / realloc(). Yet, if I can access the individual char* elements through B-floors[i], shouldn't I be able to free it using the same syntax by just switching the function call from printf() to free() (I got the idea from this stackoverflow answer)?

The second free() call works perfectly fine.

My program is executing as expected, but I'd like to get rid of memory leaks - hence the usage of valgrind. If it's helpful, I'm using the -Werror -ggdb switches for gcc and the --track-origins=yes --leak-check=full arguments when executing valgrind.

`B->floors` is a pointer, but you never make it point anywhere. You need e.g. `B->floors = malloc(10 * sizeof *B->floors)` before the loop. And of course `free(B->floors)` once you're done with it (*after* the loop to free each element). — Some programmer dude, Aug 04 '23 at 15:57
It is also possible to define `floors` as a *flexible array member* `char *floors[];` and extend the allocation of `B` to incorporate the required number of elements: `struct building *B = malloc(sizeof(*B) + 10 * sizeof(B->floors[0]));`. — Ian Abbott, Aug 04 '23 at 16:42

David Ranieri · Accepted Answer · 2023-08-04T17:11:53.440

5

Here:

for(size_t i=0;i<10;i++)
    B->floors[i] = malloc(20 * sizeof(char*));

B->floors is used uninitialized.

You need to reserve space for 10 elements:

B->floors = malloc(10 * sizeof(char*));

then you can fill the jagged array:

for (int i = 0; i < 10; i++) {
    B->floors[i] = strdup(...); // or malloc + memcpy if strdup is not available
}

finally:

for (int i = 0; i < 10; i++) {
    free(B->floors[i]);
}
free(B->floors);

Since floors is the last member of the array you can use it as a "flexible array member":

struct building* B = malloc(sizeof(struct building) + (sizeof(char *) * 10));

in this case you don't call free(B->floors); at the end.

edited Aug 04 '23 at 17:11

answered Aug 04 '23 at 16:02

David Ranieri

39,972
7
52
94

This would just get the OP an uninitialized array of 20 `char *` pointers. I suspect that's not all of what's needed, but the question is incomplete... – Andrew Henle Aug 04 '23 at 16:18
While that took care of the heap allocation, if I don't initialize the embedded char* pointers now valgrind complains that `Conditional jump or move depends on uninitialised value(s)` and that this `Uninitialised value was created by a heap allocation`. This is why I initially used the malloc loop and set the embedded char* pointers to a default value. – smellyourbooks Aug 04 '23 at 16:25
@AndrewHenle each pointer needs to point to some valid address on the stack or reserved with `strdup` or `malloc` + `memcpy` of course, I assume OP knows that but edited anyway. – David Ranieri Aug 04 '23 at 16:29
@DavidRanieri why on the stack? – 0___________ Aug 04 '23 at 16:45
@DavidRanieri: Apparently, if I just use calloc() instead of malloc() the errors disappear. However, just to test that my understanding is correct, this is because calloc() would initialize the char* pointers to NULL, yes? And that this is a secure way to handle this situation? – smellyourbooks Aug 04 '23 at 16:47
@0___________ why not? `char str[][4] = {"abc", "def", ...}; ... B->floors[i] = str[i];` is an option. – David Ranieri Aug 04 '23 at 16:55
@smellyourbooks using `calloc` doesn't fix the problem, you still don't have space for 10 elements in the jagged array. – David Ranieri Aug 04 '23 at 16:58
@DavidRanieri Tried to do it the `malloc() + memcpy()` way, ended up getting a SIGSEV. For reference: ```for(size_t i=0;i<17;i++){ B->floors[i] = malloc(32 * sizeof(char)); memcpy(B->floors[i], "INITIALIZATION_VALUE", 17); } ``` And then free via the loop route mentioned above. – smellyourbooks Aug 04 '23 at 17:27
@DavidRanieri no, as ypu cant `free` as in the question. – 0___________ Aug 04 '23 at 17:29
@smellyourbooks "INITIALIZATION_VALUE" doesn't fit in 17 bytes, you need 20 bytes + 1 for the trailing NUL. – David Ranieri Aug 04 '23 at 17:30
After fixing the 21 bytes error, valgrind is again saying that the loop statement `free(B->floors[i])` is `Invalid free() / delete / delete[] / realloc()`! (The previous errors have disappeared completely, thank you.) – smellyourbooks Aug 04 '23 at 17:41
@0___________ are you aware that I was answering to a comment? _I suspect that's not all of what's needed_. I mean each pointer in a jagged array must point to a valid address regardless of the logic of the question. – David Ranieri Aug 04 '23 at 17:51
1

I'd just like conclude that this answer was perfect. Turns out, I was assigning a char* pointer values via the `=` operator when I should I have used `memcpy` within an internal function that was causing memory issues. Thank you @DavidRanieri for all your help! – smellyourbooks Aug 04 '23 at 19:39
@smellyourbooks you are welcome :) – David Ranieri Aug 04 '23 at 19:59

free() on char* recognized as invalid by valgrind

1 Answers1