-
Notifications
You must be signed in to change notification settings - Fork 30.7k
More ReDOS fixes! #36964
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
More ReDOS fixes! #36964
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with config changes!
imports should be tested in terms of speed. Ast is a bit slow as I remember it but for something lile this we can cache / optimize!
cc @LysandreJik
|
@Rocketknight1 yeah, the |
f52247a
to
a6d5eba
Compare
One more performance note: Since most Transformers code is contained within classes, we can speed up walking the However, I don't think this is really that important - parsing and walking the tree takes << 1s for me. I'm not even sure it's slower than the regex solution! |
a6d5eba
to
0317f64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM then! Parsing a single file is fast!
3378c82
to
ce86970
Compare
ce86970
to
d5534ab
Compare
cc @Michellehbn merged! Should be in the next release and then we can close out those issues |
* More ReDOS fixes! * Slight regex cleanup * Cleanup regex replacement * Drop that regex entirely too * The regex didn't match config.json, let's make sure we don't either * Cleanup allowed_value_chars a little * Cleanup the import search * Catch multi-condition blocks too * Trigger tests * Trigger tests
Hi Matt, I find the ast-style parsing would introduce an error if a function is import torch.nn as nn
class Model(nn.Module):
def __init__(self):
super().__init__()
self.linear = nn.Linear(10, 10)
def forward(self, x):
if x.size() == 1: # error
raise ValueError("x.size() == 1")
return self.linear(x) When I try to run |
* More ReDOS fixes! * Slight regex cleanup * Cleanup regex replacement * Drop that regex entirely too * The regex didn't match config.json, let's make sure we don't either * Cleanup allowed_value_chars a little * Cleanup the import search * Catch multi-condition blocks too * Trigger tests * Trigger tests
This PR replaces three more problematic regexes. In all cases I eliminate the regex entirely.
When parsing Python files for
import
statements we now rely onast.parse()
instead. This is the most complex code but it seems to pass tests.In
chat.py
I think I figured out what the regex was doing and wrote a Python method to duplicate its functionality - cc @gante to confirm I got it right!Finally, the change in
configuration_utils.py
was very straightforward; we could just usestr.startswith()
andstr.endswith()
instead.