C: Memory allocation issues

Question

I'm having an issue with (I think) memory reallocation in C. The program is meant to run such that when fopen(array, &num); is called, it will first retrieve the number of elements in the array from file and place that in num, reallocate memory for the array pointer given to give it enough room to store the contents of the file proper, then copy the values over into that array. This seems to work while still in the fopen function (shown by 'mark 1'), but does not work outside of this (shown by 'mark 2') instead seeming to spew out random memory garbage. Any help appreciated (both with code and formatting my poorly laid out question).

//main.c
void Rtest(){

  char num;
  struct individual *array;
  array = (struct individual *) malloc(sizeof(struct individual));

  openf(array, &num);

  printf("%d\n", num);

  for (int i = 0; i < num; i++) {printf("%s\n", array[i].name);} //mark 2

  free(array);

}

//fil.h
struct individual {
    char name[32];
    char stats[7];
    char role;
    char roles[13];
};

void openf(struct individual *array, char *num){

  FILE *fp;

  fp = fopen("save.bin", "rb");
  fread(num, 1, sizeof(char), fp);
  array = (struct individual *)realloc(array, *num * sizeof(struct individual));
  printf("%d\n", sizeof(*array));
  fread(array, *num, sizeof(struct individual), fp);
  for (int i = 0; i < *num; i++) {printf("%s\n", array[i].name);} //mark 1

  fclose(fp);

}

File contents:

03 43 61 72 6C 73 6F 6E 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 02 03 04 05 06 08 00 01 02 03 04 05 06 07 08 09 0A 0B 0C 43 61 72 6C 73 6F 6E 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 02 03 04 05 06 08 00 01 02 03 04 05 06 07 08 09 0A 0B 0C 43 61 72 6C 73 6F 6E 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 02 03 04 05 06 08 00 01 02 03 04 05 06 07 08 09 0A 0B 0C

In `openf`, the code `array = (struct individual *)realloc....` means nothing to the *caller* of `openf`, except possibly (and in fact, likely) dangling the pointer value that was passed in to this function. The original value of `array` in caller `Rtest` remains, but is now no longer valid; hence "dangling". Btw, why is `num` a `char` ? I mean, it can work, but is pretty limiting. And I would also consider you stop assuming all of your IO just works. It's a terrible practice. — WhozCraig, Jan 30 '21 at 18:54
@WhozCraig num is a char because I'm just a little bit daft. So would the solution be to break this up into two parts, reallocating memory within Rtest? — JohnDent, Jan 30 '21 at 18:57
If the intent is that `openf` can possibly change the value of that pointer, and you want that change reflected back to the caller, you devise a way to do so. Using the otherwise-unused return value of the function, or passing the original pointer by address (a pointer-to-pointer, and dereferenced therein within `openf` when changing the pointed-to pointer), are both common solutions to that scenario. — WhozCraig, Jan 30 '21 at 18:58

Gal Birka · Accepted Answer · 2021-01-30T22:03:57.697

3

When you want to change the argument inside a function, you pass a pointer to it. For example, inside Rtest you declared a char called num. It has no value, and you sent it to openf, But you actually sent the pointer to num since you wanted to change its value, you did it correctly and indeed openf changed num value successfully.

But how about array? Well, you declared it on Rtest and allocated space in memory for it, which is all correct. Then, you wanted to send it to Rtest as a pointer so the function could change it. array is a variable of the type "pointer to struct individual". This is okay, but if you wanted to change it inside Rtest, well you need to send a pointer for that variables.. hence, you needed a "POINTER TO pointer to struct individual". Note that the variable name was copied from before and I just added "POINTER TO"

I'm sure you know what pointer to pointer is, and what you needed to do is use:

openf(&array, &num);

And of course modift openf as well so it will use the new "pointer to pointer", something like that:

void openf(struct individual **array, char *num){

  FILE *fp;

  fp = fopen("save.bin", "rb");
  fread(num, 1, sizeof(char), fp);
  *array = (struct individual **)realloc(*array, *num * sizeof(struct individual));
  printf("%d\n", sizeof(**array));
  fread(*array, *num, sizeof(struct individual), fp);
  for (int i = 0; i < *num; i++) {printf("%s\n", (*array)[i].name);} //mark 1

  fclose(fp);

}

When I run this code on my machine, along with Rtest and provided save.bin I get the following output:

53
Carlson
Carlson
Carlson
3
Carlson
Carlson
Carlson

EDIT: As @WhozCraig mentioned in the comments, You could use the unused return value for the function and return the pointer for the "new" array, which might be a slightly better way of doing things here instead of the "pointer to pointer" stuff, but its up to you.

edited Jan 30 '21 at 22:03

answered Jan 30 '21 at 19:06

Gal Birka

581
6
16

1

I tihnk there's a bug here, shouldn't this be `*array = (struct individual **)realloc(...` ? Yu want to change the data at the memory address , `array`, eg `*array`, not the memory location itself which is `array`. Right? – erik258 Jan 30 '21 at 19:20
The solution you've provided does not seem to work. After copying in this code, it no longer prints at mark 1 and instead crashes out. – JohnDent Jan 30 '21 at 19:59
@DanielFarrell Yes you are absolutely correct. I edited my original post, thank you. John you can try copy the code again after I changed what Daniel spotted. – Gal Birka Jan 30 '21 at 20:44
@GalBirka I'm very sorry, but I'm still crashing out. Does this code work with your machine, because I'm beginning to suspect this may be an issue with my setup. – JohnDent Jan 30 '21 at 20:56
I can't run the code on my machine, since I don't have the binary file. I noticed another problem with malloc function so you can first try to copy paste my original code and try. If it does not work, you can try give some details like what errors you get or in which line it crashes. Lastly, you can try upload your binary file and link it here, and I will take a look in my machine. – Gal Birka Jan 30 '21 at 21:19
Consider the cleaner `*array = realloc(*array, sizeof **array * *num);` and `fread(*array, *num, sizeof **array, fp);` – chux - Reinstate Monica Jan 30 '21 at 21:28
Given `*num` might be outside the 0 to 127 range, safer code would test for that range or use as if the value was an `unsigned char`. – chux - Reinstate Monica Jan 30 '21 at 21:31
The bin file may be obtained here: https://www.mediafire.com/file/cegrya2dbxbngz5/save.bin/file – JohnDent Jan 30 '21 at 21:46
@chux-ReinstateMonica In my opinion sizeof(**array) is not cleaner than sizeof(struct individual), I think the latter one makes the code more readable. I do agree with your num comment, and overtall there isn't much checking in this code, which totally should be. About the binary file, I modified the code in my solution a bit and added the output I get on my machine. If its not good, please provide the expected output. – Gal Birka Jan 30 '21 at 22:06
@GalBirka That is the expected output. After reentering your code this now works as intended. Many thanks! – JohnDent Jan 30 '21 at 22:19
if you're going to realloc the array anyway, why not omit passing one in the first place, and instead return a newly `calloc`ed one? – erik258 Jan 30 '21 at 22:19
@GalBirka Note: your code is here is `(struct individual **)realloc(*array, *num * sizeof(struct individual));`, not only `sizeof(**array)`. Consider your whole line including the [unneeded cast](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). Using the type, rather than `*pointer` obliges more review work and harder to maintain. The `()` are not needed with `*pointer`, hence the `sizeof **array` and not the `sizeof(**array)` as you commented. – chux - Reinstate Monica Jan 31 '21 at 01:43

C: Memory allocation issues

1 Answers1