bytedance / bhook

:fire: ByteHook is an Android PLT hook library which supports armeabi-v7a, arm64-v8a, x86 and x86_64.
https://github.com/bytedance/bhook/tree/main/doc#readme
MIT License
2.07k stars 316 forks source link

fix: clang static analyzer warning #1

Closed ChasonTang closed 3 years ago

ChasonTang commented 3 years ago
> ~/Library/Android/sdk/ndk/21.4.7075529/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang --target=i686-none-linux-android16 --analyze --sysroot=~/Library/Android/sdk/ndk/21.4.7075529/toolchains/llvm/prebuilt/darwin-x86_64/sysroot -I~/Documents/bhook/bytehook/src/main/cpp -I~/Documents/bhook/bytehook/src/main/cpp/include --analyzer-output html -o ./static_analyze_report ~/Documents/bhook/bytehook/src/main/cpp/bh_elf_manager.c

include search path and sysroot should use canonical prefixes instead of ~

clang analyzer output as below

~/Documents/bhook/bytehook/src/main/cpp/bh_elf_manager.c:198:35: warning: 1st function call argument is an uninitialized value
            if(cb_next) cb_next = cb(copy_elfs[i], cb_arg);
                                  ^~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
caikelun commented 3 years ago

@ChasonTang The existing logic and read-write lock of this module can ensure that the problems you imagine will not occur. If it does happen, it should be fixed instead of adding such a test and assertion.

In addition, it is generally unacceptable to call assert directly in an SDK used online.

ChasonTang commented 3 years ago

Thanks for your review. It's true that it should be fixed when it does happen.But it is not test and assert.It is defensive programming.It is potentially result If you change the module logic that effect this code.Assert can alert it in Debug mode and become NO-Op in release mode(-DNDEBUG that will be made in NDK toolchain) Assert can still be used in online SDK. assert aborts the process, but is turned into a no-op when the program is compiled with -DNDEBUG. It should be only used to check for situations that "can't happen"