Workaround crash in atrac3+ decoding #66
Conversation
| ./configure --logfile=conflog.txt --target-os=linux \ | ||
| --prefix=./android/arm64 \ | ||
| --arch=aarch64 \ | ||
| ${GENERAL} \ | ||
| --sysroot=$NDK/sysroot \ | ||
| --extra-cflags=" --target=aarch64-none-linux-android21 -no-canonical-prefixes -fdata-sections -ffunction-sections -fno-limit-debug-info -funwind-tables -fPIC -O2 -DANDROID -DANDROID_PLATFORM=$BUILD_ANDROID_PLATFORM -Dipv6mr_interface=ipv6mr_ifindex -fasm -fno-short-enums -fno-strict-aliasing -Wno-missing-prototypes" \ | ||
| --extra-cflags="$EXTRA_CC --target=aarch64-none-linux-android21 -no-canonical-prefixes -fdata-sections -ffunction-sections -fno-limit-debug-info -funwind-tables -g -O2 -DANDROID -DANDROID_PLATFORM=$BUILD_ANDROID_PLATFORM -Dipv6mr_interface=ipv6mr_ifindex -fasm -fno-short-enums -fno-strict-aliasing" \ |
There was a problem hiding this comment.
Intentional to leave -g here? Also, last time I tried the latest NDK, there were issues related to PIC. This stuff:
ld: error: relocation R_AARCH64_ADR_PREL_PG_HI21 cannot be used against symbol ff_h264_idct_dc_add_neon; recompile with -fPIC
ld: error: can't create dynamic relocation R_AARCH64_ADD_ABS_LO12_NC against symbol: ff_h264_idct_dc_add_neon in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
Not sure if this is just old binaries or something, though. I wonder if we need -pic or something else?
-[Unknown]
There was a problem hiding this comment.
pic is enabled with --enable-pic in the GENERAL settings above. -g might not be neeed, I just wanted as much stack trace info as I can get... And it did compile and work with no issues.
| if ((ret = init_get_bits8(&ctx->gb, avpkt->data, avpkt->size)) < 0) | ||
| // PPSSPP workaround: With bad/corrupt input, the atrac3plus decoder does not | ||
| // reliably stay inside the bounds of the buffer. Instead of carefully checking everything | ||
| // inside it, for now let's just give it more space to read from. | ||
| const int extra_bytes = 1024; | ||
|
|
||
| uint8_t *bigger_buffer = malloc(avpkt->size + extra_bytes); | ||
| memset(bigger_buffer + avpkt->size, 0, extra_bytes); | ||
| memcpy(bigger_buffer, avpkt->data, avpkt->size); |
There was a problem hiding this comment.
Is this directly the packet buffer we pass in via avcodec_decode_audio4? If so, we can control the buffer size there. Usually it points at PSP RAM, though... but it can also point to dataBuf_. Does padding that out help?
-[Unknown]
There was a problem hiding this comment.
Oh, I was so happy to have found the workaround that I forgot to track outwards to see if we control the buffer. Will have a look later today.
|
Replaced with hrydgard/ppsspp#15990 and its followup fix hrydgard/ppsspp@42b2b0a |
Seems the atrac3+ decoder isn't quite robust with its bitstream reading, it can end up reading out of bounds in some situations. We got lucky with memory layouts on previous versions of Android somehow, but on Android 12 with ARM64, not so much.
The crash was reported in hrydgard/ppsspp#15788 and happens at the end of decoding certain atrac3+ files.
I hope to soon build ffmpeg for Android through our regular cmake build so we won't have to check in binaries anymore, but we're not there yet and this is a critical issue, so here we go, let's go for binary bloat for the moment.
The size 1024 of the additional memory was completely arbitrarily chosen, but works.
I made an additional branch where I tried to fix the bitstream lookahead, but it's just too much stuff to check. For the interested, that's 5cfbff9 .