Test framework#116
Open
wrp wants to merge 64 commits into
Open
Conversation
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. |
wrp
force-pushed
the
test-framework
branch
2 times, most recently
from
c73266f to
3b66baa
Compare
Contributor
|
All CI workflows are passing now. LGTM. |
esabol
approved these changes
Jan 20, 2026
Contributor
Author
|
Rebased on d5d4888 |
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.
fits_hdecompress was modified to take an additional argument, and this commit adds that new argument to the tests.
krutkow
requested changes
May 28, 2026
| #AC_CONFIG_HEADERS(config.h) | ||
| AC_CONFIG_SRCDIR([fitscore.c]) | ||
| AM_INIT_AUTOMAKE([foreign]) | ||
| AM_SILENT_RULES([yes]) |
Collaborator
There was a problem hiding this comment.
This edits the output of the make system and is unrelated to testing. Please remove. (and update the generated configure and Makefile.in files)
|
|
||
| # Distribution: ========================================================== | ||
|
|
||
| DISTCHECK_CONFIGURE_FLAGS = --enable-reentrant |
Collaborator
There was a problem hiding this comment.
This is unrelated to the test framework and conflicts/is redundant with our existing CI, which already tests reentrant. Please remove.
Collaborator
|
now that PR133 is merged, you can merge develop into this PR and remove your fits_hdecompress.c commit from this PR. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Note that, other than trivial changes to Makefile.am and configure.ac (and the subsequent changes to the autogenerated Makefile.in and configure) and a change to comments in fits_hdcompress.c, this PR is just adding files in tests/. Although the PR is large, it does not modify the existing code in any significant way.