Why my struct created in function gets overwritten?

Question

How can I create structs in a function? I'm trying this way:

#include <string.h>
#include <stdio.h>

typedef struct { char name[10]; } Item;

typedef struct { Item *item; } MyStruct;

MyStruct make(char *name) {
    MyStruct st;

    Item item;
    strcpy(item.name, name);
    st.item = &item;
    printf("name: %s\n", st.item->name);

    return st;
}

int main() {
    MyStruct s1, s2;
    s1 = make("hi");
    s2 = make("hey");

    printf("\nname: %s\n", s1.item->name);
    printf("name: %s\n", s2.item->name);
}

The output is:

name: hi
name: hey

name: hey
name: hey

The struct s1 is being overwritten when I create s2. It looks that the address of the struct st on the function make is the same every time the function is called. Is it correct? I would like to avoid using malloc if possible.

Edit: The result is different on macOS and Linux, the output above is from Linux, below is from macOS. Why is it different?

name: hi
name: hey

name: p
name: p

Answer 1

The MyStruct st; is destroyed as soon as the function returns, so that's undefined behavior, so you'll have to use malloc and return a pointer to MyStruct.

Actually, returning structs is perfectly fine, and the compiler will make sure that a copy of the original data is returned, so no data is lost. But the problem is, the copy is shallow, so, for example, pointers are copied literally (the very address the original pointer points to is copied, but not the data), which means that any pointer inside the newly copied struct is potentially invalid, because the memory location it used to point to may have been deallocated while destroying the stack frame on function return.

Your function uses a struct with a pointer inside, and this pointer points to a local variable. The latter is (as all local variables) destroyed on function exit, and while a copy of the original struct is returned, the pointer now points to a memory address that's (probably) no longer controlled by your program.

That's why your data gets overwritten on the second call. So, if you want to preserve Item item, you should allocate it, and not the return value of your function (although you can do this too, of course, but this won't solve the problem since item will be destroyed anyway), on the heap with malloc.

Answer 2

When you declare Item item; you're allocating that locally and it falls out of scope. The next function call coincidentally recycles that same memory position.

This is undefined behaviour as you need to dynamically allocate that if it's to persist outside that scope.

Answer 3

Your problem is misunderstanding pointers. Let's start with some very similar code that would work.

#include <string.h>
#include <stdio.h>

typedef struct { char name[10]; } Item;

typedef struct { Item item; } MyStruct; // <--- Removed the *

MyStruct make(char *name) {
    MyStruct st;

    Item item;
    strncpy(item.name, name, 10);  // <--- strncpy because name[] is a fixed length
    st.item = item;                // <-- Removed the &; *copies* item into st
    printf("name: %s\n", st.item.name); // <-- . rather than ->

    return st;
}

int main() {
    MyStruct s1, s2;
    s1 = make("hi");
    s2 = make("hey");

    printf("\nname: %s\n", s1.item.name); // <-- . rather than ->
    printf("name: %s\n", s2.item.name);   // <-- . rather than ->
}

In this code I got rid of the pointers and everything should work. Item is 10 chars, and MyStruct is also 10 chars long. When you return it, those 10 chars of memory are copied, just like an int.

But you didn't do this; you added pointers. Since item is a local variable, it's in the current stack frame, not the heap. The stack frame disappears at the end of the current scope. (But it doesn't really disappear. It's just undefined behavior to reference it out of scope, so it can return anything.)

Doing it without pointers means more copying (though actually not that much more copying; on a 64-bit system, a pointer is 8 bytes). It also means changes to one copy don't impact the others. That's either good or bad depending on your problem. Getting rid of pointers this way can actually be a very good design as long as the struct doesn't get too big. Once you add pointers, you're going to need to manage memory and decide about ownership and when to free the memory and all the other headaches. It depends on your problem which approach is best.

As a note, if you go this way, there's no reason to create an intermediate item (and it's inefficient to do so because it's making two copies of the string). Instead you'd just copy directly into st:

MyStruct make(char *name) {
    MyStruct st;

    strncpy(st.item.name, name, 10);
    printf("name: %s\n", st.item.name);

    return st;
}