0

i'm obviously not blaming printf here, i probably messed up my mem allocations and access but i can't understand where i did wrong. the program crash on the second printf in main. It also crash on the third if i comment the second one. Actually it crash whenever i access p after the first printf !

Can someone explains me what i am doing wrong ?

Thanks a lot.

typedef struct 
{
    char * firstname;
    char * lastname;
    int age;
} person;

person * new_person(char * firstname, char * lastname, int age)
{
    person p;
    int lf = strlen(firstname);
    int ll = strlen(lastname);
    p.firstname = (char *)malloc(++lf * sizeof(char));
    p.lastname = (char *)malloc(++ll * sizeof(char));
    strcpy(p.firstname, firstname);
    strcpy(p.lastname, lastname);
    p.age = age;

    return &p;
}    

int main()
{
    person * p = new_person("firstname", "last", 28);

    printf("nom : %s ; prenom : %s ; age : %d\n", p->lastname, p->firstname, p->age);
    printf("nom : %s ; prenom : %s ; age : %d\n", p->lastname, p->firstname, p->age);

    printf("nom : %s ; prenom : %s ; age : %d\n", (*p).lastname, (*p).firstname,(*p).age);

    return 0;
}
mytodayquestionis
  • 47
  • 5
  • 7
    You're returning a pointer to a local variable. – jrok Sep 06 '12 at 19:50
  • 2
    C++, but the explanation stands: http://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope – chris Sep 06 '12 at 19:50
  • 2
    Try turning on compiler warnings. You should be getting `warning C4172: returning address of local variable` or `warning: function returns address of local variable` (or something equivalent, depending on what compiler you're using). – Raymond Chen Sep 06 '12 at 19:52
  • Also, mallocing and strcpy'ing should be punished on platforms where strdup is implemented. –  Sep 06 '12 at 19:56

4 Answers4

5

You're returning the address of a local variable.

You can either modify your new_person to take an argument (a pointer to a person) or you can malloc one inside the function and manipulate that.

The person you declared in your function goes out of scope when the function returns. Everything that happens to it after that is undefined. It might coincidentally keep its value for a while, but you should not depend on this. When you make your call to printf, the stack grows and overwrites the old location of your person with new stuff.

Wug
  • 12,956
  • 4
  • 34
  • 54
3

I think the issue is in this line:

return &p;

Notice that you are returning a pointer to a local variable. This results in undefined behavior, since as soon as the function returns, the local variable p no longer exists. As a result, reading or writing that pointer will read or write garbage data.

The fact that this doesn't immediately crash is an artifact of how the compiler generates code. Chances are, the first time you call printf, it reuses space that was previously used for p in a way that, by sheer coincidence, works out fine. However, after the function returns, its stack frame has clobbered the old memory for p. As a result, the second call to printf is reading garbage data left behind from the call to printf, hence the crash.

(Specifically: when you pass the parameters, it copies the pointers to the strings onto the stack, so when printf runs, it's probably trashing the original pointers, but using the copies. The second call then loads garbage pointers from the expired printf stack frame, since it lives at the same address that p used to live in.)

To fix this, consider changing p to a pointer to a person and then using malloc to allocate it. That way, the memory persists beyond the function call, so this crash should go away.

Hope this helps!

templatetypedef
  • 362,284
  • 104
  • 897
  • 1,065
  • You might mention specifically that until the printf call is made, the value of the dangling person is unchanged. The arguments for printf are assembled before the call is made, and since the arguments are all pointers to char arrays somewhere else in memory, the operation of printf does not destroy those arrays. However, as printf is working, it clobbers the person itself, so the second attempt to load the pointers from the original location of the person returns garbage, and the program segment faults when it tries to dereference the garbage. You touched on it but didn't go into details. – Wug Sep 06 '12 at 20:02
1
 person p;

// stuff

return &p

This is wrong. After the function returns, the local variable is leaving the scope - its address will be invalid. You have to allocate the structure on the heap:

person *new_person(char *firstname, char *lastname, int age)
{
    person *p = malloc(sizeof(*p));
    p->firstname = strdup(firstname);
    p->lastname = strdup(lastname);
    p->age = age;

    return p;
}
0

The problem is in the function new_person. You create person p on the stack and return its address. You need to allocate person* p = new person(.....

danatel
  • 4,844
  • 11
  • 48
  • 62