Possible UB in memcpy due to non-null argument

[bwrsandman]

I know this is an esoteric warning but clang-analyzer warns that even though calls to drmp3dec_decode_frame have asserts to make sure mp3 is not null, the parameter itself is not marked as non-null.

Since the parameter is plugged into memcpy without checking, it emits this warning on clang and gcc.

---

warning: Null pointer passed to 2nd parameter expecting 'nonnull' [clang-analyzer-core.NonNullParamChecker]

DRMP3_COPY_MEMORY(dec->header, hdr, DRMP3_HDR_SIZE);
^

Additional context

externals/dr_libs/dr_mp3.h:734: expanded from macro 'DRMP3_COPY_MEMORY'

#define DRMP3_COPY_MEMORY(dst, src, sz) memcpy((dst), (src), (sz))
                                        ^

src/Audio/MpegAudioDecoder.cpp:28: Calling 'drmp3_get_pcm_frame_count'

    auto frameCount = drmp3_get_pcm_frame_count(&_mp3);
                   ^

externals/dr_libs/dr_mp3.h:3991: Calling 'drmp3_get_mp3_and_pcm_frame_count'

    if (!drmp3_get_mp3_and_pcm_frame_count(pMP3, NULL, &totalPCMFrameCount)) {
         ^

externals/dr_libs/dr_mp3.h:3933: 'pMP3' is not equal to NULL

    if (pMP3 == NULL) {
        ^

externals/dr_libs/dr_mp3.h:3933: Taking false branch

    if (pMP3 == NULL) {
    ^

externals/dr_libs/dr_mp3.h:3943: Assuming field 'onSeek' is not equal to NULL

    if (pMP3->onSeek == NULL) {
        ^

externals/dr_libs/dr_mp3.h:3943: Taking false branch

    if (pMP3->onSeek == NULL) {
    ^

externals/dr_libs/dr_mp3.h:3950: Calling 'drmp3_seek_to_start_of_stream'

    if (!drmp3_seek_to_start_of_stream(pMP3)) {
         ^

externals/dr_libs/dr_mp3.h:3773: Calling 'drmp3__on_seek'

    if (!drmp3__on_seek(pMP3, 0, drmp3_seek_origin_start)) {
         ^

externals/dr_libs/dr_mp3.h:2600: Assuming the condition is false

    if (!pMP3->onSeek(pMP3->pUserData, offset, origin)) {
        ^

externals/dr_libs/dr_mp3.h:2600: Taking false branch

    if (!pMP3->onSeek(pMP3->pUserData, offset, origin)) {
    ^

externals/dr_libs/dr_mp3.h:2604: 'origin' is equal to drmp3_seek_origin_start

    if (origin == drmp3_seek_origin_start) {
        ^

externals/dr_libs/dr_mp3.h:2604: Taking true branch

    if (origin == drmp3_seek_origin_start) {
    ^

externals/dr_libs/dr_mp3.h:2610: Returning without writing to 'pMP3->atEnd', which participates in a condition later

    return DRMP3_TRUE;
    ^

externals/dr_libs/dr_mp3.h:2610: Returning without writing to 'pMP3->pData'

    return DRMP3_TRUE;
    ^

externals/dr_libs/dr_mp3.h:3773: Returning from 'drmp3__on_seek'

    if (!drmp3__on_seek(pMP3, 0, drmp3_seek_origin_start)) {
         ^

externals/dr_libs/dr_mp3.h:3773: Taking false branch

    if (!drmp3__on_seek(pMP3, 0, drmp3_seek_origin_start)) {
    ^

externals/dr_libs/dr_mp3.h:3778: Calling 'drmp3_reset'

    drmp3_reset(pMP3);
    ^

externals/dr_libs/dr_mp3.h:3765: Returning without writing to 'pMP3->pData'

}
^

externals/dr_libs/dr_mp3.h:3778: Returning from 'drmp3_reset'

    drmp3_reset(pMP3);
    ^

externals/dr_libs/dr_mp3.h:3779: Returning without writing to 'pMP3->pData'

    return DRMP3_TRUE;
    ^

externals/dr_libs/dr_mp3.h:3950: Returning from 'drmp3_seek_to_start_of_stream'

    if (!drmp3_seek_to_start_of_stream(pMP3)) {
         ^

externals/dr_libs/dr_mp3.h:3950: Taking false branch

    if (!drmp3_seek_to_start_of_stream(pMP3)) {
    ^

externals/dr_libs/dr_mp3.h:3957: Loop condition is true. Entering loop body

    for (;;) {
    ^

externals/dr_libs/dr_mp3.h:3960: Calling 'drmp3_decode_next_frame_ex'

        pcmFramesInCurrentMP3Frame = drmp3_decode_next_frame_ex(pMP3, NULL);
                                     ^

externals/dr_libs/dr_mp3.h:2795: Assuming field 'pData' is equal to NULL

    if (pMP3->memory.pData != NULL && pMP3->memory.dataSize > 0) {
        ^

externals/dr_libs/dr_mp3.h:2795: Left side of '&&' is false

    if (pMP3->memory.pData != NULL && pMP3->memory.dataSize > 0) {
                                   ^

externals/dr_libs/dr_mp3.h:2798: Calling 'drmp3_decode_next_frame_ex__callbacks'

        return drmp3_decode_next_frame_ex__callbacks(pMP3, pPCMFrames);
               ^

externals/dr_libs/dr_mp3.h:2651: Field 'atEnd' is 0

    if (pMP3->atEnd) {
              ^

externals/dr_libs/dr_mp3.h:2651: Taking false branch

    if (pMP3->atEnd) {
    ^

externals/dr_libs/dr_mp3.h:2655: Loop condition is true. Entering loop body

    for (;;) {
    ^

externals/dr_libs/dr_mp3.h:2659: Field 'dataSize' is < DRMP3_MIN_DATA_CHUNK_SIZE

        if (pMP3->dataSize < DRMP3_MIN_DATA_CHUNK_SIZE) {
                  ^

externals/dr_libs/dr_mp3.h:2659: Taking true branch

        if (pMP3->dataSize < DRMP3_MIN_DATA_CHUNK_SIZE) {
        ^

externals/dr_libs/dr_mp3.h:2663: Assuming pointer value is null

            if (pMP3->pData != NULL) {
                ^

externals/dr_libs/dr_mp3.h:2663: Assuming field 'pData' is equal to NULL

            if (pMP3->pData != NULL) {
                ^

externals/dr_libs/dr_mp3.h:2663: Taking false branch

            if (pMP3->pData != NULL) {
            ^

externals/dr_libs/dr_mp3.h:2669: Assuming the condition is false

            if (pMP3->dataCapacity < DRMP3_DATA_CHUNK_SIZE) {
                ^

externals/dr_libs/dr_mp3.h:2669: Taking false branch

            if (pMP3->dataCapacity < DRMP3_DATA_CHUNK_SIZE) {
            ^

externals/dr_libs/dr_mp3.h:2684: Calling 'drmp3__on_read'

            bytesRead = drmp3__on_read(pMP3, pMP3->pData + pMP3->dataSize, (pMP3->dataCapacity - pMP3->dataSize));
                        ^

externals/dr_libs/dr_mp3.h:2593: Returning without writing to 'pMP3->pData'

    return bytesRead;
    ^

externals/dr_libs/dr_mp3.h:2684: Returning from 'drmp3__on_read'

            bytesRead = drmp3__on_read(pMP3, pMP3->pData + pMP3->dataSize, (pMP3->dataCapacity - pMP3->dataSize));
                        ^

externals/dr_libs/dr_mp3.h:2685: Assuming 'bytesRead' is not equal to 0

            if (bytesRead == 0) {
                ^

externals/dr_libs/dr_mp3.h:2685: Taking false branch

            if (bytesRead == 0) {
            ^

externals/dr_libs/dr_mp3.h:2695: Assuming field 'dataSize' is <= INT_MAX

        if (pMP3->dataSize > INT_MAX) {
            ^

externals/dr_libs/dr_mp3.h:2695: Taking false branch

        if (pMP3->dataSize > INT_MAX) {
        ^

externals/dr_libs/dr_mp3.h:2703: Passing null pointer value via 2nd parameter 'mp3'

        pcmFramesRead = drmp3dec_decode_frame(&pMP3->decoder, pMP3->pData + pMP3->dataConsumed, (int)pMP3->dataSize, pPCMFrames, &info);    /* <-- Safe size_t -> int conversion thanks to the check above. */
                                                              ^

externals/dr_libs/dr_mp3.h:2703: Calling 'drmp3dec_decode_frame'

        pcmFramesRead = drmp3dec_decode_frame(&pMP3->decoder, pMP3->pData + pMP3->dataConsumed, (int)pMP3->dataSize, pPCMFrames, &info);    /* <-- Safe size_t -> int conversion thanks to the check above. */
                        ^

externals/dr_libs/dr_mp3.h:2275: Assuming 'mp3_bytes' is <= 4

    if (mp3_bytes > 4 && dec->header[0] == 0xff && drmp3_hdr_compare(dec->header, mp3))
        ^

externals/dr_libs/dr_mp3.h:2275: Left side of '&&' is false

    if (mp3_bytes > 4 && dec->header[0] == 0xff && drmp3_hdr_compare(dec->header, mp3))
                      ^

externals/dr_libs/dr_mp3.h:2283: 'frame_size' is 0

    if (!frame_size)
         ^

externals/dr_libs/dr_mp3.h:2283: Taking true branch

    if (!frame_size)
    ^

externals/dr_libs/dr_mp3.h:2287: Assuming 'frame_size' is not equal to 0

        if (!frame_size || i + frame_size > mp3_bytes)
            ^

externals/dr_libs/dr_mp3.h:2287: Left side of '||' is false

        if (!frame_size || i + frame_size > mp3_bytes)
            ^

externals/dr_libs/dr_mp3.h:2287: Assuming the condition is false

        if (!frame_size || i + frame_size > mp3_bytes)
                           ^

externals/dr_libs/dr_mp3.h:2287: Taking false branch

        if (!frame_size || i + frame_size > mp3_bytes)
        ^

externals/dr_libs/dr_mp3.h:2294: Null pointer value stored to 'hdr'

    hdr = mp3 + i;
    ^

externals/dr_libs/dr_mp3.h:2295: Null pointer passed to 2nd parameter expecting 'nonnull'

    DRMP3_COPY_MEMORY(dec->header, hdr, DRMP3_HDR_SIZE);
    ^

externals/dr_libs/dr_mp3.h:734: expanded from macro 'DRMP3_COPY_MEMORY'

#define DRMP3_COPY_MEMORY(dst, src, sz) memcpy((dst), (src), (sz))
                                        ^

---

A possible fix would be to declare the attribute for gcc and clang as

__attribute__((nonnull (2)))
DRMP3_API int drmp3dec_decode_frame(drmp3dec *dec, const drmp3_uint8* mp3, int mp3_bytes, void *pcm, drmp3dec_frame_info *info);

or for c only

DRMP3_API int drmp3dec_decode_frame(drmp3dec *dec, const drmp3_uint8 mp3[static 1], int mp3_bytes, void *pcm, drmp3dec_frame_info *info);

Obviously a macro such as DRMP3_NONNULL(x) could be added.

Another fix would be to just early out from bad usage. Don't know if that's an option but the function has public (non-implmentation) visibility.

[mackron]

Thanks. This is a weird one because there are literal asserts in there to account for this like you mentioned. It feels like for some reason the compilers are not recognizing those asserts:

DRMP3_ASSERT(pMP3->pData != NULL);
DRMP3_ASSERT(pMP3->dataCapacity > 0);

pcmFramesRead = drmp3dec_decode_frame(&pMP3->decoder, pMP3->pData + pMP3->dataConsumed, (int)pMP3->dataSize, pPCMFrames, &info);    /* <-- Safe size_t -> int conversion thanks to the check above. */

I'm almost wondering if maybe I just do a normal run-time check for pData being null and just returning 0. Have you by chance tried that to see if it cleans up that warning? I don't like the idea of modifying drmp3dec_decode_frame() simply because that code is from minimp3 which I'm not fully intimate with.

I'm not really the biggest fan of the nonnull thing because it just feels a bit hacky, and I'd be nice to avoid compiler-specific stuff if possible. But that said, if I need to do a compiler-specific thing so be it.

[bwrsandman]

I tried what you said and naively returned 0. This seems to silence the error.

It also complains about this move operation. https://github.com/mackron/dr_libs/blob/master/dr_mp3.h#L2725

[mackron]

I've pushed a potential fix to clean up this warning to the dev branch. Are you able to try that?

[bwrsandman]

That seems to have fixed it.

[mackron]

OK, that's good to hear. The fact that the static analyzer is recognizing the runtime check indicates to me that it's not detecting or recognizing the assert for some reason. In any case I'll get this released shortly.

[bwrsandman]

Could be due to NDEBUG

[mackron]

I've updated the master branch with this change. Version 0.6.37.

[bwrsandman]

I've updated dr_wav.h and theres this new one that popped up.

warning: Array access (from variable 'pBufferOut') results in a null pointer dereference [clang-analyzer-core.NullDereference]

                ((drwav_uint8*)pBufferOut)[iSample] += 128;
                                                    ^

Additional context

**src/Audio/WavAudioDecoder.cpp:31:** Calling 'drwav_read_pcm_frames_s16' ```cpp [[maybe_unused]] auto framesRead = drwav_read_pcm_frames_s16(&_wav, frameCount, buffer.data()); ^ ``` **externals/dr_libs/dr_wav.h:6788:** 'pWav' is not equal to NULL ```cpp if (pWav == NULL || framesToRead == 0) { ^ ``` **externals/dr_libs/dr_wav.h:6788:** Left side of '||' is false ```cpp if (pWav == NULL || framesToRead == 0) { ^ ``` **externals/dr_libs/dr_wav.h:6788:** Assuming 'framesToRead' is not equal to 0 ```cpp if (pWav == NULL || framesToRead == 0) { ^ ``` **externals/dr_libs/dr_wav.h:6788:** Taking false branch ```cpp if (pWav == NULL || framesToRead == 0) { ^ ``` **externals/dr_libs/dr_wav.h:6792:** Assuming 'pBufferOut' is equal to NULL ```cpp if (pBufferOut == NULL) { ^ ``` **externals/dr_libs/dr_wav.h:6792:** Taking true branch ```cpp if (pBufferOut == NULL) { ^ ``` **externals/dr_libs/dr_wav.h:6793:** Passing null pointer value via 3rd parameter 'pBufferOut' ```cpp return drwav_read_pcm_frames(pWav, framesToRead, NULL); ^ ``` **/usr/lib/llvm-15/lib/clang/15.0.7/include/stddef.h:83:** expanded from macro 'NULL' ```cpp # define NULL __null ^ ``` **externals/dr_libs/dr_wav.h:6793:** Calling 'drwav_read_pcm_frames' ```cpp return drwav_read_pcm_frames(pWav, framesToRead, NULL); ^ ``` **externals/dr_libs/dr_wav.h:5757:** Taking false branch ```cpp if (drwav_is_container_be(pWav->container)) { ^ ``` **externals/dr_libs/dr_wav.h:5775:** Taking true branch ```cpp if (drwav__is_little_endian()) { ^ ``` **externals/dr_libs/dr_wav.h:5787:** Assuming field 'container' is equal to drwav_container_aiff ```cpp if (pWav->container == drwav_container_aiff && pWav->bitsPerSample == 8 && pWav->aiff.isUnsigned == DRWAV_FALSE) { ^ ``` **externals/dr_libs/dr_wav.h:5787:** Left side of '&&' is true ```cpp if (pWav->container == drwav_container_aiff && pWav->bitsPerSample == 8 && pWav->aiff.isUnsigned == DRWAV_FALSE) { ^ ``` **externals/dr_libs/dr_wav.h:5787:** Assuming field 'bitsPerSample' is equal to 8 ```cpp if (pWav->container == drwav_container_aiff && pWav->bitsPerSample == 8 && pWav->aiff.isUnsigned == DRWAV_FALSE) { ^ ``` **externals/dr_libs/dr_wav.h:5787:** Left side of '&&' is true ```cpp if (pWav->container == drwav_container_aiff && pWav->bitsPerSample == 8 && pWav->aiff.isUnsigned == DRWAV_FALSE) { ^ ``` **externals/dr_libs/dr_wav.h:5787:** Assuming field 'isUnsigned' is equal to DRWAV_FALSE ```cpp if (pWav->container == drwav_container_aiff && pWav->bitsPerSample == 8 && pWav->aiff.isUnsigned == DRWAV_FALSE) { ^ ``` **externals/dr_libs/dr_wav.h:5787:** Taking true branch ```cpp if (pWav->container == drwav_container_aiff && pWav->bitsPerSample == 8 && pWav->aiff.isUnsigned == DRWAV_FALSE) { ^ ``` **externals/dr_libs/dr_wav.h:5790:** Assuming the condition is true ```cpp for (iSample = 0; iSample < framesRead * pWav->channels; iSample += 1) { ^ ``` **externals/dr_libs/dr_wav.h:5790:** Loop condition is true. Entering loop body ```cpp for (iSample = 0; iSample < framesRead * pWav->channels; iSample += 1) { ^ ``` **externals/dr_libs/dr_wav.h:5791:** Array access (from variable 'pBufferOut') results in a null pointer dereference ```cpp ((drwav_uint8*)pBufferOut)[iSample] += 128; ^ ```

[mackron]

Thanks. That's a genuine bug. I've gone ahead and fixed that in the dev branch.