fix: validate row type byte before enum cast in MPS parser#1491
Open
ramakrishnap-nv wants to merge 1 commit into
Open
fix: validate row type byte before enum cast in MPS parser#1491ramakrishnap-nv wants to merge 1 commit into
ramakrishnap-nv wants to merge 1 commit into
Conversation
Replace unvalidated static_cast<RowType>(char) with convert_to_row_type(), which validates against the four legal values (N/L/G/E) and raises a ValidationError for anything else. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ramakrishnap-nv
added
non-breaking
📝 WalkthroughWalkthroughA new MPS Row-type Validation
🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/io/mps_parser.cpp`:
- Around line 809-812: Parse the free-format row type as a full token in the MPS
parser instead of using a single char in the row-type read path. In
mps_parser.cpp, update the logic around std::stringstream ss, read_word, and
convert_to_row_type so it extracts the complete field, validates that exactly
one token was present, and rejects malformed inputs like extra trailing text
rather than accepting the first character only. Also ensure the code does not
use an uninitialized read_word when extraction fails.
- Around line 805-807: Guard the fixed-format ROWS parsing in mps_parser.cpp
before accessing line[1] or calling line.substr(4, 8) inside the
fixed_mps_format branch. Add a length validation in the ROWS handling path used
by convert_to_row_type and trim so malformed short records fail with
ValidationError first, instead of risking undefined behavior or
std::out_of_range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75d6b1e8-1bf2-4d31-a13f-4783f161c1c5
📒 Files selected for processing (1)
cpp/src/io/mps_parser.cpp
Comment on lines
805
to
807
| if (fixed_mps_format) { | ||
| type = static_cast<RowType>(line[1]); | ||
| type = convert_to_row_type(line[1]); | ||
| name = trim(line.substr(4, 8)); // max of 8 chars allowed |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard fixed-format ROWS lines before indexing.
line[1] and line.substr(4, 8) run before any validation, so a short malformed ROWS record can still hit undefined behavior or throw std::out_of_range instead of the intended ValidationError.
Suggested fix
if (fixed_mps_format) {
+ mps_parser_expects(line.size() >= 5,
+ error_type_t::ValidationError,
+ "ROWS should have at least a row type and name! line=%s",
+ std::string(line).c_str());
type = convert_to_row_type(line[1]);
name = trim(line.substr(4, 8)); // max of 8 chars allowed
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (fixed_mps_format) { | |
| type = static_cast<RowType>(line[1]); | |
| type = convert_to_row_type(line[1]); | |
| name = trim(line.substr(4, 8)); // max of 8 chars allowed | |
| if (fixed_mps_format) { | |
| mps_parser_expects(line.size() >= 5, | |
| error_type_t::ValidationError, | |
| "ROWS should have at least a row type and name! line=%s", | |
| std::string(line).c_str()); | |
| type = convert_to_row_type(line[1]); | |
| name = trim(line.substr(4, 8)); // max of 8 chars allowed |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/io/mps_parser.cpp` around lines 805 - 807, Guard the fixed-format
ROWS parsing in mps_parser.cpp before accessing line[1] or calling
line.substr(4, 8) inside the fixed_mps_format branch. Add a length validation in
the ROWS handling path used by convert_to_row_type and trim so malformed short
records fail with ValidationError first, instead of risking undefined behavior
or std::out_of_range.
Comment on lines
809
to
+812
| std::stringstream ss{std::string(line)}; | ||
| char read_word; | ||
| ss >> read_word; | ||
| type = static_cast<RowType>(read_word); | ||
| type = convert_to_row_type(read_word); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse the free-format row type as a token, not a single char.
operator>>(char) only consumes one non-whitespace byte. That means malformed input like NN COST is accepted as row type N, and if extraction fails read_word is used uninitialized. The new validation should reject the whole field, not just its first byte.
Suggested fix
} else {
std::stringstream ss{std::string(line)};
- char read_word;
- ss >> read_word;
- type = convert_to_row_type(read_word);
+ std::string row_type_token;
+ ss >> row_type_token;
+ mps_parser_expects(!row_type_token.empty() && row_type_token.size() == 1,
+ error_type_t::ValidationError,
+ "Invalid row type '%s' found in ROWS section; expected N, L, G, or E",
+ row_type_token.c_str());
+ type = convert_to_row_type(row_type_token[0]);
ss >> name;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| std::stringstream ss{std::string(line)}; | |
| char read_word; | |
| ss >> read_word; | |
| type = static_cast<RowType>(read_word); | |
| type = convert_to_row_type(read_word); | |
| std::stringstream ss{std::string(line)}; | |
| std::string row_type_token; | |
| ss >> row_type_token; | |
| mps_parser_expects(!row_type_token.empty() && row_type_token.size() == 1, | |
| error_type_t::ValidationError, | |
| "Invalid row type '%s' found in ROWS section; expected N, L, G, or E", | |
| row_type_token.c_str()); | |
| type = convert_to_row_type(row_type_token[0]); | |
| ss >> name; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/io/mps_parser.cpp` around lines 809 - 812, Parse the free-format row
type as a full token in the MPS parser instead of using a single char in the
row-type read path. In mps_parser.cpp, update the logic around std::stringstream
ss, read_word, and convert_to_row_type so it extracts the complete field,
validates that exactly one token was present, and rejects malformed inputs like
extra trailing text rather than accepting the first character only. Also ensure
the code does not use an uninitialized read_word when extraction fails.
CI Test Summary2 failed · 29 passed · 0 skipped
|
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
1 participant
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.
Replace unvalidated
static_cast<RowType>(char)inparse_rowswith aconvert_to_row_type()helper that checks the input against the four valid values (N/L/G/E) and raises aValidationErrorfor anything else. Follows the same pattern as the existingconvert()(BoundType) andconvert_to_obj_sense()helpers.Test plan
ValidationErrorinstead of UB