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

[build/ci] Enable native code analysis nightly #6420

Merged
merged 6 commits into from
Oct 26, 2021
Merged

Conversation

pjcollins
Copy link
Member

@pjcollins pjcollins commented Oct 22, 2021

Context: aaa37c3

Updates the build to conditionally generate extra native libraries with
the ASAN and UBSAN runtime sanitizers when
'$(EnableNativeAnalyzers)' == 'true'.

We will only enable this behavior in our nightly builds, and the
*-checked+asan and *-checked+ubsa native libraries have been removed
from our regular installers.

Context: aaa37c3

Updates the build to conditionally run `<ClangTidyCheck/>` and build
extra native libraries with the ASAN and UBSAN runtime sanitizers when
`'$(EnableNativeAnalyzers)' == 'true'`.

We will only enable this behavior in our nightly builds, and the
`*-checked+asan` and `*-checked+ubsa` native libraries have been removed
from our regular installers.  Nightly tests have been updated to run
against these `*-checked+*` native libraries.
@pjcollins
Copy link
Member Author

@grendello please take a look at the general approach here when you get the chance. I am assuming we want to be running this ClangTidyCheck target during the build at some point, and also want to run some tests with /p:UseASAN=true and /p:UseUBSAN=true? Is there anything extra we need to do to validate test results that ran against these "checked" binaries?

@pjcollins
Copy link
Member Author

Nightly run triggered here to test initial changes - https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5362029&view=results

@grendello
Copy link
Contributor

@grendello please take a look at the general approach here when you get the chance. I am assuming we want to be running this ClangTidyCheck target during the build at some point, and also want to run some tests with /p:UseASAN=true and /p:UseUBSAN=true? Is there anything extra we need to do to validate test results that ran against these "checked" binaries?

Yes, it would be good to run tests with these sanitizers enabled. They should run as usual, but we must capture stdout and stderr, as that's where the compiler will show any issues (and logcat, of course - it might be good to run adb logcat -G 16M before running these tests)

@pjcollins
Copy link
Member Author

@grendello clang-tidy looks like it's failing for all architectures, here's one log: https://gist.github.com/pjcollins/b1aeefaddea98cdbd78505b12d402cd6

@pjcollins
Copy link
Member Author

@grendello The tests which had support for running against these checked binaries appear to be crashing from this test run:
logcat-Release-Xamarin.Forms_Performance_Integration.txt
logcat-Release-Mono.Android_Tests.txt

Do any of the crash dumps look meaningful to you?

@grendello
Copy link
Contributor

@grendello clang-tidy looks like it's failing for all architectures, here's one log: https://gist.github.com/pjcollins/b1aeefaddea98cdbd78505b12d402cd6

It appears it's getting confused by the macros. I need to run this locally and add exceptions to the code

@grendello
Copy link
Contributor

@grendello The tests which had support for running against these checked binaries appear to be crashing from this test run: logcat-Release-Xamarin.Forms_Performance_Integration.txt logcat-Release-Mono.Android_Tests.txt

Do any of the crash dumps look meaningful to you?

They appear to have spotted an actual bug, but I can't tell where that is exactly as there are no symbols. I didn't find any APKs in the artifacts, do you think we could have them stored? With the apk we might be able to translate addresses to actual source lines.

@grendello
Copy link
Contributor

@pjcollins Can you try applying this diff? It should leave debug symbols intact when building the sanitized runtimes:

diff --git a/src/monodroid/CMakeLists.txt b/src/monodroid/CMakeLists.txt
index f1aa3fc91..bf7aabf4a 100644
--- a/src/monodroid/CMakeLists.txt
+++ b/src/monodroid/CMakeLists.txt
@@ -35,9 +35,16 @@ endif()

 option(ENABLE_CLANG_ASAN "Enable the clang AddressSanitizer support" OFF)
 option(ENABLE_CLANG_UBSAN "Enable the clang UndefinedBehaviorSanitizer support" OFF)
+
+if(ENABLE_CLANG_ASAN OR ENABLE_CLANG_UBSAN)
+  set(STRIP_DEBUG_DEFAULT OFF)
+else()
+  set(STRIP_DEBUG_DEFAULT ON)
+endif()
+
 option(ENABLE_NET6 "Enable compilation for .NET6" OFF)
 option(ENABLE_TIMING "Build with timing support" OFF)
-option(STRIP_DEBUG "Strip debugging information when linking" ON)
+option(STRIP_DEBUG "Strip debugging information when linking" ${STRIP_DEBUG_DEFAULT})
 option(DISABLE_DEBUG "Disable the built-in debugging code" OFF)
 option(USE_CCACHE "Use ccache, if found, to speed up recompilation" ${CCACHE_OPTION_DEFAULT})

@pjcollins
Copy link
Member Author

@pjcollins
Copy link
Member Author

@grendello the test run above has .apk files and should include your debug symbol diff when you have a chance to take a look.

@pjcollins pjcollins changed the title [build/ci] Run native code analyzer tests nightly [build/ci] Enable native code analysis nightly Oct 26, 2021
@pjcollins pjcollins marked this pull request as ready for review October 26, 2021 17:04
@pjcollins
Copy link
Member Author

pjcollins commented Oct 26, 2021

