Guard summary keyword lookups#1239
Open
MagnusSletten wants to merge 6 commits into
Open
Conversation
MagnusSletten
force-pushed
the
fix-null-def
branch
2 times, most recently
from
629bfef to
4f6ec34
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens summary (SMSPEC/UNSMRY) parsing by adding explicit validation around keyword lookups and data payload types, aiming to replace null-dereferences/UB with clear, actionable failures discovered via fuzzing.
Changes:
- Add negative/invalid-case tests for malformed SMSPEC/UNSMRY structures (missing keywords, wrong payload types, fixed-width keyword edge case).
- Add validation checks in SMSPEC/UNSMRY loaders (keyword presence, header entries, payload type/unit presence) to fail fast with exceptions.
- Update fixed-width keyword string extraction to be width-bounded (no reliance on NUL terminators).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rd_tests/test_rd_sum.py | Adds regression tests for malformed SMSPEC/UNSMRY inputs and fixed-width keyword handling. |
| lib/resdata/smspec_node.cpp | Adds argument/grid-dimension validation when deriving IJK from NUMS for block/completion variables. |
| lib/resdata/rd_unsmry_loader.cpp | Adds PARAMS keyword/type validation and additional file-view entry/header checks. |
| lib/resdata/rd_smspec.cpp | Adds guards for required SMSPEC keywords and payload validation (e.g., STARTDAT int payload, TIME unit). |
| lib/resdata/rd_file_view.cpp | Adds a guard for missing keyword entries during indexed keyword loading. |
| lib/include/resdata/rd_kw.hpp | Changes stripped-string extraction to use fixed-width reads (string_view) instead of relying on NUL termination. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Check keyword lookup results before dereferencing them in summary readers. Missing or inconsistent keywords now fail with clear exceptions instead of causing null dereferences.
MagnusSletten
force-pushed
the
fix-null-def
branch
from
d7c48ff to
e1a1c77
Compare
MagnusSletten
force-pushed
the
fix-null-def
branch
from
e1a1c77 to
fc9dc12
Compare
MagnusSletten
force-pushed
the
fix-null-def
branch
from
fc9dc12 to
e5c1c4e
Compare
| const rd_kw_type *intehead = | ||
| rd_file_iget_named_kw(header.get(), INTEHEAD_KW, 0); | ||
| if (intehead == NULL) | ||
| throw std::invalid_argument( |
Collaborator
There was a problem hiding this comment.
This seems like a logic bug in how rd_file handles kw. It shouldn't ever say it "has" a keyword and then return NULL. Maybe we could have a closer look at how this happened if you still have the file that created this scenario?
| static_cast<const char *>(rd_kw_iget_ptr(kw, index))); | ||
| const char *raw = static_cast<const char *>(rd_kw_iget_ptr(kw, index)); | ||
| const size_t width = rd_type_get_sizeof_iotype(rd_kw_get_data_type(kw)); | ||
| size_t len = 0; |
Collaborator
There was a problem hiding this comment.
Could be nice with some unit-tests for this.
Contributor
Author
There was a problem hiding this comment.
Added a few now
eivindjahren
requested changes
Jul 10, 2026
eivindjahren
left a comment
eivindjahren
left a comment
Collaborator
There was a problem hiding this comment.
Had some small comments that are easily fixed :)
MagnusSletten
force-pushed
the
fix-null-def
branch
from
e5c1c4e to
cc97938
Compare
MagnusSletten
force-pushed
the
fix-null-def
branch
from
cc97938 to
511f4d9
Compare
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.
Check keyword lookup results before dereferencing them in summary readers. Missing or inconsistent keywords now fail with clear exceptions instead of causing null dereferences.
Approach
Errors found by fuzzing and it's verified that the corresponding error is fixed.