ndk icon indicating copy to clipboard operation
ndk copied to clipboard

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

Open zchrissirhcz opened this issue 3 years ago • 4 comments

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

zchrissirhcz avatar Mar 23 '22 09:03 zchrissirhcz

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.

eugenis avatar Mar 23 '22 23:03 eugenis

@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?

stephenhines avatar Mar 24 '22 05:03 stephenhines

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

zchrissirhcz avatar Mar 24 '22 05:03 zchrissirhcz

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.

zchrissirhcz avatar Mar 24 '22 06:03 zchrissirhcz