weird issue using struct pointer in c

Question

Ok, so i'm totally new to structs in c, and i have a problem that seems very strange to me.
When passing a simple struct to a function using it's pointer, the struct automatically takes one of the other arguments of that function as it's new data. I have no idea why this would happen.. At this moment move_walker() should do nothing at all, right?

typedef struct {
    int x,
        y;
} walker_t;

walker_t* init_walker(int x, int y) {
    walker_t walker;
    walker.x = x;
    walker.y = y;
    walker_t *pointer = malloc(sizeof(walker));
    pointer = &walker;
    return pointer;
}

int move_walker(walker_t * walker, int direction) {
    return 0;
}

walker_t* walker;
walker = init_walker(8,2);

printf("%d %d\n", walker->x, walker->y); //will print '8 2'
move_walker(walker, 3);
printf("%d %d\n", walker->x, walker->y); //will print '0 3'

(I'm pretty sure that it doesnt matter, but this code is actually spreaded over multiple files.)

You have a typo ;-) change pointer = &walker; for *pointer = walker; — qPCR4vir
– qPCR4vir, Commented Feb 19, 2013 at 12:03
Why did you decide to use dynamic memory allocation for this? — Lundin
– Lundin, Commented Feb 19, 2013 at 12:56

Medo42 · Accepted Answer · 2013-02-19 11:50:37Z

3

The problem is that your walker pointer goes to invalid stack memory, because init_walker has a bug: You create a walker_t struct on the stack, then reserve memory with malloc and assign the address of that memory to pointer. So far so good.

However, the line pointer = &walker does not copy the struct from the stack to the new memory, instead it makes pointer point to your struct on the stack! &walker is the address of walker, and you assign that to your pointer. What you probably want to do is to copy the struct. To do that, you have to dereference your pointer:

*pointer = walker

That should make your program work as intended. You can also skip the struct on the stack completely:

walker_t* init_walker(int x, int y) {
    walker_t *walker = malloc(sizeof(walker_t));
    walker->x = x;
    walker->y = y;
    return walker;
}

answered Feb 19, 2013 at 11:50

Medo42

3,8411 gold badge23 silver badges37 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

tom · Accepted Answer · 2013-02-20 00:39:25Z

Your init_walker is wrong, because it returns a pointer to a stack-local variable, walker. That variable's memory gets reclaimed once init_walker exits. Your first printf still works, sort of by accident, because your walker variable's value is still untouched on the stack. As soon as you make any function call after that, however, the stack frame of your original init_walker call gets overwritten, and the walker pointer is now pointing to some random garbage.

When you malloc inside init_walker, you're already allocating memory on the heap (which, unlike the stack, lives beyond the lifetime of a stack frame) for your walker_t. So, you should do this instead:

walker_t* init_walker(int x, int y) {
    walker_t *pointer = malloc(sizeof(walker_t));
    pointer->x = x;
    pointer->y = y;
    return pointer;
}

bash.d · Accepted Answer · 2013-02-19 11:58:56Z

2

You are creating your struct-object on the stack. You'll need to allocate it using

walker_t* init_walker(int x, int y) {
walker_t* walker = malloc(sizeof(walker_t));
...
return walker;
}

With

walker_t *pointer = malloc(sizeof(walker));
pointer = &walker;

you are creating a memory-leak! You assign new memory to *pointer and lose the pointer when you assign &walker to pointer.

edited Feb 19, 2013 at 11:58

answered Feb 19, 2013 at 11:48

bash.d

13.3k3 gold badges33 silver badges42 bronze badges

9 Comments

SomeWittyUsername Over a year ago

Memory leak is another issue, and your solution doesn't resolve it. The issue that you're fixing is dereferencing dangling reference, not the memory leak!

user1666419 Over a year ago

You're right! this works. Altough i still don't fully understand what the problem is, since the first printf-statement works, but as soon as the pointer is passed to a function it doesn't.

bash.d Over a year ago

@icepack Sorry, don't get you... I assign memory to a new object, what goes beyond this, is of no importance in this function.

Medo42 Over a year ago

@user1666419 It works at first because the memory you point to still has the value of the stack-allocated walker. The memory is not valid anymore, but it has not been needed for anything else yet, so it is not overwritten. However, once you call the function it sets up its own stack frame, which overwrites your struct.

qPCR4vir Over a year ago

@icepack not exactly. It can be normal that a init function return a pointer that the user need to free after use. But if rigth after malloc you overwrite the pointer you losse any chance to free the memory.

|

qPCR4vir · Accepted Answer · 2013-02-19 11:54:57Z

0

 walker_t* init_walker(int x, int y) {
    walker_t walker;
    walker.x = x;
    walker.y = y;
    walker_t *pointer = malloc(sizeof(walker));
    *pointer = walker;  /* here was the error. Copy the value not the adress */
    return pointer;
}

But it can be simpler:

walker_t* init_walker(int x, int y) {

    walker_t *pointer = malloc(sizeof(*pointer));
    pointer->x = x;
    pointer->y = y;       
    return pointer;
}

edited Feb 19, 2013 at 11:54

answered Feb 19, 2013 at 11:49

qPCR4vir

3,5821 gold badge24 silver badges32 bronze badges

Comments

Anonymouse · Accepted Answer · 2013-02-19 11:55:47Z

0

your code is to put it polity 'very strange'.

This will work better...

walker_t *init_walker (int x, int y)
{
    walker_t *p_walker = (walker_t *)malloc (sizeof(walker));

    if (p_walker != NULL)
    {
        p_walker->x = x;
        p_walker->y = y;
    }
    return (p_walker);
}

Then call free (walker) when you've finished with them

answered Feb 19, 2013 at 11:55

Anonymouse

95510 silver badges22 bronze badges

4 Comments

Lundin Over a year ago

Why did you cast the result of malloc? I would like to hear the reason why.

Anonymouse Over a year ago

because if you compile the code with lots of compiler warnings enabled (which is a VERY GOOD THING), or compile as C++, you'll get a warning\error about casting a void* to walker_t*. Compiler warnings are your friends, enable them.

Lundin Over a year ago

In C you do not get any compiler warnings for implicit casts of void pointers on any decent compiler. For example, try removing the cast and compile with gcc -std=c99 -Wall -Wextra -pedantic. On the contrary, your typecast hides useful warnings, which is dangerous on a C90 compiler. Also read this. In C++, you shouldn't use malloc at all, it is dangerous in C++.

Lundin Over a year ago

So please kindly remove the typecast from your answer, since this is tagged C.

Lundin · Accepted Answer · 2013-02-19 13:00:11Z

0

...or, after a sanity check of the code, you could as well write:

typedef struct 
{
    int x,
    int y;
} walker_t;

void init_walker(walker_t* obj, int x, int y) 
{
   obj->x = x;
   obj->y = y;
}

walker_t walker;
init_walker(&walker, 8,2);

answered Feb 19, 2013 at 13:00

Lundin

220k47 gold badges282 silver badges446 bronze badges

Collectives™ on Stack Overflow

weird issue using struct pointer in c

6 Answers 6

Comments

Comments

9 Comments

Comments

4 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

Comments

Comments

9 Comments

Comments

4 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related