Address runaway regexp in 942260

[fgsch]

Should fix remaining issue in #1359.

[dune73]

Thank you very much. Good have a PR here.

Two questions:

• So by using a possessive quantifier, we are limiting us to PCRE. It is no longer possible to transport our RegExes to alternatives like RE2?
• You had to extend the regex-assembly script. Makes sense, but technically, this forces us to test the script against the other existing rules. Or am I wrong?

[fgsch]

So by using a possessive quantifier, we are limiting us to PCRE. It is no longer possible to transport our RegExes to alternatives like RE2?

Yes, for now. I'd like to review it at a later time.
It's worth noting that we already have patterns using PCRE specific syntax.

You had to extend the regex-assembly script. Makes sense, but technically, this forces us to test the script against the other existing rules. Or am I wrong?

There is just one more pattern-file using ++ (util/regexp-assemble/regexp-942130.data) and I've checked it but you are correct.

[dune73]

Thank you for infos. Good to know you think it can be revised. I am aware we are not quite portable yet.

@franbuehler : Could you give us a hand with reviewing the effect of the update to the regex-assembly script?

I will review PR and merge afterwards.

[dune73]

Thank you for the PR. I have tested it and confirm that it solves the 2 ReDoS issues in question.

However, I have also looked at some of the machine generated regexes and I get a delta. I do not really think it is due to your change on the assemble script, but we will probably need to investigate some more. Examples: 932130, 942280.

[fgsch]

Can you share the deltas?

[dune73]

932130
In Rule:      (?:\$(?:\((?:\(.*\)|.*)\)|\{.*\})|[<>]\(.*\))
New Assembly: (?:\$(?:\((?:\(.*\)|.*)\)|\{.*})|[<>]\(.*\))

942280
In Rule:      (?i:(?:;\s*?shutdown\s*?(?:[#;]|\/\*|--|\{)|waitfor\s*?delay\s?[...
New Assembly: (?:;\s*?shutdown\s*?(?:[#;{]|\/\*|--)|waitfor\s*?delay\s?[...

[fgsch]

Thanks, I will take a look.

[dune73]

Thank you. We probably need to make sure we test the regex assembly against the regexes in the rules with every PR. Otherwise we can not make sure, the rules really, really are up to date with the assembly src files and the latest version of the scripts.
At least, that would be the proper way to do it.

[fgsch]

Yes. Unfortunately we modify the patterns in place either by appending or prepending another pattern, making it case insensitive, or a combination of these, so we will need to incorporate this in the input file somehow. I have some ideas I'm going to explore.
If we do that we can add a test to ensure the final pattern matches what is in the rule.
Anyways, not in this PR / issue but I will take a look when I have some time, after we figure this one out.

[dune73]

Let's test the old assembly script and the new assembly script. If every delta is related to your change, then OK and we merge. If we have additional deltas, you did not anticipate, we need to look further into the changes of this PR.

[dune73]

Sorry, but we have a substantial delta.

Here are the generated regexes in v3.2/dev and 1417. A striking one is the additional trailing 0x0a in the output with the new version of regex-assemble.pl.

regexes-3.2-dev.txt
regexes-1417.txt

[fgsch]

Thanks for checking it. That's unfortunate 😞
I will take a look hopefully this weekend.

[dune73]

We decided in the June community meeting to take out the regex assembly change out of this PR and then merge the updated regex. The updated assembly script would be added separately as a 2nd assembly script aimed at this rule. That would allow us to ship the new release with this fix.

[fgsch]

Updated. Please let me know if this is what you had in mind.

[dune73]

Works as advertised. Thank you very much for the PR and for creating the 2nd script. Merging now.