Fix nested repeat warning - #682
Open
kx7m2qd wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
It looks like these changes are specific to your local setup. We generally avoid committing .gitignore changes to this repository unless necessary. Could you please revert this file?
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.
Description
Fixes the "nested repeat operator '+' and '?' was replaced with '*'" warning
that Ruby emits when compiling the Cisco RV Series fingerprint pattern in
xml/snmp_sysdescr.xml.(?:[A-Z]+)?(a non-capturing group containing "one or more" wrapped in"optional") is redundant with
(?:[A-Z]*)("zero or more") - both match theexact same set of strings. Since it's a non-capturing group, there's no
nil-vs-empty-string capture semantics to worry about here (unlike
(X+)?vs
(X*)inside a capturing group, which does change behavior).Motivation and Context
This picks up where #662 left off. That PR proposed a blanket
(.+)?->(.*)substitution across many patterns in the XML database, but reviewfound that (a) most of those patterns don't actually produce the warning
under Ruby 3.3, and (b) for capturing groups,
(.+)?->(.*)isn'tsemantically safe - it changes an unmatched capture from
nilto"",which breaks branching logic elsewhere in the recog-ruby gem (see
rapid7/recog-ruby, separate PR incoming for that).
So rather than reuse that diff, I re-audited every
pattern=attribute inxml/directly against Ruby's warning output. Only one pattern in theentire database actually warns - this one. Everything else is left
untouched.
Relates to rapid7/metasploit-framework#20121 (RegEx Replacement Warning in
smb_version) and supersedes #662.
How Has This Been Tested?
patternattribute from all 52 files inxml/viaRegexp.newunderruby -wand checks stderr for the"nested repeat operator" warning. Only
xml/snmp_sysdescr.xml's Cisco RVpattern triggers it, both before and after cross-checking against Ruby
3.3.8 behavior reported in review on Fix nested repeat operator warnings in Ruby 3.3 #662.
MatchData#capturesforall examples in the fingerprint (
RV320,RV345-K9) plus a syntheticRV320Wcase, confirming the fix doesn't change matching behavior.bin/recog_verify xml/snmp_sysdescr.xmllocally, all examples stillpass.
Types of changes
Checklist: