HBASE-27315 Add timeout to JavaRegexEngine #4720
Open
+375
−1
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Sep 15, 2022
|
|
||
| @Override | ||
| public char charAt(int index) { | ||
| final long diff = EnvironmentEdgeManager.currentTime() - startMillis; |
There was a problem hiding this comment.
The implementation is a bit tricky... And will this slow down the normal process? As we will do a System.currentTimeMillis call every time when we want to get a char...
There was a problem hiding this comment.
@Apache9
Thank you for your review.
I've heard that ReDoS is caused by backtracking.
Therefore, I considered adding RE2J as a new engine to prevent ReDoS.
(RE2J does not use backtracking.)
However, in the case of RE2J, the syntax and results were slightly different from the existing engines.
I couldn't have confidence if it was okay to add a RE2J engine.
so I looked for an alternative other than adding a RE2J engine.
For the Java Regex engine, I founded that the engine call charAt method every time.
Java Regex engine does not provide a timeout feature, but it seemed that it could be implemented (anomalously) with charAt.
I considered checking the timeout with additional Threads.
However, I felt that running additional Threads was a waste of resources.
When monitored the internal cluster, no large overhead was found due to System.currentTimeMillis calls in the pattern matching operation.
(There may be some overhead, of course.)
By default, TimeoutCharSequence is not used, so most HBase users are unlikely to be affected by System.currentTimeMillis overhead.
I wanted to provide a way to solve the ReDoS.
If there is another good way, I would like to get some advice.
There was a problem hiding this comment.
@Apache9
I changed to not call System.currentTimeMillis every time.
Could you please check this PR again?
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Apr 3, 2023
| @@ -255,11 +258,20 @@ static interface Engine { | |||
| * This is the default engine. | |||
| */ | |||
| static class JavaRegexEngine implements Engine { | |||
| private static final Configuration conf = HBaseConfiguration.create(); | |||
There was a problem hiding this comment.
Can we find a way to pass this Configuration in?
There was a problem hiding this comment.
@Apache9
I think it is possible.
RSRpcService pass configuration to ProtobufUtil.
->
Get clientGet = ProtobufUtil.toGet(get, regionServer.conf);ProtobufUtil#toComparator pass configuration to ByteArrayComparable when calling parseFrom method.
->
public static ByteArrayComparable toComparator(ComparatorProtos.Comparator proto, Configuration conf) {
return (ByteArrayComparable)parseFrom.invoke(null, value, conf);
}I'll try to make the change.
There was a problem hiding this comment.
To pass configurations to RegexStringComparator, a new class ConfigurationHolder was created. It holds the configuration as a member variable. When HMaster and HRegionserver are started, it registers itself as an observer to the ConfigurationManager.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
mwkang
force-pushed
the
HBASE-27315
branch
2 times, most recently
from
778b2ac to
16968a7
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
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.
Add timeout to JavaRegexEngine to prevent ReDoS.
https://issues.apache.org/jira/browse/HBASE-27315