Port fix for parsing #93
Conversation
WalkthroughAdds a bounds check in parsePrimitiveDataType to avoid comparing beyond input length; also includes a whitespace-only variable declaration change in parseHeader. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Parser as OpenDDLParser
participant Grammar as Grammar::PrimitiveTypeToken
Caller->>Parser: parsePrimitiveDataType(in, end)
loop For each primitive type i
Parser->>Parser: prim_len = strlen(Grammar[i])
alt Remaining < prim_len
Note over Parser: New guard: skip comparison
Parser-->>Parser: continue
else Remaining >= prim_len
Parser->>Grammar: Compare with strncmp
alt Match
Parser-->>Caller: Return matched type
else No match
Parser-->>Parser: continue
end
end
end
Parser-->>Caller: Return "unknown" if none matched
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/OpenDDLParser.cpp (1)
838-845: Critical: hex literal validation condition is inverted (accepts non-hex) and loop order risks deref at end.The current condition uses
<and>with&&, which can never be true for a single character range check; it effectively treats any character as valid hex. Also, the loop condition callsisSeparator(*in)before checkingin != end, risking dereference at end.Apply this safe and correct diff:
- while (!isSeparator(*in) && in != end) { - if ((*in < '0' && *in > '9') || (*in < 'a' && *in > 'f') || (*in < 'A' && *in > 'F')) { - ok = false; - break; - } - ++pos; - ++in; - } + while (in != end && !isSeparator(*in)) { + const char c = *in; + if (!((c >= '0' && c <= '9') || + (c >= 'a' && c <= 'f') || + (c >= 'A' && c <= 'F'))) { + ok = false; + break; + } + ++pos; + ++in; + }This is orthogonal to the PR’s objective but should be fixed promptly. I’m happy to submit a separate PR if you prefer.
🧹 Nitpick comments (2)
code/OpenDDLParser.cpp (2)
590-602: Optional hardening: ensure token-boundary after a successful match.Today, a prefix match like "doubleX" would still be accepted as "double". If the grammar forbids trailing identifier chars after the primitive type, add a boundary check. Also avoid recomputing (end - in) and reusing prim_len for clarity.
Apply this diff to tighten matching:
- for (size_t i = 0; i < (size_t) Value::ValueType::ddl_types_max; i++) { - prim_len = strlen(Grammar::PrimitiveTypeToken[i]); - if (static_cast<size_t>(end - in) < prim_len) { - continue; - } - if (0 == strncmp(in, Grammar::PrimitiveTypeToken[i], prim_len)) { - type = static_cast<Value::ValueType>(i); - break; - } - } + for (size_t i = 0; i < (size_t)Value::ValueType::ddl_types_max; ++i) { + prim_len = ::strlen(Grammar::PrimitiveTypeToken[i]); + const size_t remaining = static_cast<size_t>(end - in); + if (remaining < prim_len) { + continue; + } + if (0 == ::strncmp(in, Grammar::PrimitiveTypeToken[i], prim_len)) { + // Ensure we don't accept partial identifiers like "doubleX". + if (remaining > prim_len) { + const char next = *(in + prim_len); + if (isCharacter(next) || isNumeric(next) || next == '_') { + continue; + } + } + type = static_cast<Value::ValueType>(i); + break; + } + }If the spec does allow immediately-adjacent punctuation (e.g., '[' or '{'), the above still permits it.
577-596: Add a regression test for truncated/partial primitive type tokens.To prevent future regressions of the heap-buffer-overflow, add tests that feed inputs where the buffer ends mid-token (e.g., "unsign", "doubl", "ref" without enough trailing bytes) and verify parsePrimitiveDataType returns ddl_none without touching memory past end. Also add a positive test per token to ensure behavior is unchanged when fully present.
I can draft a minimal unit test that iterates over Grammar::PrimitiveTypeToken, truncates each by 1..N, and asserts no match/crash. Want me to open a follow-up PR with those tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
code/OpenDDLParser.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR (address)
🔇 Additional comments (2)
code/OpenDDLParser.cpp (2)
301-301: LGTM: whitespace-only change in local declaration.No functional impact; consistent with surrounding style.
587-589: Correct and sufficient OOB read fix before strncmp.The length guard ensures we never read past end when comparing against PrimitiveTypeToken, addressing the heap-buffer-overflow reported in issue #92.
|
Summary by CodeRabbit
Bug Fixes
Style