-
Notifications
You must be signed in to change notification settings - Fork 489
automata: fix onepass::DFA::try_search_slots panic when too many slots are given
#1330
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
base: master
Are you sure you want to change the base?
Conversation
afa03cc to
f03bef3
Compare
…ots are given The API specifically documents that the caller may provide *any* number of slots. But the onepass implementation was panicking when too many were provided. (I had only considered the case when too few were provided when I wrote one-pass implementation I think.) This already works for the PikeVM and the bounded backtracker. This commit adds tests covering those as well. Note that the bug report specifically cites a zero-repetition capture group as the instigator here. While that may have provided the circumstances for the bug to appear, the actual bug is more general than that. You don't need any capture groups at all to provoke it. You just need to provide more slots than there are in the compiled regex. Fixes #1327, Closes #1329
f03bef3 to
685c875
Compare
keith-hall
left a comment
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.
Looks good to me
| // caller provided slots is bigger than the number of slots in the | ||
| // compiled regex. (It's a bit of a weird case, but for simplicity and | ||
| // flexibility reasons, it is an API guarantee that the caller can | ||
| // provided any number of slots that they want.) |
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.
| // provided any number of slots that they want.) | |
| // provide any number of slots that they want.) |
|
|
||
| // There is an edge case when a regex has capture groups with zero | ||
| // repetition (e.g., (abc){0}), the cache may have fewer explicit | ||
| // slots than what the caller provided. | ||
| // So we copy only the slots that exist in the cache. | ||
| // let cache_slots = cache.explicit_slots(); | ||
| // let available = core::cmp::min( | ||
| // cache_slots.len(), | ||
| // slots.len().saturating_sub(self.explicit_slot_start), | ||
| // ); | ||
| // slots[self.explicit_slot_start | ||
| // ..self.explicit_slot_start + available] | ||
| // .copy_from_slice(&cache_slots[..available]); |
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.
probably it makes sense to remove this commented out section, the new comment covers it nicely and more accurately 🙂
| // There is an edge case when a regex has capture groups with zero | |
| // repetition (e.g., (abc){0}), the cache may have fewer explicit | |
| // slots than what the caller provided. | |
| // So we copy only the slots that exist in the cache. | |
| // let cache_slots = cache.explicit_slots(); | |
| // let available = core::cmp::min( | |
| // cache_slots.len(), | |
| // slots.len().saturating_sub(self.explicit_slot_start), | |
| // ); | |
| // slots[self.explicit_slot_start | |
| // ..self.explicit_slot_start + available] | |
| // .copy_from_slice(&cache_slots[..available]); |
The API specifically documents that the caller may provide any number
of slots. But the onepass implementation was panicking when too many
were provided. (I had only considered the case when too few were
provided when I wrote one-pass implementation I think.)
This already works for the PikeVM and the bounded backtracker. This
commit adds tests covering those as well.
Note that the bug report specifically cites a zero-repetition capture
group as the instigator here. While that may have provided the
circumstances for the bug to appear, the actual bug is more general than
that. You don't need any capture groups at all to provoke it. You just
need to provide more slots than there are in the compiled regex.
Fixes #1327, Closes #1329