fix: strip inline comments from single-value config sections#7546
Open
pete-csyn wants to merge 2 commits into
Open
fix: strip inline comments from single-value config sections#7546pete-csyn wants to merge 2 commits into
pete-csyn wants to merge 2 commits into
Conversation
Inline '#' comments on a value line crash rippled at startup for single-value sections (e.g. [peer_private], [ledger_history], [network_id]): parseIniFile() only discards whole-line comments, so the value is passed verbatim to lexicalCast and throws beast::BadLexicalCast (a std::bad_cast, not std::runtime_error) -> uncaught terminate -> crash-loop, with no line number or section in the message. The same inline comments are silently stripped for key=value sections via BasicConfig::Section::append. Strip a trailing, unescaped '#' comment (matching Section::append's escaped-'\#' handling) when reading a single-value section, and add a regression case to Config_test::testNetworkID. Fixes XRPLF#7545
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a startup crash caused by inline # comments in single-value config sections by stripping trailing, unescaped comments before values are passed to beast::lexicalCastThrow, aligning behavior with the existing key = value parsing path.
Changes:
- Add
removeTrailingComment()inConfig.cppand apply it ingetSingleSection()so single-value sections tolerate trailing inline comments (with\#escape handling). - Extend
Config_test::testNetworkIDto cover a single-value section containing a trailing inline comment and ensure it parses successfully.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/xrpld/core/detail/Config.cpp | Strip trailing inline # comments (respecting \#) for single-value sections before lexical casting. |
| src/test/core/Config_test.cpp | Add regression coverage for inline comments in [network_id] single-value parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // which escaped as an uncaught terminate at startup. Catch | ||
| // std::exception so a regression fails the test rather than aborting | ||
| // the test binary. See issue #7545. | ||
| try |
Author
There was a problem hiding this comment.
Done — added error.clear(); immediately before the try in testNetworkID (commit edcea92). The new inline-comment case is now self-contained and no longer relies on error being empty from the preceding assertions. Thanks for the catch.
Per Copilot review on PR XRPLF#7546: error is only assigned in the catch block, so the trailing BEAST_EXPECT(error.empty()) relied on error already being empty on entry. Clearing it explicitly makes the new case independent of preceding assertions and robust to future edits.
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.
Summary
Inline
#comments on a value line crashrippledat startup for single-value config sections, even though the same comments are silently stripped forkey = valuesections. Fixes #7545.Problem
parseIniFile()(src/xrpld/core/detail/Config.cpp) treats a line as a comment only if it starts with#. Single-value sections are then read bygetSingleSection()and passed straight tobeast::lexicalCastThrow<...>(), so a value like:becomes
lexicalCastThrow<bool>("1 # refuse unsolicited inbound peering")→beast::BadLexicalCast. Because that derives fromstd::bad_cast(notstd::runtime_error), it escapes as an uncaughtterminate, crash-looping the daemon with no line number or section name in the message.key = valuesections are unaffected becauseBasicConfig::Section::append()already strips inline comments (including escaped\#).Fix
Strip a trailing, unescaped
#comment when reading a single-value section, mirroringSection::append's escaped-\#handling. Scoped togetSingleSection()so multi-line sections (already handled bySection::append) are untouched.This implements option (a) from #7545. Happy to refactor the stripping into a helper shared with
Section::append, or switch to option (b) (fail with a clear parse error naming the section/value) if maintainers prefer.Test
Extends
Config_test::testNetworkIDwith a single-value section carrying a trailing inline comment, asserting it parses (networkId == 1024) instead of throwing. The new case catchesstd::exceptionso a regression fails the test rather than aborting the test binary.Type of change