undefined behavior: left shift of negative number

[tamird]

More details in https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=46738, but the gist is:

  ../../third_party/android/platform/external/aac/libFDK/include/fixmadd.h:134:28: runtime error: left shift of negative value -32768
  #0.2  0x000042cb30bc2f24 in fixmadddiv2_SD(FIXP_DBL, FIXP_DBL const, FIXP_SGL const) ../../third_party/android/platform/external/aac/libFDK/include/fixmadd.h:134 <<application>>+0x457f24
  #0.1  0x000042cb30bc2f24 in fMultAddDiv2(FIXP_DBL, FIXP_DBL, FIXP_SGL) ../../third_party/android/platform/external/aac/libFDK/include/common_fix.h:318 <<application>>+0x457f24
  #0    0x000042cb30bc2f24 in fLog2(FIXP_DBL, INT, INT*) ../../third_party/android/platform/external/aac/libFDK/include/fixpoint_math.h:839 <<application>>+0x457f24

We can reproduce this in our infrastructure by commenting out this line, and I'd be happy to help test fixes.

[mstorsjo]

Thanks for the report! FWIW, this repo isn't the canonical upstream, but Fraunhofer is - who then deliver fixes to AOSP, where I pull them in. For issues found elsewhere (e.g. with oss-fuzz via ffmpeg) I do try to some makeshift fix here, but often they end up fixed later properly upstream by Fraunhofer.

I believe that the oss-fuzz setup of ffmpeg only tests decoding, which probably is why this issue hasn't been hit there. FDK-AAC is built there with -fno-sanitize=shift-base,signed-integer-overflow though, since it otherwise would hit more bugs than upstream were interested in addressing - see https://github.com/google/oss-fuzz/blob/master/projects/ffmpeg/build.sh#L55.

I think AOSP does some fuzzing of their libraries these days as well - not sure if they fuzz the encoding side though. Their sanitizers are configured like this: https://cs.android.com/android/platform/superproject/+/master:external/aac/Android.bp;l=62-69

[tamird]

Thanks for the context! Looks like nobody particularly cares about this UB, I'll go ahead and close the bug on our side.

[mstorsjo]

Thanks for the context! Looks like nobody particularly cares about this UB, I'll go ahead and close the bug on our side.

Yeah, possibly. I didn’t check whether this instance is covered by the sanitization disables in AOSP though. If it’s easy to trigger, I presume they’ve either not fuzzed encoding, or don’t care about it. If it’s not entirely immediately triggered, it could be a useful thing for Fraunhofer to look into, but then guess it would require a repro sample too, not just the backtrace.