Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regex: enable safe handling of invalid UTF-8 by default #39524

Merged
merged 2 commits into from
Mar 8, 2021
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 4, 2021

No description provided.

In the 10 years since this list was written, there are new options. I
have left a few disabled in the list if I was not sure if they made
sense in our context.
UTF

# arguments to pcre2_set_newline
const COMPILE_NEWLINE_MASK = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seemed like someone wanted them in a list, so I kept them here

@JeffBezanson
Copy link
Member

Any performance cost?

@vtjnash
Copy link
Member Author

vtjnash commented Feb 5, 2021

We have some really good @nanosoldier runbenchmarks("shootout" || "problem", vs=":master") for those 😬

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

ProcessExitedException(2)

cc @christopher-dG

@kshyatt kshyatt added the unicode Related to unicode characters and encodings label Feb 6, 2021
@vtjnash vtjnash added the needs nanosoldier run This PR should have benchmarks run on it label Feb 12, 2021
@vtjnash
Copy link
Member Author

vtjnash commented Feb 26, 2021

@nanosoldier runbenchmarks("shootout" || "problem", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG

@vtjnash
Copy link
Member Author

vtjnash commented Feb 26, 2021

Seems safe to say this is well within the noise, as the benchmarks that include a regex explicitly (as opposed to using them from somewhere else, like Base.Filesystem) are:

src/problem/IMDBGraphs.jl
src/problem/JSONParse.jl
src/problem/SpellCheck.jl
src/shootout/k_nucleotide.jl
src/shootout/regex_dna.jl
src/shootout/revcomp.jl

@vtjnash vtjnash removed the needs nanosoldier run This PR should have benchmarks run on it label Feb 26, 2021
@vtjnash
Copy link
Member Author

vtjnash commented Mar 2, 2021

Will merge soon, unless any objections

@vtjnash vtjnash merged commit a01d7f3 into master Mar 8, 2021
@vtjnash vtjnash deleted the jn/25997 branch March 8, 2021 19:17
@StefanKarpinski
Copy link
Member

I object.

@vtjnash
Copy link
Member Author

vtjnash commented Mar 24, 2021

Ah, sorry, I assumed you were in favor of this from past discussions. What's your objection?

@StefanKarpinski
Copy link
Member

This is an improvement but my understanding is that this treats \x80 in a regex literal as U+80 whereas I think it should either be an error or match the byte 0x80.

@vtjnash
Copy link
Member Author

vtjnash commented Mar 27, 2021

No, you're thinking of the other issue. This doesn't change the match string interpretation. This simply sets the flag to disable UB in pcre2 when the searched string isn't valid utf8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unicode Related to unicode characters and encodings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants