Crash when checking collisions in Qt6

black_gay

I want to detect all items in collision with a QGraphicsItem (item), yet the code below crashes, id like to know the reason.

void PlayerDefences::checkCollisions(QGraphicsItem* item)
    {
        QList<QGraphicsItem*> itemCollidesWithShip = item->collidingItems(); ///Returns a list of all items that collide with this item.

 
    for (int i = 0; i < itemCollidesWithShip.size(); i++) {
        if(itemCollidesWithShip[i]==nullptr) continue;
 
        if(dynamic_cast<Bullet*>(itemCollidesWithShip[i])){ //if the colliding item is of type Bullet*
            life-=10;
            setActualLife();
            item->scene()->removeItem(itemCollidesWithShip[i]);
            std::vector<Bullet*>* bullets = Level1::getBulletContainer();
 
            bullets->erase(std::remove(bullets->begin(), bullets->end(), itemCollidesWithShip[i]), bullets->end());
            delete itemCollidesWithShip[i] ;
        }
    }
}

The crash goes away, when i add this line below delete:

itemCollidesWithShip.erase(std::remove(itemCollidesWithShip.begin(), itemCollidesWithShip.end(), itemCollidesWithShip[i]), itemCollidesWithShip.end())

SGaist

Hi,

The best way to understand your crash is to use the debugger.

One thing that you do not do after the delete is to set the pointer value to nullptr so what I guess is happening is that you enter that function again and access the deleted pointer.

With the removal of that entry from your vector, you avoid reusing stuff that are deleted and not relevant anymore.

black_gay

I tried with adding itemsCollideWithShip[i] = nullptr; but it also crashes. The only thing that prevents crashes is either adding itemsCollideWithShip.erase(std::remove(itemsCollideWithShip.begin(),itemsCollideWithShip.end(), itemsCollideWithShip[i]), itemsCollideWithShip.end()); or adding i++; though i dont seem to enter the loop again.

SGaist

As suggested, run your application through the debugger to see exactly where it crashes.

black_gay

@SGaist debugger shows segmentation fault at line: if(dynamic_cast<Bullet*>(itemsCollideWithShip[i])){
Another way to solve the problem is:
```
QGraphicsItem* item = itemsCollideWithShip.takeAt(i);
delete item;
item = nullptr;

One more is adding i++.
Which suggests, that for some reason a nullpointer cannot be present in QList, though this check "if(itemsCollideWithShip[i]==nullptr) continue;" prevents using nullpointer, so i dont understand why would i need to remove this nullpointer from the QList

SGaist

QList has no problem storing nullptr.

You should not modify the size of container while you are iterating through it in a for loop.

You can build a list of the index to remove and do it after you're done with your for loop.

black_gay

@SGaist The main question is why would i even want to delete nullpointer from QList at this point, as i only wish to delete the colliding item from bullets (a vector of Bullet*)? I iterate through the QList one by one, so order of its elements doesnt change and keeping a nullpointer in QList seemingly does no harm, as i make sure i dont use the nullpointer later .... but the behaviour of the program results with a crash (segmentation fault). In short QList shouldnt be affected by altering bullets vector (even though they store the same elements).

JonB

@black_gay
When you step through/run under the debugger and it segment faults, please post the full stack trace, which you will find in some debugger window.

If it actually segments on dynamic_cast<Bullet*>(itemCollidesWithShip[i]), I believe code for dynamic_cast<> has to actually deference the pointer to examine what is there in order to determine its type. That would mean that pointer is pointing somewhere bad --- either to random/bad memory, or possibly to already-free/deleted memory.

black_gay

@JonB This is full stack trace dump:
1 ```
libstdc++-6!.dynamic_cast 0x7ffa394002f2
2 PlayerDefences::checkCollisions playerdefences.cpp 50 0x7ff7c1886545
3 Player::advance player.cpp 97 0x7ff7c188553c
4 Level1::advance level1.cpp 102 0x7ff7c1884522
5 QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (Level1:: *)()>::call(void (Level1:: *)(), Level1 *, void * *) qobjectdefs_impl.h 152 0x7ff7c188d669
6 QtPrivate::FunctionPointer<void (Level1:: *)()>::call<QtPrivate::List<>, void>(void (Level1:: *)(), Level1 *, void * *) qobjectdefs_impl.h 185 0x7ff7c188e33c
7 QtPrivate::QSlotObject<void (Level1:: *)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase *, QObject *, void * *, bool *) qobjectdefs_impl.h 395 0x7ff7c188d86a
8 QMimeType::filterString() const 0x7ffa30d99357
9 QTimer::timerEvent(QTimerEvent *) 0x7ffa30b28a9a
10 QObject::event(QEvent *) 0x7ffa30b0b3b7
11 QApplicationPrivate::notify_helper(QObject *, QEvent *) 0x7ffa30415946
12 QCoreApplication::sendEvent(QObject *, QEvent *) 0x7ffa30ad6ed8
13 QEventDispatcherWin32Private::sendTimerEvent(int) 0x7ffa30c49f2f
14 QEventDispatcherWin32::event(QEvent *) 0x7ffa30c4f700
15 QApplicationPrivate::notify_helper(QObject *, QEvent *) 0x7ffa30415946
16 QCoreApplication::notifyInternal2(QObject *, QEvent *) 0x7ffa30ad69ed
17 QCoreApplicationPrivate::sendPostedEvents(QObject *, int, QThreadData *) 0x7ffa30ada3a9
18 QWindowsGuiEventDispatcher::sendPostedEvents() 0x7ffa2b4b9e6e
19 QEventDispatcherWin32::processEvents(QFlagsQEventLoop::ProcessEventsFlag) 0x7ffa30c4e9f0
20 QWindowsGuiEventDispatcher::processEvents(QFlagsQEventLoop::ProcessEventsFlag) 0x7ffa2b4b9e55
21 QEventLoop::exec(QFlagsQEventLoop::ProcessEventsFlag) 0x7ffa30ae0fd3
22 QCoreApplication::exec() 0x7ffa30adf8b6
23 main main.cpp 9 0x7ff7c1884c52

