dr_mp3: potential memory corruption after free (flagged by Coverity)

[kcgen]

Coverity flagged that drmp3_init_memory frees the address of "mp3", possibly corrupting it for future pathways in drmp3_open_memory_and_read_pcm_frames_s16 and drmp3_open_memory_and_read_pcm_frames_s32.

A non-heap pointer is placed on the free list, likely causing a crash later. In drmp3_open_memory_and_read_pcm_frames_s16 (and s32): Free of an address-of expression, which can never be heap allocated.

4209 if (!drmp3_init_memory(&mp3, pData, dataSize, pAllocationCallbacks)) {
4210 return NULL;
4211 }
4212 
4213 return drmp3__full_read_and_close_f32(&mp3, pConfig, pTotalFrameCount);
4214 }

and

4219 if (!drmp3_init_memory(&mp3, pData, dataSize, pAllocationCallbacks)) {
4220 return NULL;
4221 }
4222 
4223 return drmp3__full_read_and_close_s16(&mp3, pConfig, pTotalFrameCount);
4224 }

[kcgen]

and

[mackron]

I'm not seeing where the mp3 object is being freed anywhere. Does it say the exact line where the alleged free is occurring?

[kcgen]

Does it say the exact line where the alleged free is occurring?

When init_memory(..) returns, we pass the the drmp3 mp3 stack-variable into full_read_and_close(...), which is where the free happens via the uninit(..) function..

[mackron]

drmp3_uninit() certainly does not attempt to free the mp3 object. If it did I'd be experiencing crashes all over the place in my own software :)

DRMP3_API void drmp3_uninit(drmp3* pMP3)
{
    if (pMP3 == NULL) {
        return;
    }

#ifndef DR_MP3_NO_STDIO
    if (pMP3->onRead == drmp3__on_read_stdio) {
        fclose((FILE*)pMP3->pUserData);
    }
#endif

    drmp3__free_from_callbacks(pMP3->pData, &pMP3->allocationCallbacks);
}

The only thing it's freeing is an internal data buffer. I'm wondering if the analyzer is getting confused with the mp3 object and the mp3.pData object? I'm tempted to flag this as a false positive as I'm still not seeing the issue here.

[kcgen]

I agree @mackron ; this is looking like a false positive.

The code is straight forward and the variable's scope is short-lived and then it's gone. Fortunately we don't have to chase a static object or memory allocation across the lifespan of the program!

I'm OK if you close it as a false positive. Thanks for digging into it!

[mackron]

No worries. Closing.