Strange behaviour with std::stack, pop() returns same value

Question

I have class that uses std::stack:

class NotificationService{
    public:
        void addPendingNotification(uint8_t *uuid);
        uint8_t* getNextPendingNotification();
        void popPending();
    private:
        std::stack<uint8_t*> pendingNotification;
};

void NotificationService::addPendingNotification(uint8_t *uuid) {
    pendingNotification.push(uuid);
    Serial.print("Insert to stack: ");
    Serial.print(uuid[0]);
    Serial.print(uuid[1]);
    Serial.print(uuid[2]);
    Serial.println(uuid[3]);
}

uint8_t *NotificationService::getNextPendingNotification() {
    if (pendingNotification.size() > 0) {
        uint8_t *uuid = pendingNotification.top();
        Serial.println(*uuid);
        pendingNotification.pop();
        return uuid;
    } else {
        return NULL;
    }
};

void NotificationService::popPending(){
    while (!pendingNotification.empty())
    {
        uint8_t *uuid = pendingNotification.top();
        Serial.print(uuid[0]);
        Serial.print(uuid[1]);
        Serial.print(uuid[2]);
        Serial.println(uuid[3]);
        pendingNotification.pop();
    }
}

I add to stack in my main code (BLE Notification Callback):

static void NotificationSourceNotifyCallback(
    BLERemoteCharacteristic *pNotificationSourceCharacteristic,
    uint8_t *pData,
    size_t length,
    bool isNotify)
{
    if (pData[0] == 0)
    {
        uint8_t messageId[4] = {pData[4], pData[5], pData[6], pData[7]};
        switch (pData[2])
        {
            //Incoming Call
        case 1:
        {
            notificationService->addPendingNotification(messageId);
        }
/** code **/
}

Everything work fine until I want to pop items from the stack, then every item has the same value (last inserted element).

Serial print log:

Insert to stack: 8000
Insert to stack: 32000
Insert to stack: 19000
Insert to stack: 44000
Insert to stack: 4000
Pop whole stack:
4000
4000
4000
4000
4000

So I try to write similar code in an online compiler:

http://cpp.sh/7hv4

And it works fine.

What am I doing wrong?

My ***guess*** (since you don't show a [Minimal, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve)) is that you pass a pointer to a single variable, so all pointers in the stack are pointing to that single variable. I suggest you make a distinct type for the "uuid", one that can be copied or moved and passed by value. Or perhaps reuse a type like `std::array`? — Some programmer dude, Jul 30 '18 at 06:07
Examining the address returned from each `top` invoke should make your wonder how that is possible. Winding back the clock and seeing what you're pushing via each invoke to `notificationService->addPendingNotification(messageId);` will confirm that. Worse, you're invoking undefined behavior regardless. You're effectively pushing *dangling* pointers in to your stack. Each time the `if (pData[0] == 0)` scope block is exited, the address expressed from `messageId` just pushed into the stack is no longer valid for dereference. And you have a whole stack filled with such addresses. — WhozCraig, Jul 30 '18 at 06:15

score 3 · Accepted Answer · edited Jul 30 '18 at 06:22

std::stack<uint8_t*> pendingNotification;

You have a stack of pointers. For this to make sense, you'd have to have a bunch of distinct objects for the stack to hold pointers to, and those objects would have to remain valid for as long as you intend to use the stack of pointers. Your code doesn't do this.

Don't create stacks of pointers unless you have a really good reason to. Instead, create stacks of data values.

Sander De Dycker · Answer 2 · 2018-07-30T06:51:38.440

notificationService->addPendingNotification(messageId);

pushes a pointer to a local variable (the messageId array) onto the stack.

The scope of this local variable ends at some point later (as soon as the enclosing if block ends). But the pointer in the stack still remains. At that point, dereferencing this pointer invokes undefined behavior, so when you do after popping the pointer off the stack, the result you get is undefined behavior.

In your specific case, it seems the compiler has re-used the same memory location for all messageId instances, which means that after pushing all items, that memory location still holds the value of the last pushed item. But to re-iterate : this is undefined behavior, and you should not rely on this.

Instead, either push a copy of the value onto the stack, or push a pointer onto the stack to memory that will remain allocated for the lifetime of the pointer (stack).

Strange behaviour with std::stack, pop() returns same value

2 Answers2