Runtime Sanitizer Issues: Uninitialized structs in tests, and stack buffer read overflow

[nirosys]

Issue #, if available: None

Description of changes:
This PR is part of efforts to eliminate the runtime sanitizer errors reported when running in debug mode. Most of the issues were either related to uninitialized data in tests, or non-breaking issues with how the decNumber library is implemented. Specifically, this PR addresses uninitialized test data, and an off-by-one error in a loop that resulted in memory reads from beyond the boundaries of a buffer.

The memory access issue is identified by Clang's AddressSanitizer like so:

==3361==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffeffb60030 at pc 0x0000004e5f87 bp 0x7ffeffb5ff90 sp 0x7ffeffb5f758
READ of size 4 at 0x7ffeffb60030 thread T0
    #0 0x4e5f86 in __asan_memcpy (/home/runner/work/ion-c/ion-c/build/debug/test/all_tests+0x4e5f86)
    #1 0x7f08145fe426 in decQuadFMA /home/runner/work/ion-c/ion-c/decNumber/decBasic.c:2245:14
...
Address 0x7ffeffb60030 is located in stack of thread T0 at offset 144 in frame
    #0 0x7f08145f2ccf in decQuadFMA /home/runner/work/ion-c/ion-c/decNumber/decBasic.c:1998
  This frame has 6 object(s):
    [32, 144) 'acc' (line 2004) <== Memory access at offset 144 overflows this variable
    [176, 200) 'mul' (line 2006)
    [240, 264) 'fin' (line 2007)
    [304, 340) 'coe' (line 2008)
    [384, 388) 'uiwork' (line 2015)
    [400, 416) 'proxy' (line 2020)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (/home/runner/work/ion-c/ion-c/build/debug/test/all_tests+0x4e5f86) in __asan_memcpy

The original code consisted of a for loop that:

• walks over a buffer in 32bit increments
• memcpys every 32bit span into a 32bit int before checking to make sure the value is zero and stops if not.
• checks the boundary conditions for the loop to make sure the pointer is 4 or more bytes from the end and stops if not.
• increments the read pointer by 4bytes.

Because the boundary check is done after the memcpy, the memcpy ends up reaching beyond the size of the buffer. The data copied is only used as another stopping condition for the loop, and does not corrupt the digit data. It does however trigger the AddressSanitizer during debug build execution and could potentially cause issue with other dynamic analysis tools.

The change in this PR simply swaps the order of the bounds check and memcpy so the evaluation short circuits when memcpy would reach beyond the buffer. The already existing for-loop immediately after the change handles finishing the remainder of the pointer increments to account for non-multiple of 4 buffer sizes, in the same way it did pre-PR.

With this PR, the AddressSanitizer is no longer triggered:

~/Code/ion-c/build/debug rgiliam/tests_sanitizers ≡
❯ ./test/all_tests 2>&1 1>/dev/null | fgrep AddressSanitizer

~/Code/ion-c/build/debug rgiliam/tests_sanitizers ≡
❯

and unit tests still pass as expected:

[----------] Global test environment tear-down
[==========] 2957 tests from 41 test cases ran. (2655 ms total)
[  PASSED  ] 2957 tests.

The uninitialized options in tests triggered the UndefinedBehavior sanitizer, which presented with these:

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/runner/work/ion-c/ion-c/ionc/ion_extractor.c:70:72 in 
/home/runner/work/ion-c/ion-c/ionc/ion_extractor.c:69:70: runtime error: load of value 16, which is not a valid value for type 'bool'

These were being triggered by two uninitialized structs, but I've added initialization for all structs that were not already initialized at declaration, or memset prior to use since it is pretty dependent on what is in memory at the time of the test.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[tgregg]

Complex problems, simple solutions. Nice!

[zslayton]

Nice work!