Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ubsan: not work fine in ARM/AArch64 runtime on Android #817

Closed
butterl opened this issue Jun 1, 2017 · 18 comments
Closed

ubsan: not work fine in ARM/AArch64 runtime on Android #817

butterl opened this issue Jun 1, 2017 · 18 comments

Comments

@butterl
Copy link

butterl commented Jun 1, 2017

Hi ,

I tried built a test app with

LOCAL_CLANG := true
LOCAL_UNDEFINED_SANITIZER := true
LOCAL_SANITIZE:= undefined
LOCAL_CFLAGS += -g -Wall -Werror -std=c++11 -O0 -fsanitize=undefined -fsanitize-undefined-trap-on-error -fsanitize-trap=undefined

#include <stdint.h>
#include <stdio.h>

int main() {
(void)(int8_t(0x7f) + int8_t(0x7f));
(void)(int16_t(0x3fff) + int16_t(0x4000));

int32_t k = 0x12345678;
k += 0x789abcde;
}

From the ASM code , the abort() trap is inserted but running on board(ARM64 Android Phone) , it just abort() and nothing more is printed :(

14:30:13.368 7198 7198 F DEBUG : backtrace:
14:30:13.369 7198 7198 F DEBUG : #00 pc 000000000006bc30 /system/lib64/libc.so (tgkill+8)
14:30:13.369 7198 7198 F DEBUG : #1 pc 00000000000690cc /system/lib64/libc.so (pthread_kill+64)
14:30:13.369 7198 7198 F DEBUG : #2 pc 0000000000023e68 /system/lib64/libc.so (raise+24)
14:30:13.369 7198 7198 F DEBUG : #3 pc 000000000001c8ec /system/lib64/libc.so (abort+52)
14:30:13.369 7198 7198 F DEBUG : #4 pc 00000000000006d8 /data/ubsan_test (main+112)
14:30:13.369 7198 7198 F DEBUG : #5 pc 000000000001a70c /system/lib64/libc.so (__libc_init+88)
14:30:13.369 7198 7198 F DEBUG : #6 pc 00000000000005c8 /data/ubsan_test (_start_main+80)

The detail is https://issuetracker.google.com/issues/38250996

Anyone know how to let the trap goto the ubsan_xx_handler and give out the "runtime error :xxx ” log
Or I'm missing something important in the build ?

BR,

Chao

@chefmax
Copy link

chefmax commented Jun 1, 2017

Do you run your test on Android or Linux board? What compiler version do you use?

@chefmax
Copy link

chefmax commented Jun 1, 2017

Anyone know how to let the trap goto the ubsan_xx_handler and give out the "runtime error :xxx ” log

You can try remove -fsanitize-undefined-trap-on-error -fsanitize-trap=undefined from your CFLAGS.

@butterl
Copy link
Author

butterl commented Jun 1, 2017

Do you run your test on Android or Linux board? What compiler version do you use?

@chefmax Thanks for your attention , I run it on my Android phone and using the AOSP compile tool chain , The compiler called in the build is prebuilts/clang/host/linux-x86/clang-3859424/bin/clang++

You can try remove -fsanitize-undefined-trap-on-error -fsanitize-trap=undefined from your CFLAGS.

I tried remove those flag and test on board again, the output is the same

@butterl butterl changed the title ubsan not work fine in ARM/AArch64 runtime ubsan: not work fine in ARM/AArch64 runtime on Android Jun 1, 2017
@butterl
Copy link
Author

butterl commented Jun 1, 2017

I tried build the test code with [https://gcc.godbolt.org/#](online compiler) using x86-64 clang 3.8.1 (dosen't have an ARM clang available on it )

Test code

int32_t k = 0x12345678;
k += 0x789abcde;

is compiled to asm ,and got __ubsan_handle_add_overflow in the end of trap

        mov     dword ptr [rbp - 8], 305419896
        mov     eax, dword ptr [rbp - 8]
        mov     ecx, eax
        add     ecx, 2023406814         // k += 0x789abcde
        seto    dl                                 // trap code
        xor     dl, -1                
        test    dl, 1
        mov     dword ptr [rbp - 20], eax # 4-byte Spill
        mov     dword ptr [rbp - 24], ecx # 4-byte Spill
        jne     .LBB0_6
        movabs  rax, .L__unnamed_3
        mov     ecx, 2023406814
        mov     edx, ecx
        mov     ecx, dword ptr [rbp - 20] # 4-byte Reload
        mov     esi, ecx
        mov     rdi, rax
        call    __ubsan_handle_add_overflow  <= Something like this is what we want on ARM/AArch64 
.LBB0_6:  
        mov     eax, dword ptr [rbp - 24] # 4-byte Reload
        mov     dword ptr [rbp - 8], eax
        mov     eax, dword ptr [rbp - 4]
        add     rsp, 32
        pop     rbp
        ret

So, does it mean that abort() is not what we want or it just something on ARM/AArch64 do the replacement after calling abort()?

@eugenis
Copy link
Contributor

eugenis commented Jun 1, 2017

This behavior is controlled by -f(no-)sanitize-trap flag. Android NDK build system inserts -fsanitize-trap for you by default. This can be changed by adding LOCAL_SANITIZE_DIAG=undefined or just -fno-sanitize-trap=undefined in compiler flags.

@butterl
Copy link
Author

butterl commented Jun 2, 2017

Hi eugenis,

Thanks for your attention!

  • I checked ninja file and find something in the single build cmd:
 ...
 -g -Wall -Werror -std=c++11 -O0 -fsanitize=undefined -fPIE -D_USING_LIBCXX \
 -fsanitize-trap=all -ftrap-function=abort -fno-sanitize-trap=address,thread \
...

After removing -fsanitize-trap=all -ftrap-function=abort and build with -S ,the __ubsan_handle_add_overflow is replacing abort() (cflag passing from build/core/config_sanitizers.mk)

  • I tried with your advise using :
 LOCAL_CLANG := true
 LOCAL_SANITIZE := undefined
LOCAL_SANITIZE_DIAG := undefined
LOCAL_CFLAGS += -g -Wall -Werror -std=gnu++11 -Wno-missing-field-initializers -O0 -fno-sanitize-trap=undefined

And I got error while linking,all the symbols are defined in code . Seems there being some static lib (sanitizer common ?) missing while linking , Maybe the abort() is workaround for these linking error?

external/compiler-rt/lib/ubsan/ubsan_diag.h:27: error: undefined reference to '__sanitizer::SymbolizedStack::ClearAll()'
external/compiler-rt/lib/ubsan/ubsan_handlers.cc:102: error: undefined reference to '__sanitizer::Die()'
external/compiler-rt/lib/ubsan/ubsan_handlers.cc:136: error: undefined reference to '__sanitizer::Die()'
external/compiler-rt/lib/ubsan/ubsan_handlers.cc:138: error: undefined reference to '__sanitizer::Die()'
external/compiler-rt/lib/ubsan/ubsan_handlers.cc:140: error: undefined reference to '__sanitizer::Die()'
external/compiler-rt/lib/ubsan/ubsan_handlers.cc:352: error: undefined reference to '__sanitizer::internal_memcpy(void*, void const*, unsigned long)'
external/compiler-rt/lib/ubsan/ubsan_diag.h:27: error: undefined reference to '__sanitizer::SymbolizedStack::ClearAll()'
external/compiler-rt/lib/ubsan/ubsan_handlers.cc:412: error: undefined reference to '__sanitizer::internal_strcmp(char const*, char const*)'
external/compiler-rt/lib/ubsan/ubsan_diag.h:27: error: undefined reference to '__sanitizer::SymbolizedStack::ClearAll()'
external/compiler-rt/lib/ubsan/ubsan_diag.h:27: error: undefined reference to '__sanitizer::SymbolizedStack::ClearAll()'
external/compiler-rt/lib/ubsan/ubsan_value.cc:92: error: undefined reference to '__sanitizer::internal_memcpy(void*, void const*, unsigned long)'
external/compiler-rt/lib/ubsan/ubsan_value.cc:98: error: undefined reference to '__sanitizer::internal_memcpy(void*, void const*, unsigned long)'
external/compiler-rt/lib/ubsan/ubsan_diag.cc:107: error: undefined reference to '__sanitizer::Symbolizer::GetOrInit()'
external/compiler-rt/lib/ubsan/ubsan_diag.cc:107: error: undefined reference to '__sanitizer::Symbolizer::SymbolizePC(unsigned long)'
external/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:35: error: undefined reference to '__sanitizer::CommonSanitizerReportMutex'
external/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:35: error: undefined reference to '__sanitizer::CommonSanitizerReportMutex'
external/compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.h:27: error: undefined reference to '__sanitizer::ColorizeReports()'
external/compiler-rt/lib/sanitizer_common/sanitizer_common.h:123: error: undefined reference to '__sanitizer::MmapOrDie(unsigned long, char const*, bool)'
external/compiler-rt/lib/ubsan/ubsan_diag.cc:341: error: undefined reference to '__sanitizer::InternalScopedString::append(char const*, ...)'
......

@butterl
Copy link
Author

butterl commented Jun 2, 2017

There's a workaround saying that the ubsan so generate are disabled unless soong has support to pick a module from prebuilts , so I open it and workaround to
get the ibclang_rt.ubsan_standalone-aarch64-android.so ,but some other libs seems still picked from the prebuilts (I don't find it ,maybe there is another workaround)

I tried on my phone with the patch below ,it already works with all the libs in prebuilts

diff --git a/core/config_sanitizers.mk b/core/config_sanitizers.mk
index 04aedf401..c3d07e3dd 100644
--- a/core/config_sanitizers.mk
+++ b/core/config_sanitizers.mk
@@ -219,12 +219,6 @@ ifneq ($(filter address,$(my_sanitize)),)
   endif
 endif

-ifneq ($(filter undefined,$(my_sanitize)),)
-  ifndef LOCAL_IS_HOST_MODULE
-    $(error ubsan is not yet supported on the target)
-  endif
-endif
-
 ifneq ($(strip $(LOCAL_SANITIZE_RECOVER)),)
   recover_arg := $(subst $(space),$(comma),$(LOCAL_SANITIZE_RECOVER)),
   my_cflags += -fsanitize-recover=$(recover_arg)  

For workaround this seems ok to work now. Thanks a lot for all these help .
If you got another good idea to fix it in the AOSP ,please let me know, I'm willing to help :)

@butterl
Copy link
Author

butterl commented Jun 2, 2017

Another question, I use androidmk to translate mk to bp ,and the target seems build without ubsan

cc_binary {
    name: "ubsan_test",
    clang: true,
    sanitize: {
        undefined: true,
        diag: {
            undefined: true,
        },
    },
    cflags: [
        "-g",
        "-Wall",
        "-Werror",
        "-std=gnu++11",
        "-Wno-missing-field-initializers",
        "-O0",
    ],

    srcs: ["add-overflow.cpp"],
}

I also triedmisc_undefined: ["xxx"] flag as in other bp files ,but still not work
Is there any special grammar for sanitizers setting in bp file ?

@eugenis
Copy link
Contributor

eugenis commented Jun 2, 2017 via email

@butterl
Copy link
Author

butterl commented Jun 3, 2017

cool, it works 👍

    clang: true,
    sanitize: {
        all_undefined: true,
        diag: {
            undefined: true,
        },
    },

And a workaround patch is also needed :

diff --git a/cc/sanitize.go b/cc/sanitize.go
index 8900ee8..b9a4dc8 100644
--- a/cc/sanitize.go
+++ b/cc/sanitize.go
@@ -255,9 +255,6 @@ func (sanitize *sanitize) flags(ctx ModuleContext, flags Flags) Flags {

        if Bool(sanitize.Properties.Sanitize.All_undefined) {
                sanitizers = append(sanitizers, "undefined")
-               if ctx.Device() {
-                       ctx.ModuleErrorf("ubsan is not yet supported on the device")
-               }
        } else {
                if Bool(sanitize.Properties.Sanitize.Undefined) {
                        sanitizers = append(sanitizers,

@butterl
Copy link
Author

butterl commented Jun 3, 2017

For ubsan now using the libs in prebuilts, is there a cmd to check for the UB checking list?

I 'm using an UB demo from a real bug in AOSP to test , but the UBsan seems unaware of it.
Below is the test code from the origin code

#include <stdio.h>
#include <stdint.h>

struct SizedBuffer {
    uint32_t length;
};

int read_from_buffer( uint8_t **buffer, uint8_t * end,
    SizedBuffer *target) {
    if (*buffer + sizeof(target->length) > end) return -1;

    *buffer += sizeof(target->length);
    if (target->length != 0) {
        const uint8_t *buffer_end = *buffer + target->length; // <= unsigned overflow is not UB in C  but ubsan is said  to have -fsanitize=unsigned-integer-overflow to catch it

        if ( buffer_end > end 
          || buffer_end <= *buffer )  // <= UB here when passing -O2 to clang ,the judgement is optimized away letting  huge length pass the check
            return -1;
        *buffer += target->length;
        // boom code when length is invalid removed here
    }
    return 0;
}
uint8_t g_data[50] = {0};
int main()
{
   SizedBuffer Test = {0};
   Test.length = 32;
   uint8_t *date_end = g_data + 40;
   uint8_t *date_start = g_data;
   
   read_from_buffer(&date_start,date_end,&Test);
   return 0;
}  

UBsan inserted trap code for buffer_end <= *buffer as below, and "type_mismatch" seems not what the UB is (ordered comparison of pointers in the same object/array from Stephen's comments in the bugfix page ).

...
        test    r12b, 1
        je      .LBB0_14
...
.LBB0_14:
        mov     edi, .L__unnamed_8
        mov     rsi, rbx
        call    __ubsan_handle_type_mismatch
        mov     eax, 1
        jmp     .LBB0_15

A quick comparison could be see from the excellent online compiler with -O2 -g -fsanitize=undefined and -O0 -g -fsanitize=undefined using the demo code

@eugenis
Copy link
Contributor

eugenis commented Jun 5, 2017 via email

@butterl
Copy link
Author

butterl commented Jun 7, 2017

Clang manual.
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html

I mean that listing the enabled checking from the runtime libs, Now I'm using the libs in prebuilts, not the fresh built ones.

Adding an unsigned offset to a pointer can not decrease the pointer value,
so the comparison is impossible and can be optimized out. This is because
pointer overflow is UB. UBsan reports it as

runtime error: pointer index expression with base XXX overflowed to YYY

Try replacing "undefined" with "all_undefined" in the "diag:" section.
Otherwise you get checks for all undefined behavior, but human-readable
diagnostics only for a small subset.

I tried with all_undefined in diag,

        diag: {
            all_undefined: true,
        },

and got error returned unrecognized property "sanitize.diag.all_undefined"

And I also I tried with example test case checking the error type you posted,

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
  // SAFE-NOT: runtime error
  // ERR: runtime error: pointer index expression with base {{.*}} overflowed to

  char *p = (char *)(UINTPTR_MAX);

  printf("%p\n", p + atoi(argv[1]));

  return 0;
}

with build command to test on host

prebuilts/clang/host/linux-x86/clang-3859424/bin/clang++ -g \
-fsanitize=undefined -fno-sanitize-trap=undefined -O2 pointer.cc

it came out
==8461==Sanitizer CHECK failed: external/compiler-rt/lib/ubsan/ubsan_init.cc:62 ((UBSAN_MODE_UNKNOWN)) != ((ubsan_mode)) (0, 0)

I also tried with a newer version ,but this come even without a sanitizer warning

clang version 5.0.0 (trunk 301384)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/bin

Does it mean the ubsan lib I'm using is not support the pointer check yet?

@eugenis
Copy link
Contributor

eugenis commented Jun 7, 2017 via email

@butterl
Copy link
Author

butterl commented Jun 9, 2017

Yes, you are right. Pointer overflow checking was added to LLVM less than a
month ago and it's not in Android yet.

Yes I see ,I built a clang trunk version ,and got __ubsan_handle_pointer_overflow in the host asm for the UB code line. 👍

        mov     edi, .L__unnamed_6
        mov     qword ptr [rsp], rsi    # 8-byte Spill
        mov     rsi, r12
        mov     rdx, rbx
        mov     rbp, r8
        call    __ubsan_handle_pointer_overflow
        mov     rsi, qword ptr [rsp]    # 8-byte Reload
        mov     r8, rbp
        jmp     .LBB0_11

Is there any good way to update to the newest Clang? Seems it‘s not that easy to just replace all bins in the prebuilts.

@eugenis
Copy link
Contributor

eugenis commented Jun 9, 2017 via email

@butterl
Copy link
Author

butterl commented Jul 31, 2017

Already submit change to AOSP to enable UBsan only check

@butterl
Copy link
Author

butterl commented Jul 31, 2017

@butterl butterl closed this as completed Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants