2

I am attempting to create a structure (2nd) in a structure (1st) with dynamic memory allocation. For the sequence of steps below (case 1), where I call malloc for a pointer to the 2nd structure before calling realloc for the 1st structure, I see garbage when printing the value of the member (str[0].a1) in the first structure. If I place the same realloc code before malloc (case 2), there are no issues.

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

typedef struct string2_t {
    int a2;
} string2;

typedef struct string1_t {
    int a1;
    string2 * b;
} string1;


string1 * str = NULL;
int size = 0;

string1 test(int val){

    // case 2
    //str = (string1 *) realloc(str, size*sizeof(string1));

    string1 S;
    S.a1 = val;
    size += 1;
    S.b = (string2 *) malloc(2*sizeof(string2));
    S.b[0].a2 = 11;
    S.b[1].a2 = 12;

    // case1
    str = (string1 *) realloc(str, size*sizeof(string1));
    
    return S;
}

int main() {
    size += 1;
    str = (string1 *) malloc(size*sizeof(string1));
    str[0] = test(10);
    printf("value of a:%d\n",str[0].a1);
    str[1] = test(20);
    printf("value of a:%d\n",str[0].a1);
    return(0);
}

I can see that memory location changes for case 1, but I don't quite understand why doesn't pointer point to the right contents (str[0].a1 shows junk). I haven't returned the address of 1st structure before realloc, so I feel it should have worked. Could someone explain why it's pointing to garbage?

niil87
  • 47
  • 5
  • You shouldn't increment `size` in both `main()` and `test()`. – Barmar Dec 21 '22 at 23:25
  • 1
    [don't cast malloc](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) – Barmar Dec 21 '22 at 23:27
  • `printf("Address of S:%p\n",S);`? Compiler says: "warning: format '%p' expects argument of type 'void *', but argument 2 has type 'string1' {aka 'struct string1_t'} [-Wformat=]". Did you mean: `printf( "Address of S:%p\n", (void *) &S );` ? Oh, and that is the address of a local variable that is destroyed at fcn exit. – Avi Berger Dec 21 '22 at 23:28
  • 1
    I think you may have undefined behavior due to reassigning `str` inside `test()`, and then using that function call when assigning to `str[0]`. And even if not, it's really confusing that you manage `str` allocation in both `main()` and `test()`. – Barmar Dec 21 '22 at 23:31
  • I think the UB is from passing a non-pointer to `printf` when the format specifier expects a pointer. After that, all bets are off. – Adrian Mole Dec 21 '22 at 23:39
  • AFAICT, there is more of an issue here. It seems to be because of test() having the hidden side effect of (potentially) changing the value of str via realloc and the return value being placed according to that address/value. It is not just the bad printf. – Avi Berger Dec 22 '22 at 00:00
  • Have reproduced with both clang and gcc without the mallocs of string2 structs. – Avi Berger Dec 22 '22 at 00:18
  • AFAICT in `str[0] = test(10);` the evaluation of the address to copy the result of the fcn is not specified as sequenced before or after the call to test(). Since test() can have the side effect of changing that address, the return value can be placed in a very wrong (and no longer legal to access) place. – Avi Berger Dec 22 '22 at 00:54
  • niil87, Tip: simplfiy `str = (string1 *) malloc(size*sizeof(string1));` --> `str = malloc(sizeof *str * size);`. – chux - Reinstate Monica Dec 22 '22 at 03:45
  • string1 S is an auto variable, it's scope is only inside the test() function. It's address can't be returned to main() because that memory is taken from stack frame of test() function, and when test() returns, it's frame popped from the stack and that memory address become invalid. – Ajith C Narayanan Dec 22 '22 at 12:21
  • @Barmar / chux - Reinstate Monica unfortunately, I need to port this code to Arduino which uses a C++ compiler and hence forced to use casting. – niil87 Dec 22 '22 at 14:44
  • 1
    @AjithCNarayanan True, but not relevant here. string1 S is a struct that is being returned by value, so there is no attempt to return its address. The issue here is that realloc can change the address of the memory block and there is apparently no guaranteed ordering between main evaluating the str based address to place the result and test() calling realloc on str in test. A simple fix would be to assign the result of test to a local in main and then assign that value to the right position in the str pointed to array. Other approach would be to move the realloc of str from test to main. – Avi Berger Dec 22 '22 at 15:58

1 Answers1

1

As has been stated by several in the comments, lines like this cause undefined behavior:

str[0] = test(10);

The test() function reallocates str, and this is not sequenced with retrieving the address of str[0] as the location to store the result. It's very likely to be compiled as if it had been written

string1 *temp = str;
temp[0] = test(10);

temp caches the old address of str from before the realloc(), and this is invalid after the function returns.

Change your code to:

string1 temp = test(10);
str[0] = temp;

Better design would be to reorganize the code so that all maintenance of the str array is done in one place. test() should be a black box that just allocates the string1 object, while main() allocates and reallocates str when assigning to it.

Barmar
  • 741,623
  • 53
  • 500
  • 612