Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heap corruption when using device.rerouting with TraCI #5553

Closed
jprk opened this issue May 2, 2019 · 4 comments

Comments

@jprk
Copy link

commented May 2, 2019

When using device.rerouting with TraCI, updates to vectors in myVehicleStateChanges map may occur in parallel when vehicle rerouters call TraCIServer::vehicleStateChanged() from different threads. This frequently corrupts the myVehicleStateChanges[VEHICLE_STATE_NEWROUTE] vector with various side effects (double free, heap buffer overflow, heap use after free) and the simulation crashes without any error message.

I am currently testing a workaround that adds

std::lock_guard<std::mutex> lock(myVehicleStateChangesMutex);

before

myVehicleStateChanges[to].push_back(vehicle->getID());

so that all vehicle state vectors cannot be modified in parallel.

The myVehicleStateChangesMutex is part of TraCIServer class definition as

std::map<MSNet::VehicleState, std::vector<std::string> > myVehicleStateChanges;
std::mutex myVehicleStateChangesMutex;

I am not sure that this is the appropriate way how to do it (after all, the threads are instantiated by some part of the FOX library), but I am definitely willing to prepare a patch (we are chasing this bug since 12/2018 so I would definitely like to get rid of it ;-)).

Cheers,

Jan

@namdre

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Good find!

The same issue will show up with all other VehicleStateListener instances besides TraCIServer so it might be more appropriate to do the locking in MSNet::informVehicleStateListeners.

@jprk

This comment has been minimized.

Copy link
Author

commented May 3, 2019

Jakob, wouldn't it be better to lock access only to a single VehicleStateListener in a row there? Someting like

void
MSNet::informVehicleStateListener(
    const SUMOVehicle* const vehicle,
    VehicleState to,
    const std::string& info)
{
    for (std::vector<VehicleStateListener*>::iterator i = myVehicleStateListeners.begin();
         i != myVehicleStateListeners.end(); 
         ++i)
    {
        std::lock_guard<std::mutex> lock(...);  // place some listener-dependent mutex here?
        (*i)->vehicleStateChanged(vehicle, to, info);
    }
}

so that the lock is released immediately after the given VehicleStateListener is processed and in case of parallel processing we do not needlessly block other threads?

@namdre

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

I suppose the number of cases where multiple threads call informVehicleStateListener is quite rare (I never saw a crash from that). Thus the potenial for blocking should be low as well.

On the other hand, the number of listeners is potentially quite large (every vehicle if --vehroute-output is enabled). Thus locking every listener, potentially introduces lots of overhead.

@behrisch behrisch closed this in cee0688 May 22, 2019
@behrisch

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

I tried to push the fix Jakob proposed. @jprk Could you test again and reopen if the problem persists?

namdre added a commit that referenced this issue Jun 5, 2019
…oper life-cycle of lock object.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.