C - Address 0x0 is not stacked malloc'd or recently freed

[Powdertrail]

Hello.
I am running my program (dynamic stack made using an array of void**) through Valgrind and I get the error of the title.
I am so lost I don't even know what this error is or might be it's cause!

==10525== Invalid read of size 4
==10525==    at 0x8048701: stack_add_numbers (pruebas_alumno.c:20)
==10525==    by 0x80487C7: pupil_stack_test (pruebas_alumno.c:69)
==10525==    by 0x80484F6: main (main.c:13)
==10525==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==10525== 
==10525== 
==10525== Process terminating with default action of signal 11 (SIGSEGV)
==10525==  Access not within mapped region at address 0x0
==10525==    at 0x8048701: pila_agregar_numeros (pruebas_alumno.c:20)
==10525==    by 0x80487C7: prueba_pila_alumno (pruebas_alumno.c:69)
==10525==    by 0x80484F6: main (main.c:13)
==10525==  If you believe this happened as a result of a stack
==10525==  overflow in your program's main thread (unlikely but
==10525==  possible), you can try to increase the size of the
==10525==  main thread stack using the --main-stacksize= flag.
==10525==  The main thread stack size used in this run was 8388608.
==10525== 

This is the code of stack_add_numbers:

bool stack_add_numbers (pila_t* pila, size_t qty)
{
    for (size_t i=0; i<=qty;i++)
    {
        stack_push(stack, (void*)i); 
        void* value = stack_peek_top(stack);
        printf("%i", *((int*)value));
    }
    return true;
}

[Peter87]

Make sure stack_peek_top does not return a null pointer.

[Powdertrail]

I forced it so:

bool stack_add_numbers (pila_t* pila, size_t qty)
{
    for (size_t i=0; i<=qty;i++)
    {
        stack_push(stack, (void*)i); 
        if (stack_peek_top(stack)!=NULL)
        {
           void* value = stack_peek_top(stack);
           printf("%i", *((int*)value));
        }
    }
    return true;
}

And I got a similar error, claiming
==10525== Access not within mapped region at address 0x1
in line 9, but otherwise identical.

So I changed it a little

bool stack_add_numbers (pila_t* pila, size_t qty)
{
    size_t* iter_point; 
    for (size_t i=0; i<=qty;i++)
    {
        iter_point = &i;
        stack_push(stack, iter_point); 
        if (stack_peek_top(stack)!=NULL)
        {
           void* value = stack_peek_top(stack);
           printf("%i", *((int*)value));
        }
     }
    return true;
}

And that did it. Then I noticed that checking for null pointers at the top of the stack makes no difference (for Valgrind at least) and I don't know if having that adds stability or if it's a check suggested for other reasons. Mind explaining why it didn't work and now it does?

[CrashMeister]

Without seeing the code for stack_push and stack_peek_top, we have no way of knowing what is going on INSIDE those operations, and therefore have no vision into what the stack's structure might be.

However, I see one really huge red flag staring at me.

It's this:

   iter_point = &i;

It appears you're pushing the address of a size_t, but the &i is a local temporary.
Further, it's always the same. No matter what the value of i, no matter how large qty, the address of i never changes. So every push of the stack will be the same.

However, what's worse, is that since what is pushed is the address of i, then no matter how many pushes you make, they're all pointing to the same i, which is changing as it loops.

Once that loop is done, the pointers are invalid. Using them would likely crash the machine.

Without knowing what stack_push expects, I have no idea how to advise, but you must push a copy of i, so you retain the value of i at the time of the push. If you push a pointer, it has to be a pointer to a new object (and it's odd to make new ints - that is dynamically allocated ints), so I really don't know what you might expect.

Show more code, and you'll get better advice.

[Powdertrail]

Sorry, still trying to get the hang of "least amount of possibly useful code". While I analyze your post, here they go:

void* stack_peek_top(const stack_t* stack)
    {return stack->data[(stack->n_of_elements)-1];}

bool stack_push(stack_t* stack, void* value)
{
    if((stack->n_of_elements)>=(stack->capacity))
        {return (stack_redimension(stack, stack->n_of_elements, stack->capacity, value));}
    else
    {
        stack->data[stack->n_of_elements]=value;
        stack->n_of_elements++;
        return true;
    }
}

[Peter87]

I apologize for my first post that was not very helpful. The problem with your original code is that you cast the value of i to a pointer. When i is 0 this gives you a null pointer. When i is 1 it gives you a pointer pointing to memory address 1. What is stored there? Trying to access this location could lead to a crash.

[CrashMeister]

Peter87 is right about the first version, but the second version ( I ignored the first version, as I came in later anyway) uses the &i, which is just as bad ;)

Your stack appears to store void *. Ok, but you're pushing an integer. Seems odd, but it can be done. The problem, as I see it, you don't have an integer to push that will survive.

If you use a debugger, you'll see that every push (of this latter version) will push the same address, that of the temporary i.

If you loop through the stack AFTER the loop (another loop just to read through the stack), it will be problematic. This is because the address being pushed is from the stack. It's not a valid CHOICE.

It may happen to SUGGEST it works. Yet, it won't. Watch as you loop through just reading the stack and look at the address at each pass of the loop....it's the same address. Ask yourself, how can the same address produce a different number when dereferenced?

It can only do that because you're actually reading the stack....the loop variable, i, in the case of the latter algorithm posted.

If you must push addresses, you have to have an address that makes sense...a copy of i.

Unfortunately, that would suggest a push of:

iter_point  = new int( i );
push_stack( iter_point );

But that says the stack now owns that memory....it would have to DELETE that int as it pops.

Not a good plan.

For pushing ints, you really need a stack that takes ints, not integer pointers.

For that matter, even if you intended to push pointers to objects on the stack, those objects STILL must be owned - that is, something has to be responsible for deleting those objects later...and that should NOT be the stack itself.

[Powdertrail]

Peter: Thanks, now it's clear. What I intended to do was "stack_push(stack, (void*)&i)" (though that seems to be wrong too)


CrashMeister: Your analysis is spot on. Thanks for your reply.
"It can only do that because you're actually reading the stack".
This is wrong due to the small size of the stack that can lead to overflow, right?


All I'm trying to do is make a test that checks whether the stack works properly.
The stack is meant to take anything you throw at it. Therefore I thought ints were the simplest thing.

I understand the right way to do this is by pushing pointers whose pointed values are those of the FOR loop (0,1,2...).

How could I get to create, name and set the values of those pointers within a loop? I need over 200 of them...
Freeing the allocated memory is not an issue, I can do that later within the same test.

If you think there's an easier way to check the stack works, please let me know that too.

EDIT: I'M WORKING IN C! God am I dumb, can't believe I hadn't written that!