Test framework #116
Open
Test framework #116
+61,464
−38
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wrp
force-pushed
the
test-framework
branch
2 times, most recently
from
15514cb to
1866a9e
Compare
Contributor
|
This is a huge contribution. Unit tests have long been much needed. Thank you! The CI is currently failing with: |
Contributor
I recommend opening separate issues for these in https://github.com/HEASARC/cfitsio/issues/ I would then delete the ISSUES file from the pull request. |
Contributor
Author
|
Removed ISSUES and created issues 117-119. PTAL |
Contributor
Author
|
Checking the test failures on ubuntu. |
The pseudo-random number generator in test_rcomp_high_entropy() used
signed integer arithmetic that overflows when i >= 2:
original[i] = ((i * 1103515245 + 12345) >> 16) & 0x7FFF;
The multiplication i * 1103515245 exceeds INT_MAX (2147483647) at i=2,
producing 2207030490 which cannot be represented as a signed int. This
is undefined behavior per the C standard (C11 6.5/5).
GCC on Ubuntu ARM with -O2 exploits this UB via aggressive loop
optimizations (-Waggressive-loop-optimizations), causing the test
to abort with a core dump during distcheck. The compiler warning:
warning: iteration 2 invokes undefined behavior
The fix uses unsigned arithmetic throughout:
original[i] = (((unsigned)i * 1103515245U + 12345U) >> 16) & 0x7FFF;
This produces identical pseudo-random values but with well-defined
overflow semantics (modulo 2^32 per C11 6.2.5/9).
The fits_rcomp, fits_rcomp_short, and fits_rcomp_byte functions do not
perform end-of-buffer checking. As documented in ricecomp.c:
"Note that beginning with CFITSIO v3.08, EOB checking was removed
to improve speed, and so now the input compressed bytes buffers
must have been allocated big enough so that they will never be
overflowed."
The removed tests passed intentionally undersized buffers expecting
the library to detect this and return an error. Instead, the library
writes past the buffer bounds, causing stack corruption. This was
detected on Linux by FORTIFY_SOURCE/glibc, triggering SIGABRT.
The test_rdecomp_buffer_too_small test is retained since decompression
reads from the buffer (bounded by clen parameter) rather than writing.
Contributor
|
All CI workflows are passing now. LGTM. |
esabol
approved these changes
Jan 20, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a simplistic test framework. testprog only exercises about 16% of the code (as reported by gcov), and 22 files have zero coverage. This adds some basic unit testing to exercise a larger chunk of the code base to protect against regressions.
A few issues discovered while developing the tests are mentioned in tests/ISSUES