I filed #6433 and #6434 for the issues we've seen so far. I think we can merge these changes to remove native analysis pieces from our regular build, and incrementally fix the other issues in future PRs.

grendello added a commit to grendello/xamarin-android that referenced this pull request Oct 26, 2021
Context: dotnet#6420 (comment)

Clang's AddressSanitizer detected the following:

    10-26 15:55:25.393  2488  2488 I Mono.Android_Tests: ==2488==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x8a600774 at pc 0xaeee9982 bp 0xbf98dc68 sp 0xbf98dc60
    10-26 15:55:25.394  2488  2488 I Mono.Android_Tests: WRITE of size 4 at 0x8a600774 thread T0
    10-26 15:55:25.398  2488  2488 I Mono.Android_Tests:     #0 0xaeee9981  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x38981)
    10-26 15:55:25.398  2488  2488 I Mono.Android_Tests:     #1 0xaeef92d9  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x482d9)
    10-26 15:55:25.398  2488  2488 I Mono.Android_Tests:     #2 0xaef009ae  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x4f9ae)
    10-26 15:55:25.398  2488  2488 I Mono.Android_Tests:     #3 0xaef06d14  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x55d14)
    10-26 15:55:25.399  2488  2488 I Mono.Android_Tests: 0x8a600774 is located 0 bytes to the right of 4-byte region [0x8a600770,0x8a600774)
    10-26 15:55:25.399  2488  2488 I Mono.Android_Tests: allocated by thread T0 here:
    10-26 15:55:25.399  2488  2488 I Mono.Android_Tests:     #0 0xaedbe925  (/data/app/Mono.Android_Tests-1/lib/x86/libclang_rt.asan-i686-android.so+0xb6925)
    10-26 15:55:25.399  2488  2488 I Mono.Android_Tests:     #1 0xaeee9ae1  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x38ae1)
    10-26 15:55:25.399  2488  2488 I Mono.Android_Tests:     #2 0xaeee9751  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x38751)
    10-26 15:55:25.399  2488  2488 I Mono.Android_Tests:     #3 0xaeef92d9  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x482d9)
    10-26 15:55:25.399  2488  2488 I Mono.Android_Tests:     dotnet#4 0xaef009ae  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x4f9ae)
    10-26 15:55:25.400  2488  2488 I Mono.Android_Tests:     dotnet#5 0xaef06d14  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x55d14)
    10-26 15:55:25.400  2488  2488 I Mono.Android_Tests:     dotnet#6 0xb30cb970  (/data/dalvik-cache/x86/data@app@Mono.Android_Tests-1@base.apk@classes.dex+0x5c970)
    10-26 15:55:25.400  2488  2488 I Mono.Android_Tests: SUMMARY: AddressSanitizer: heap-buffer-overflow (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x38981)

Address of the offending region points to
`BasicUtilities::monodroid_strsplit` and is likely the line modified in
this commit.  Append terminating `nullptr` to vector instead of
overwriting the last element.
@pjcollins pjcollins merged commit f9d86ae into main Oct 26, 2021
@pjcollins pjcollins deleted the dev/pjc/nightly-checked branch October 26, 2021 22:26
jonpryor pushed a commit that referenced this pull request Oct 29, 2021
Context: #6420 (comment)

Clang's AddressSanitizer detected the following:

	Mono.Android_Tests: ==2488==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x8a600774 at pc 0xaeee9982 bp 0xbf98dc68 sp 0xbf98dc60
	Mono.Android_Tests: WRITE of size 4 at 0x8a600774 thread T0
	Mono.Android_Tests:     #0 0xaeee9981  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x38981)
	Mono.Android_Tests:     #1 0xaeef92d9  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x482d9)
	Mono.Android_Tests:     #2 0xaef009ae  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x4f9ae)
	Mono.Android_Tests:     #3 0xaef06d14  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x55d14)
	Mono.Android_Tests: 0x8a600774 is located 0 bytes to the right of 4-byte region [0x8a600770,0x8a600774)
	Mono.Android_Tests: allocated by thread T0 here:
	Mono.Android_Tests:     #0 0xaedbe925  (/data/app/Mono.Android_Tests-1/lib/x86/libclang_rt.asan-i686-android.so+0xb6925)
	Mono.Android_Tests:     #1 0xaeee9ae1  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x38ae1)
	Mono.Android_Tests:     #2 0xaeee9751  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x38751)
	Mono.Android_Tests:     #3 0xaeef92d9  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x482d9)
	Mono.Android_Tests:     #4 0xaef009ae  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x4f9ae)
	Mono.Android_Tests:     #5 0xaef06d14  (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x55d14)
	Mono.Android_Tests:     #6 0xb30cb970  (/data/dalvik-cache/x86/data@app@Mono.Android_Tests-1@base.apk@classes.dex+0x5c970)
	Mono.Android_Tests: SUMMARY: AddressSanitizer: heap-buffer-overflow (/data/app/Mono.Android_Tests-1/lib/x86/libmonodroid.so+0x38981)

Address of the offending region points to
`BasicUtilities::monodroid_strsplit()` and is likely the line
modified in this commit.  Append terminating `nullptr` to `vector`
instead of overwriting the last element.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants