Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Validator][RecursiveContextualValidator] Prevent validated hash collisions#36415
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a59494a tof8cea76Compare| if (\is_object($group)) { | ||
| $groupHash =spl_object_hash($group); | ||
| if (!isset($this->validatedObjectsReferences[$groupHash])) { |
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.
Why is this needed for group sequences?
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.
GroupSequences can suffer from the same problem since they can be passed directly to the validate method as objects.
nicolas-grekas commentedApr 10, 2020
We should find a place where these arrays can be emptied. The memory leak should be avoided. |
fancyweb commentedApr 14, 2020
I think we can clear constraints and group sequences arrays at the end of each validate methods. However, we can never clean the initialized objects. Would that work for you? |
f8cea76 to146634dComparesrc/Symfony/Component/Validator/Validator/RecursiveContextualValidator.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedAug 12, 2020
@fancyweb What's the status of this PR? |
fancyweb commentedAug 13, 2020
I need to check Nicolas's suggestion, please keep it open. |
146634d to6f28e2dCompare6f28e2d to1ec0d3aComparefancyweb commentedAug 14, 2020
Unfortunately that does not work. I had to remove this part. |
xabbuh commentedAug 23, 2020
Can you update the |
fancyweb commentedAug 27, 2020
@xabbuh I looked at this a few days ago but it needs more work. My patch does not fix all cases because it does not take into account the current defaultPropertyPath. One of FormValidator tests fails because of this. You can have a look if you want to. |
xabbuh commentedOct 2, 2020
…t hashes (fancyweb, xabbuh)This PR was merged into the 4.4 branch.Discussion----------[Validator] prevent hash collisions caused by reused object hashes| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fix#36415| License | MIT| Doc PR |Commits-------8dd1a6e prevent hash collisions caused by reused object hashes9645fa3 [Validator][RecursiveContextualValidator] Prevent validated hash collisions
RecursiveContextualValidatorusesspl_object_hashat different places to generate cache keys. The problem with this method is that there can be collisions (same hash for two different objects) since once an object is destroyed,spl_object_hashcan return a previously used hash.I encountered the problem on a project with constraints that are created on the fly. The first constraint is applied and is "cached". Then it is destroyed. Then another constraint happen to have the exact same
spl_object_hash. It is skipped while it would cause violations. I was able to reproduce the problem consistently in the unit test.The easy way to deal with this is to keep references to all objects (proposed solution). But I guess it could leak too much.
We could also try to "clean" the
ExecutionContextstores at the end of the three validate methods. However, we would still need to keep references to the initialized objects since they must really be initialized one time, even on multiple calls