Unit test framework pkg enable sanitize #6408

mdkinney · 2024-11-06T01:46:19Z

Description

UnitTestFrameworkPkg: Use /MTd and enable Address Sanitizers

Update host based unit test VS20xx builds to use /MTd instead of
/MT to enable use of debug libraries for host based unit tests.
Enable /fsanitize=address for host based unit test VS2019 builds
Enable /fsanitize=address for host based unit test VS2022 builds
Enable -fsanitize=address for host based unit test GCC builds
Add UNIT_TESTING_ADDRESS_SANITIZER_ENABLE define that is set to
TRUE by default so it is always enabled, but can be set to FALSE
to temporarily disable during development/debug of unit tests.

Enabling the Address Sanitizer can detect double frees, buffer
overflow, buffer underflow, access to invalid addresses, and
various exceptions. These can be detected in both the unit test
case sources as well as the code under test.

Breaking change?
- Breaking change - Does this PR cause a break in build or boot behavior?
- Examples: Does it add a new library class or move a module to a different repo.
Impacts security?
- Security - Does this PR have a direct security impact?
- Examples: Crypto algorithm change or buffer overflow fix.
Includes tests?
- Tests - Does this PR include any explicit test code?
- Examples: Unit tests or integration tests.

How This Was Tested

Integration Instructions

The environment variable GTEST_CATCH_EXCEPTION must be set to 0 for so all exceptions are handled by the address sanitizer and got GoogleTest. This allows stack back trace and other details to be logged by the address sanitizer so the source of the issue identified address sanitizer can be determined.

The environment variable ASAN_OPTIONS must be set to detect_leaks=0 to disable memory leak detection. The unit test frameworks may have memory leaks and some firmware code under test use cases may perform a memory allocation without a matching memory free as their expected behavior.

Windows

set GTEST_CATCH_EXCEPTIONS=0
set ASAN_OPTIONS=detect_leaks=0

Linux

export GTEST_CATCH_EXCEPTIONS=0
export ASAN_OPTIONS=detect_leaks=0

mdkinney · 2024-11-06T02:04:15Z

@heatd @mhaeuser @shijunjing please provide feedback on unconditionally adding address sanitizer to all host based unit tests.

There are email discussion about ASAN, UBSAN, and fuzzing and this would be first step along that path.

mhaeuser · 2024-11-06T13:53:50Z

...meworkPkg/Test/UnitTest/Sample/SampleUnitTestBufferUnderflow/SampleUnitTestBufferUnderflow.c

+
+  Pointer = (UINT8 *)AllocatePool (100);
+  UT_ASSERT_NOT_NULL (Pointer);
+  Pointer[110] = 0;


This is the same test as for Overflow.

good catch. I will fix on next update

mhaeuser · 2024-11-06T13:55:26Z

...rameworkPkg/Test/UnitTest/Sample/SampleUnitTestInvalidAddress/SampleUnitTestInvalidAddress.c

