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, Feb 19 '13 at 12:03
Why did you decide to use dynamic memory allocation for this? — Lundin, Feb 19 '13 at 12:56

tom · Answer 1 · 2013-02-20T00:39:25.653

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;
}

score 3 · Answer 2 · answered Feb 19 '13 at 11:50

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;
}

bash.d · Answer 3 · 2013-02-19T11:58:56.813

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 '13 at 11:58

answered Feb 19 '13 at 11:48

bash.d

13,029
3
29
42

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! – SomeWittyUsername Feb 19 '13 at 11:55
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. – user1666419 Feb 19 '13 at 11:56
@icepack Sorry, don't get you... I assign memory to a new object, what goes beyond this, is of no importance in this function. – bash.d Feb 19 '13 at 11:58
@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. – Medo42 Feb 19 '13 at 12:02
1

@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 Feb 19 '13 at 12:08
@qPCR4vir this is exactly what I said, thank you! It is not the concern of the function to deallocate the allocated memory, so this is fine... – bash.d Feb 19 '13 at 12:09
@bash.d whether it's important or not in the function, is another question. Your solution doesn't affect in any way the possible memory leak. It does fixes access to freed memory. – SomeWittyUsername Feb 19 '13 at 13:43
@icepack Sorry, your concern does not make any sense at all... If I call malloc and leave out the part where he assigns the address to the previously allocated memory, everything is fine! – bash.d Feb 20 '13 at 08:13
@bash.d Everything is fine as far as OP's question is considered. You, however, talk about memory leak, which is entirely different problem - and your solution doesn't address it in any way. Memory leak shouldn't be mentioned at all here. Look at the answers of Medo42 & tom – SomeWittyUsername Feb 20 '13 at 09:07

qPCR4vir · Answer 4 · 2013-02-19T11:54:57.667

 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;
}

score 0 · Answer 5 · answered Feb 19 '13 at 11:55

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 '13 at 11:55

Anonymouse

935
9
20

Why did you cast the result of malloc? I would like to hear the reason why. – Lundin Feb 19 '13 at 13:02
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. – Anonymouse Feb 19 '13 at 14:11
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](http://c-faq.com/malloc/mallocnocast.html). Also [read this](http://stackoverflow.com/questions/1565496/specifically-whats-dangerous-about-casting-the-result-of-malloc). In C++, you shouldn't use malloc at all, it is [dangerous in C++](http://www.parashift.com/c++-faq-lite/mixing-malloc-and-delete.html). – Lundin Feb 19 '13 at 14:35
So please kindly remove the typecast from your answer, since this is tagged C. – Lundin Feb 19 '13 at 14:37

score 0 · Answer 6 · answered Feb 19 '13 at 13:00

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 '13 at 13:00

Lundin

195,001
40
254
396

weird issue using struct pointer in c

6 Answers6