mackron
commented
4 years ago Does it actually sound right when running through simple_playback? I ran it through my automated test program which uses libFLAC as a benchmark and it's passing everything. That is, it's output is identical to libFLAC. Some of those numbers (-442, -2861, etc.) would result in blatantly wrong output so I'm not sure about this.
I'm not convinced yet that this is a bug in dr_flac. It looks like those shifts are from lines like this?
unusedBitsPerSample + pFlac->currentFLACFrame.subframes[0].wastedBitsPerSample;Here, wastedBitsPerSample cannot be negative since it's an unsigned uint8, so it's not that causing it. That leaves unusedBitsPerSample as the culprit, and that is calculated as 32 - pFlac->bitsPerSample. That means pFlac->bitsPerSample would need to be greater than 32 for it to become negative. However, this should never happen because pFlac->bitsPerSample is set from a lookup table with this code:
const drflac_uint8 bitsPerSampleTable[8] = {0, 8, 12, (drflac_uint8)-1, 16, 20, 24, (drflac_uint8)-1}; /* -1 = reserved. */
...
header->bitsPerSample = bitsPerSampleTable[bitsPerSample];And the -1 cases are guarded with this:
if (bitsPerSample == 3 || bitsPerSample == 7) {
continue;
}So considering dr_flac is producing identical output to libFLAC and I'm not seeing any obvious logic errors, I'm not convinced (yet) that this is a dr_flac bug. I also tried explicitly checking for the case where pFlac->bitsPerSample is > 32 and nowhere did it get triggered. This is running on MSVC and GCC on Windows by the way (haven't been able to run it with -fsanitize=undefined).
Is it somehow possible to break on one of those errors and post a stack trace (I've never used the ubsan stuff so I'm not sure if this is possible or not)? That may be handy to see exactly which variables are out of place.
Is it happening with just that one file by the way?
kcgen
u3shit
Download the flac file from here: https://opengameart.org/content/a-journey-awaits I'm using the simple_playback example from miniaudio to try this, but I could also reproduce it in my other project that uses dr_flac directly. I've updated dr_flac to 0.12.9 on miniaudio master then compiled the simple_playback demo:
OS: gentoo linux, clang 10.0.0. I get the same errors with gcc 9.3.0's ubsan too. Since these errors mostly came from
#if defined(DRFLAC_SUPPORT_SSE2), I tried to disable simd, I got even more errors:Strictly speaking, shifting a negative number is implementation defined (https://wiki.sei.cmu.edu/confluence/display/c/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands), but it should work correctly on msvc and gcc according to the above link. To get rid of this warning, we should either have to cast those integers to unsigned, or use
__attribute__((no_sanitize("shift-base")))annotation on the functions that do bit shifting.