+  IN UNIT_TEST_CONTEXT  Context
+  )
+{
+  *(UINT8 *)(NULL) = 0;


Due to various quirks around the 0-address, I think "null dereference" and "invalid dereference" should be two entirely distinct concepts.

I agree, The will change this one to match google test death test on access to address (-1) and will add a new unit tests that does NULL pointer dereference.

I agree, The will change this one to match google test death test on access to address (-1) and will add a new unit tests that does NULL pointer dereference.

you can't actually reliably do NULL pointer dereference in C without workarounds. Here's clang generating a completely empty function (not even a RET!) because of UB: https://godbolt.org/z/dGzj893e6

Yes. Adding these types of unit tests to verify the ability to catch developer errors are challenging. I would like to see CLANG used for host based unit testing in the future, but right now, the only tool change enabled are VS20xx and GCC.

I'm not sure this is challenging, you "simply" need to employ volatile for such cases. Somehow I missed this, thanks @heatd!

Since we compile unit tests with all optimizations disabled, I am not sure of even volatile is required. But can always add volatile as needed for compiler compatibity.

Here is what is see from VS2019 for the statement *(UINT8 *)(NULL) = 0;

Sample Unit Test Sanitize NULL Pointer v0.1 --------------------------------------------------------- ------------ RUNNING ALL TEST SUITES -------------- --------------------------------------------------------- --------------------------------------------------------- RUNNING TEST SUITE: Sanitize tests --------------------------------------------------------- [==========] Running 1 test(s). [ RUN ] Sanitize NULL Address ================================================================= ==15628==ERROR: AddressSanitizer: access-violation on unknown address 0x000000000000 (pc 0x7ff79c2215d1 bp 0x000000000000 sp 0x0027c54feb60 T0) ==15628==The signal is caused by a WRITE memory access. ==15628==Hint: address points to the zero page. #0 0x7ff79c2215d0 in SanitizeNullAddress c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Test\UnitTest\Sample\SampleUnitTestNullAddress\SampleUnitTestNullAddress.c:40 #1 0x7ff79c25641d in CmockaUnitTestFunctionRunner c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Library\UnitTestLib\RunTestsCmocka.c:56 #2 0x7ff79c251aa1 in cmocka_run_one_test_or_fixture c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Library\CmockaLib\cmocka\src\cmocka.c:2890 #3 0x7ff79c251f8d in cmocka_run_one_tests c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Library\CmockaLib\cmocka\src\cmocka.c:2998 #4 0x7ff79c24a49b in _cmocka_run_group_tests c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Library\CmockaLib\cmocka\src\cmocka.c:3129 #5 0x7ff79c257594 in RunTestSuite c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Library\UnitTestLib\RunTestsCmocka.c:220 #6 0x7ff79c256f10 in RunAllTestSuites c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Library\UnitTestLib\RunTestsCmocka.c:276 #7 0x7ff79c221899 in UefiTestMain c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Test\UnitTest\Sample\SampleUnitTestNullAddress\SampleUnitTestNullAddress.c:90 #8 0x7ff79c221981 in main c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Test\UnitTest\Sample\SampleUnitTestNullAddress\SampleUnitTestNullAddress.c:109 #9 0x7ff79c2abce8 in invoke_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78 #10 0x7ff79c2abc3d in __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288 #11 0x7ff79c2abafd in __scrt_common_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:330 #12 0x7ff79c2abd5d in mainCRTStartup D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp:16 #13 0x7ffe7cd4257c (C:\windows\System32\KERNEL32.DLL+0x18001257c) #14 0x7ffe7de8af07 (C:\windows\SYSTEM32\ntdll.dll+0x18005af07) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: access-violation c:\work\github\tianocore\edk2\UnitTestFrameworkPkg\Test\UnitTest\Sample\SampleUnitTestNullAddress\SampleUnitTestNullAddress.c:40 in SanitizeNullAddress ==15628==ABORTING

mhaeuser · 2024-11-06T13:56:56Z

...rameworkPkg/Test/UnitTest/Sample/SampleUnitTestInvalidAddress/SampleUnitTestInvalidAddress.c

+  Status = RunAllTestSuites (Framework);
+
+EXIT:
+  if (Framework) {


Don't the same rules apply as for EDK II code, where "implicit" comparisons are forbidden?

Yes. Pointer should check against NULL. Only BOOLEAN variable can be used this way. Unfortunately this pattern is widely used in unit tests. I will fix in a different PR

mhaeuser · 2024-11-06T14:12:20Z

UnitTestFrameworkPkg/Library/UnitTestLib/UnitTestLib.c

+  // TODO: Finish this function.
+  if (SuiteEntry) {
+    TestCase = (UNIT_TEST_LIST_ENTRY *)GetFirstNode (&(SuiteEntry->UTS.TestCaseList));
+    while ((LIST_ENTRY *)TestCase != &(SuiteEntry->UTS.TestCaseList)) {


The double-casts and the manual comparison look very weird, should this not use (BASE_)CR and functions like IsNull()?

heatd · 2024-11-07T18:49:31Z

@heatd @mhaeuser @shijunjing please provide feedback on unconditionally adding address sanitizer to all host based unit tests.

There are email discussion about ASAN, UBSAN, and fuzzing and this would be first step along that path.

https://edk2.groups.io/g/devel/message/89684

https://edk2.groups.io/g/devel/message/88133

https://edk2.groups.io/g/devel/message/86933

You shouldn't enable ASAN unconditionally, because various sanitizers are incompatible with each other. For instance, clang supports MSAN which can catch uninitialized memory reads (also TSAN, but concurrency is nonexistent in UEFI).

I didn't look too much at the code but otherwise SGTM.
Also consider that the OpenCore folks have their own test harness and fuzzing harness they used on Ext4Dxe (and other components, I believe) to great effect.

I also think that doing ASAN in an actual firmware environment could be very beneficial for catching bugs, which otherwise you're not catching (particularly considering that very little EFI code has unit tests).

mdkinney · 2024-11-07T19:02:53Z

@heatd I am aware that some sanitizers can not be combined. I think I will be valuable to enable more sanitizers over time, but that will have to be balanced against the CI time if unit tests have to be build and run with multiple configurations. More investigation is needed. I am suggesting in this draft PR that address sanitization always be enabled when running edk2 host-based unit tests and we can handle sanitizer compatibilities as they are enabled later.

I agree that this feature only provide real value if the unit tests start to cover large amounts of FW sources. We should focus efforts on this gap.

Enabling sanitizers in target FW environments is very challenging, would require significant development, support, and maintenance. It would also always be behind the latest features on sanitizers targeted for OS applications.

mdkinney · 2024-11-07T19:04:04Z

I have pushed an update that should address some of the CI failures and some of the feedback from @mhaeuser

heatd · 2024-11-07T19:22:19Z

@heatd I am aware that some sanitizers can not be combined. I think I will be valuable to enable more sanitizers over time, but that will have to be balanced against the CI time if unit tests have to be build and run with multiple configurations. More investigation is needed. I am suggesting in this draft PR that address sanitization always be enabled when running edk2 host-based unit tests and we can handle sanitizer compatibilities as they are enabled later.

OK.

I agree that this feature only provide real value if the unit tests start to cover large amounts of FW sources. We should focus efforts on this gap.

Right. Feasibility of that testing goal aside, I seriously suspect unit tests can't carry nearly as far as a real workload.

Enabling sanitizers in target FW environments is very challenging, would require significant development, support, and maintenance. It would also always be behind the latest features on sanitizers targeted for OS applications.

FWIW, I suspect it wouldn't be that hard to do this. Basically you'd need to reserve 1/8th of memory in PEI to be set up as shadow memory in DxeCore (I think?). Steven Shi IIRC had a nice sample implementation of this that I remember fairly impressed me at the time. You'd also probably want to add a heap quarantine a-la ASAN malloc, but that's also not super hard.

Also, these interfaces are quite stable. ASAN doesn't really have new features.

In any case, these are just things folks should consider; I'm certainly not enlisting to do any of this work anymore :). Having any kind of ASAN is already a good improvement.

mdkinney · 2024-11-07T19:27:16Z

Right. Feasibility of that testing goal aside, I seriously suspect unit tests can't carry nearly as far as a real workload.

Actually unit tests can cover corner cases that are very hard or impossible to reproduce on real targets. Mostly around error paths for out of memory and error paths for hardware errors/failures. Host based unit testing can use mocks to and code coverage to help ensure all code paths are tested.

mhaeuser · 2024-11-07T21:14:48Z

Actually unit tests can cover corner cases that are very hard or impossible to reproduce on real targets. Mostly around error paths for out of memory and error paths for hardware errors/failures. Host based unit testing can use mocks to and code coverage to help ensure all code paths are tested.

I have to say, despite being a fan of automated tests, I have never been a fan of classical unit tests, let alone test-driven development (though I acknowledge its advantages). This is less about "this is useless", it is not, but more about "this is very time-consuming and prone to errors that effectively risk making it useless". People tend to think of the typical corner-cases that are already prominent during development, but miss out on the finer details (this obviously includes myself).

My personal approach has been to utilize fuzz-testing using fault injection (e.g. our AUDK userland fuzzing allocator is shimmed to randomly inject allocation faults to test exactly these OOM paths) as much as possible, and when there is a bug, I save the bugged input for regression-testing. To make this somewhat scale, I wrote multiple scripts to better handle corpus-based testing (heavily PoC): https://github.com/mhaeuser/MastersThesis/tree/ue_test/Scripts/Corpus

Examples of this include:

ExtendConfig: The fuzzing environment configuration is handled by a trailer of a fixed structure. This includes things like the allocation failure modulus (when it holds that NumAllocs % AllocMod == AllocMod - 1, the allocation is faulted). The script helps updating the corpus (and any stored inputs for regression- or unit-testing) in case the config struct layout is updated.
FabricateAllocModulus: The idea here was to pre-generate allocation failures (by taking an otherwise valid input that would trigger all success code paths and continuously increasing the allocation failure threshold), so coverage-based fuzzing would easily pick up on generating allocation failures across various kinds of inputs.

If you give it enough valid inputs and enough ways to break the environment, the fuzzer will do its work, given enough time and RAM. I combined this with manually checking the coverage reports and fabricating faulty inputs for unreached error paths alike FabricateAllocModulus, to speed up fuzz-testing coverage and ensure lots of permutations. Throwing a fuzzing worker at the problem feels both safer and easier in most cases, rather than sitting down and thinking of all critical error conditions, risking to miss some important combination.

mdkinney · 2024-11-07T23:24:35Z

I agree with the potential value of fuzz testing and how it can compliment other testing methods. There are just better development and debug tools for host based unit tests that are OS applications than target FW envs, so I am interested in seeing how we can apply fuzz testing in host envs.

mdkinney · 2024-11-08T03:54:03Z

@VivianNK please review

mhaeuser · 2024-11-08T10:25:40Z

There are just better development and debug tools for host based unit tests that are OS applications than target FW envs, so I am interested in seeing how we can apply fuzz testing in host envs.

All our fuzz-testing is in host envs. I don’t think fuzz-testing in FW envs makes any sense (this is separate from sanitizers, which absolutely do make sense there!).

VivianNK · 2024-11-08T19:43:09Z

UnitTestFrameworkPkg/Library/UnitTestLib/UnitTestLib.c

+  IN UNIT_TEST_LIST_ENTRY  *TestEntry
+  )
+{
+  // TODO: Finish this function.


What's the goal in "finishing" the function?

Old comment that needs to be removed now that the function is implemented

Oh, that function is finished? Then the superfluous return type should be dropped, the result is discarded anyway.

VivianNK · 2024-11-08T19:45:12Z

UnitTestFrameworkPkg/Test/GoogleTest/Sample/SampleGoogleTest/SampleGoogleTest.cpp

+  ASSERT_NE (Pointer, (UINT8 *)NULL);
+  FreePool (Pointer);
+  //
+  // Second free that should be caught by address sanitizer, log details, and exit


It exits the entire GoogleTest or is it able to continue execution outside of this TEST() function? What does "exit" mean in this case?

These test cases verify that the address sanitizer does catch the issue and is verified with the use of EXPECT_DEATH() that allows the rest of the tests to be run.

If a developer actually has a double free in their unit test code or code under test, then the address sanitizer would catch the condition, log a stack back trace and other details, and terminate/exit the unit test application. This is the expected developer experience when a condition is caught by the address sanitizer. It would also generate an error for CI.

VivianNK · 2024-11-08T19:47:51Z

UnitTestFrameworkPkg/Test/GoogleTest/Sample/SampleGoogleTest/SampleGoogleTest.cpp

 int
 main (
  int   argc,
  char  *argv[]
  )
 {
  testing::InitGoogleTest (&argc, argv);
+  testing::FLAGS_gtest_catch_exceptions = 0;


Moving forward we should be adding this flag to any GoogleTest that uses EXPECT_DEATH?

This is a setting for the address sanitizer, not EXPECT_DEATH(). When GoogleTest is combined with the address sanitizer, all exceptions need to be handled by the address sanitizer so it can show details of the failure. This can be done globally with setting the env var GTEST_CATCH_EXCEPTIONS=0. I have done this env var setting in this PR for Azure Pipelines CI agents. And it is recommended that developers set this as well to always see full details.

I have updated the Integration Instruction in the description of this PR with those details. They also need to be added to the UnitTestFrameworkPkg ReadMe.

Add use of DEBUG_CLEAR_MEMORY() macros on all allocation and free operations in MemoryAllocationLibPosix to match behavior of all other MemoryAllocationLib instances. Signed-off-by: Michael D Kinney <[email protected]>

Implement FreeUnitTestEntry(), FreeUnitTestSuiteEntry(), and FreeUnitTestFramework() so the UnitTestLib does not introduce any memory leaks. Signed-off-by: Michael D Kinney <[email protected]>

Update DSC files to always enable DEBUG_CLEAR_MEMORY() macros so memory is cleared on every memory allocation/free operation. Signed-off-by: Michael D Kinney <[email protected]>

* Update host based unit test VS20xx builds to use /MTd instead of /MT to enable use of debug libraries for host based unit tests. * Enable /fsanitize=address for host based unit test VS2019 builds * Enable /fsanitize=address for host based unit test VS2022 builds * Enable -fsanitize=address for host based unit test GCC builds * Add UNIT_TESTING_ADDRESS_SANITIZER_ENABLE define that is set to TRUE by default so it is always enabled, but can be set to FALSE to temporarily disable during development/debug of unit tests. Enabling the Address Sanitizer can detect double frees, buffer overflow, buffer underflow, access to invalid addresses, and various exceptions. These can be detected in both the unit test case sources as well as the code under test. Signed-off-by: Michael D Kinney <[email protected]>

Add GoogleTest and Framework based unit tests that are expected to fail and be caught by Address Sanitizer. These unit tests verify that an address sanitizer is enabled and detecting the following conditions. It also provide examples of the expected output when an Address Sanitizer detected these types of issues. * double free * buffer overflow * buffer underflow * null ptr * invalid address * divide by zero Signed-off-by: Michael D Kinney <[email protected]>

Use snprintf() in host based unit tests to format log messages and add host specific wrapper for LongJump() that is decorated with NORETURN to provide hint to address sanitizer that LongJump() never returns. Signed-off-by: Michael D Kinney <[email protected]>

Signed-off-by: Michael D Kinney <[email protected]>

…ress Add HostMemoryAllocationBelowAddressLib class and implementation that uses OS specific services to perform pool and page allocations below a specified address in a host based unit test application execution environment. This library class is only required for mocking buffers that are assumed to be below a specific address by code under test. Signed-off-by: Michael D Kinney <[email protected]>

mdkinney requested review from ajfish and leiflindholm November 6, 2024 01:46

mhaeuser reviewed Nov 6, 2024

View reviewed changes

mdkinney force-pushed the UnitTestFrameworkPkg_EnableSanitize branch from 1e87ddd to 8e59990 Compare November 7, 2024 18:56

mdkinney force-pushed the UnitTestFrameworkPkg_EnableSanitize branch 3 times, most recently from 0519d7b to 1202caa Compare November 8, 2024 03:21

mdkinney requested review from spbrogan and makubacki November 8, 2024 03:32

This was referenced Nov 8, 2024

CryptoPkg/BaseCryptLib: Fix serial number read overrun #6418

Merged

UefiCpuPkg/MtrrLib: Fix unit test read overflow #6419

Merged

NetworkPkg/Dhcp6Dxe: Fix sanitizer issues #6420

Merged

MdePkg/Test/DevicePathLib: Remove FreePool(NULL) #6421

Merged

VivianNK reviewed Nov 8, 2024

View reviewed changes

VivianNK approved these changes Nov 8, 2024

View reviewed changes

mdkinney force-pushed the UnitTestFrameworkPkg_EnableSanitize branch 2 times, most recently from 7f79ea1 to b6d0f2b Compare November 14, 2024 02:43

mdkinney added 7 commits November 25, 2024 23:14

UnitTestFrameworkPkg/UnitTestLib: Implement Free*() services

74c3f9b

Implement FreeUnitTestEntry(), FreeUnitTestSuiteEntry(), and FreeUnitTestFramework() so the UnitTestLib does not introduce any memory leaks. Signed-off-by: Michael D Kinney <[email protected]>

UnitTestFraworkPkg: Enable DEBUG_CLEAR_MEMORY in host tests

f7a00cd

Update DSC files to always enable DEBUG_CLEAR_MEMORY() macros so memory is cleared on every memory allocation/free operation. Signed-off-by: Michael D Kinney <[email protected]>

.azurepipelines: Install ASAN libs and set ASAN options

2a5e5c1

Signed-off-by: Michael D Kinney <[email protected]>

mdkinney force-pushed the UnitTestFrameworkPkg_EnableSanitize branch 2 times, most recently from c6c374c to 22d1162 Compare November 26, 2024 15:31

mdkinney force-pushed the UnitTestFrameworkPkg_EnableSanitize branch from 22d1162 to 1ed15fd Compare November 26, 2024 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit test framework pkg enable sanitize #6408

Unit test framework pkg enable sanitize #6408

mdkinney commented Nov 6, 2024 •

edited

Loading

mdkinney commented Nov 6, 2024

mhaeuser Nov 6, 2024

mdkinney Nov 7, 2024

mhaeuser Nov 6, 2024

mdkinney Nov 7, 2024

heatd Nov 7, 2024

mdkinney Nov 7, 2024

mhaeuser Nov 7, 2024

mdkinney Nov 8, 2024

mhaeuser Nov 6, 2024

mdkinney Nov 7, 2024 •

edited

Loading

mhaeuser Nov 6, 2024

heatd commented Nov 7, 2024

mdkinney commented Nov 7, 2024

mdkinney commented Nov 7, 2024

heatd commented Nov 7, 2024

mdkinney commented Nov 7, 2024

mhaeuser commented Nov 7, 2024

mdkinney commented Nov 7, 2024

mdkinney commented Nov 8, 2024

mhaeuser commented Nov 8, 2024

VivianNK Nov 8, 2024

mdkinney Nov 8, 2024

mhaeuser Nov 8, 2024

VivianNK Nov 8, 2024 •

edited

Loading

mdkinney Nov 8, 2024

VivianNK Nov 8, 2024

mdkinney Nov 8, 2024

mdkinney Nov 8, 2024

Unit test framework pkg enable sanitize #6408

Are you sure you want to change the base?

Unit test framework pkg enable sanitize #6408

Conversation

mdkinney commented Nov 6, 2024 • edited Loading

Description

How This Was Tested

Integration Instructions

Windows

Linux

mdkinney commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdkinney Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heatd commented Nov 7, 2024

mdkinney commented Nov 7, 2024

mdkinney commented Nov 7, 2024

heatd commented Nov 7, 2024

mdkinney commented Nov 7, 2024

mhaeuser commented Nov 7, 2024

mdkinney commented Nov 7, 2024

mdkinney commented Nov 8, 2024

mhaeuser commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VivianNK Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdkinney commented Nov 6, 2024 •

edited

Loading

mdkinney Nov 7, 2024 •

edited

Loading

VivianNK Nov 8, 2024 •

edited

Loading