android / ndk

The Android Native Development Kit
1.99k stars 257 forks source link

ASan doesn't detect invalid read of heap memory in NEON Intrinsics #1687

Open zchrissirhcz opened 2 years ago

zchrissirhcz commented 2 years ago

Description

Using vld1_u8() intrinsic for out of bound data loading, if with Address Sanitizer, there should report invalid heap memory read, but actually no report generated.

The key code:

    uint8_t* Sp = (uint8_t*)malloc(1920);
    int xofsp[8] = {0, 1916};

    {
        uint8x8_t d0 = vld1_u8(Sp + xofsp[0]);
        uint8x8_t d1 = vld1_u8(Sp + xofsp[1]); //1916 + 8 = 1924, which is greater than 1920, should report invalid heap memory read.

        std::cout << d0 << std::endl;
        std::cout << d1 << std::endl;
    }

    free(Sp);

The full reproduce code: https://github.com/zchrissirhcz/min-repros/tree/master/test_asan_for_intrinsics

Affected versions

r23b, r25-beta2

Canary version

No response

Host OS

Linux

Host OS version

Ubuntu 20.04

Affected ABIs

armeabi-v7a, arm64-v8a

Build system

CMake

Other build system

No response

minSdkVersion

I don't know this..

Device API level

30

eugenis commented 2 years ago

The test case needs "volatile" in the declaration of Sp, otherwise this is a statically-obvious UB which gets compiled out before ASan can see it. This is not great, but hard to fix. With that, aarch64 and arm32 asan detect the bug.

NDK r21d has an apparent compiler bug on arm64 which marks the vector load as "align 8", allowing ASan to relax the check with the assumption that the address is 8 byte aligned, and makes it miss the bug. The bug is not present in NDK r23.

Also, all NDK versions show an issue in the Debug build flavor (-O0) where arm64 represents the vector load as an actual IR load <8 x i8>, <8 x i8>* %6, align 1, while arm32 keeps it as an intrinsic call <8 x i8> @llvm.arm.neon.vld1.v8i8.p0i8(i8* %5, i32 1) which is turned into a load in the backend. This results in a missed bug on arm32 with -O0. This could be fixed in ASan but I can't promise that we will get to it soon (arm32 is pretty low priority for us).

Thank you for the great test case. The build scripts made it very easy to look at.

stephenhines commented 2 years ago

@eugenis I see that https://github.com/zchrissirhcz/min-repros/blob/master/test_asan_for_intrinsics/build/android-arm64-build.sh says it is running on r23b. I see you said that the bug isn't present on NDK r23, but this seems odd. Did you mean that only the ARM32 one isn't present on r23?

zchrissirhcz commented 2 years ago

Hi, I add Apple M1 result now, which is my expected behavior of Android NDK, "located 3 bytes to the right of ..."

zchrissirhcz commented 2 years ago

Tried with android-ndk-r24 and android-ndk-r25-beta2.

android-ndk-r24 failed when calling cmake, reported in https://github.com/android/ndk/issues/1688 . android-ndk-r24 and android-ndk-r25-beta2 outputs same as android-ndk-r23b, i.e. not reporting Address Sanitizer invalid heap read.