Phase 0 of #149: docs and guards, no behaviour change - #150
Merged
Conversation
Ships the first phase of the deep-audit plan of action from #149 -- "stop the bleeding": documentation fixes and defensive guards only, no change to any correct code path. - D1: fix dedx_get_program_list()/dedx_get_material_list()/ dedx_get_ion_list() @return docs, which claimed the arrays are 0-terminated when they are actually terminated by -1. - B2: dedx_internal_get_atom_charge()/get_atom_mass()/get_nucleon() in dedx_periodic_table.c only guarded the upper bound (id < 113), letting a caller-supplied id <= 0 (e.g. an unchecked custom-compound elements_id[] entry) index dedx_amu[]/dedx_nucl[] out of bounds. Both bounds are now checked in the lookup functions themselves. - B4: add DEDX_ERROR_STRING_MAX (64), documenting the minimum buffer size dedx_get_error_code() requires -- previously undocumented. - B5: NULL-check the calloc() in load_bethe_2(), the last unchecked allocation in the library. - B1: dedx_internal_validate_config() now rejects a config with elements_id set but no elements_mass_fraction (and nothing to derive it from) with DEDX_ERR_INCONSISTENT_COMPOUND. Previously a tabulated program (< 100) with a non-zero target and a custom elements_id list skipped both branches that populate elements_mass_fraction and reached load_compound()'s Bragg sum with a NULL weight array. - C5: dedx_get_error_code() is now table-driven instead of a switch, so a new DEDX_ERR_* code can no longer be added to dedx_error.h without a matching message -- which is exactly how DEDX_ERR_INCONSISTENT_COMPOUND (211) ended up with no message ("No such error code.") despite being a real, documented code. Regression tests are added for all five fixes: negative/zero element ids (test_validate_internal.c), the elements_id-without-weights NULL deref (test_error_codes.c), and a walk over every DEDX_ERR_* code asserting dedx_get_error_code() resolves it to a real message (test_error_codes.c). Verified each new test fails against the pre-fix code (segfault for B1, wrong err for B2, missing message for C5) and passes after the fix. Full suite (32/32) is green both in a plain Debug build and under -fsanitize=address,undefined.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
==========================================
+ Coverage 74.51% 78.22% +3.71%
==========================================
Files 12 12
Lines 1711 1649 -62
Branches 317 320 +3
==========================================
+ Hits 1275 1290 +15
+ Misses 436 359 -77 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The calloc() NULL check added for B5 in load_bethe_2() can't be exercised by tests (there's no portable way to force calloc() to fail), so Codecov flagged it as uncovered patch lines. The codebase already has a convention for this exact situation -- see the calloc failure guards in dedx_allocate_workspace() -- so apply the same LCOV_EXCL_START/STOP markers here instead of leaving it unmarked.
Contributor
There was a problem hiding this comment.
Pull request overview
Phase 0 implementation of issue #149 focused on documentation corrections and defensive guards to prevent API-reachable memory-safety failures, with accompanying regression tests.
Changes:
- Correct public-header documentation for
dedx_get_program_list(),dedx_get_material_list(), anddedx_get_ion_list()to reflect-1-terminated arrays. - Add/strengthen guard rails to prevent out-of-bounds periodic-table lookups, a NULL-deref in compound loading, and an unchecked allocation in the Bethe path.
- Make
dedx_get_error_code()table-driven (including a missing message forDEDX_ERR_INCONSISTENT_COMPOUND) and add tests ensuring every defined error code resolves to a real message.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_validate_internal.c | Adds regression coverage for rejecting non-positive periodic-table IDs while keeping valid IDs working. |
| tests/test_error_codes.c | Adds regression for the former NULL-deref path and verifies all DEDX_ERR_* codes map to real strings. |
| src/dedx.c | Replaces switch-based error string mapping with a table-driven approach; adds calloc NULL-check in load_bethe_2(). |
| src/dedx_validate.c | Adds validation guard to reject inconsistent configs that would otherwise dereference a NULL weights array. |
| src/dedx_periodic_table.c | Adds lower-bound checks to prevent out-of-bounds indexing for id <= 0. |
| include/dedx.h | Documents required buffer size for dedx_get_error_code() and fixes list-terminator docs to -1. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
grzanka
marked this pull request as ready for review
August 8, 2026 16:02
This was referenced Aug 8, 2026
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
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.
Implements Phase 0 — "stop the bleeding" of the plan of action in #149: documentation fixes and defensive guards only, no change to any already-correct code path. Everything below is copied from the Phase 0 checklist in #149.
Changes
D1 —
dedx_get_program_list()/dedx_get_material_list()/dedx_get_ion_list()were documented (@return) as returning a0-terminated array; they actually return a-1-terminated array (the implementation comments already agreed). Fixed the three doc lines. A consumer following the old docs would run off the end of every one of these arrays.B2 —
dedx_internal_get_atom_charge()/get_atom_mass()/get_nucleon()indedx_periodic_table.conly guarded the upper bound (id < 113). A caller-suppliedid <= 0(e.g. an unchecked custom-compoundelements_id[]entry, which is never range-checked upstream) indexeddedx_amu[]/dedx_nucl[]out of bounds. Both bounds are now checked directly in the lookup functions.B4 — Added
DEDX_ERROR_STRING_MAX(64) and documented it as the minimum buffer sizededx_get_error_code()requires; this was previously an undocumented "caller-allocated" buffer with no stated size.B5 — NULL-checked the
calloc()inload_bethe_2(), the last unchecked allocation left in the library.B1 —
dedx_internal_validate_config()now rejects a config withelements_idset but noelements_mass_fraction(and nothing to derive it from) withDEDX_ERR_INCONSISTENT_COMPOUND. Previously, a tabulated program (program < 100) with a non-zerotargetand a caller-suppliedelements_idlist skipped both branches that populateelements_mass_fraction, anddedx_load_config()reachedload_compound()'s Bragg sum with a NULL weight array — a NULL dereference reachable from a plain public-API call.C5 —
dedx_get_error_code()is now table-driven instead of aswitch, so adding a newDEDX_ERR_*code can no longer silently desync from its message. This is exactly howDEDX_ERR_INCONSISTENT_COMPOUND(211) ended up with no message ("No such error code."instead of a real description) despite being defined and documented indedx_error.h. The table now includes a message for it.Tests
Regression tests are added for all five guard/behavior fixes:
tests/test_validate_internal.c:dedx_internal_get_atom_charge/get_atom_mass/get_nucleonrejectid <= 0(B2), and the valid range is unaffected.tests/test_error_codes.c: the B1 NULL-deref reproducer now returnsDEDX_ERR_INCONSISTENT_COMPOUNDinstead of crashing; a new test walks everyDEDX_ERR_*code defined indedx_error.hand assertsdedx_get_error_code()resolves it to a real message (C5), plus checks the unknown-code fallback still works.I verified each new test actually catches the bug it targets by temporarily reverting each individual fix and re-running: B1's reverts to a segfault, B2's reverts to wrong error codes, C5's reverts to a missing message. All tests pass again with the fixes restored.
Full suite is green (32/32) both in a plain Debug build and under
-fsanitize=address,undefined, including a standalone build/run of theB1NULL-deref reproducer from the issue against the fixed library (confirmed no crash,err == DEDX_ERR_INCONSISTENT_COMPOUND).Scope
Deliberately excludes everything from Phase 1 onward (warning flags/CI sanitizer job, the A1/A2/A5 correctness fixes, API contract changes, etc.) — those are separate, larger changes per the issue's own suggested split.
Closes nothing on its own; #149 stays open for the remaining phases.
Generated by Claude Code