seelabutk / pbnj

C++ wrapper library around OSPRay to simplify scientific volume rendering
5 stars 3 forks source link

Double free of `oCamera` #3

Open player1537 opened 7 years ago

player1537 commented 7 years ago

Most of the destructors do too much and this causes double frees. For instance, consider:


Camera::~Camera()
{
    ospRelease(this->oCamera);
}

Renderer::~Renderer()
{
    ospRelease(this->oRenderer);
    ospRelease(this->oCamera);
    ospRelease(this->oModel);
    ospRelease(this->oSurface);
    ospRelease(this->oMaterial);
}

When you destroy the Renderer first and then destroy the Camera, you get a double free on the oCamera. If you made a copy of the oCamera, then it'd probably be okay, but Renderer just asks for the Camera's ospray object.

Probably replacing the ospRelease(this->oCamera) with this->oCamera = NULL would be sufficient.

ahota commented 7 years ago

I haven't tested this before. Is there actually a double free error thrown when doing this (I'm assuming as part of the Python wrapper)?

We're not typically making copies of the objects, and the Renderer does just get a pointer to the OSPRay camera object. But the ospRelease() function is supposed to just decrease the refcounter. Since both the PBNJ Camera and Renderer both hold a pointer to the same oCamera, this should be called twice from what I understand.

player1537 commented 7 years ago

Oh, I thought that ospRelease did a free, but decrementing a ref count makes a lot more sense.

I assumed it was a double free, since it happens when destroying the Python objects. Unfortunately, I don't know what order they get destroyed in, and I suspect it might be: new camera, new renderer, destroy camera, destroy renderer.

I get the error with the script in the Python module pull request.