this is debugger's dump
http://pastebin.com/RgDwe6R8

hskoglund

Hi, just a guess but iperhaps your QList crashes because it contains duplicates of the same bullet (dynamic_cast<> doesn't like casting deleted poiinters).
And that could explain why the extra c++ line you show that avoids the crash by removing duplicates from the QList works (because then that dynamic_cast<> never sees the duplicates).

JonB

@black_gay
Immediately after delete itemCollidesWithShip[i] ; you fail to go itemCollidesWithShip[i] = nullptr. You have never shown you do, because

QGraphicsItem* item = itemsCollideWithShip.takeAt(i);
delete item;
item = nullptr;

the above does not achieve that.

That means you leave a non-nullptr sitting in the list. Next time this code is called it does not follow the if(itemCollidesWithShip[i]==nullptr) continue route. Instead it checks this pointer, now pointing to garbage, for if(dynamic_cast<Bullet*>(itemCollidesWithShip[i])), and that is liable to go weird/crash.

Adding

itemCollidesWithShip.erase(std::remove(itemCollidesWithShip.begin(), itemCollidesWithShip.end(), itemCollidesWithShip[i]), itemCollidesWithShip.end())

removes that pointer from the list (though you ought not do it inside the for loop), hence no crash.

Isn't this all your situation is?

black_gay

@JonB It also crashes when i set itemsCollideWithShip[i] = nullptr. I modified the code as below, but std::bad_case doesnt show anything. I also set loads of debug messages, and it shows that after "delete item" in previous iteration, when i want to acces next item in the QList (ex. 3rd is deleted, and 4th is accesed) in the next iteration of the loop - it crashes (segmentation fault). Crashes can be avoided only if i add i++ inside the loop, or use remove/erase idiom on QList, or store all the colliding elements in a temporary container and then call delete on each of them. But anyway, i cant get over it that the crash appears - no idea how can it be, that the next element gets corrupted by deleting the previous one in QList. There arent duplicated in QList.

void PlayerDefences::checkCollisions(QGraphicsItem* item)
{
    try {
        QList<QGraphicsItem*> itemsCollideWithShip = item->collidingItems();
        std::vector<Bullet*>* bullets = Level1::getBulletContainer();
        for (int i = 0; i < itemsCollideWithShip.size(); i++) {
            if(itemsCollideWithShip[i]==nullptr) continue;
            if(dynamic_cast<Bullet*>(itemsCollideWithShip[i]) ){
                life-=10;
                setActualLife();
                Game::getScene()->removeItem(itemsCollideWithShip[i]);
               bullets->erase(std::remove(bullets->begin(), bullets->end(), itemsCollideWithShip[i]), bullets->end());
                delete itemsCollideWithShip[i];
                itemsCollideWithShip[i] = nullptr;
            }
        }
    } catch (const std::bad_cast& e) {
        std::cout << e.what() << '\n';
    }
}

SGaist

What about dumping the content of the data structures you are using to see what's inside and if anything changes while you are accessing/modifying it ?