fix(core): error due to integer overflow when there are too many host bindings #38014
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
… bindings We currently use 16 bits to store information about nodes in a view. The 16 bits give us 65536 entries in the array, but the problem is that while the number is large, it can be reached by ~4300 directive instances with host bindings which could realistically happen is a very large view, as seen in angular#37876. Once we hit the limit, we end up overflowing which eventually leads to a runtime error. These changes bump to using 20 bits which gives us around 1048576 entries in the array or 16 times more than the current amount which could still technically be reached, but is much less likely and the user may start hitting browser limitations by that point. I picked the 20 bit number since it gives us enough buffer over the 16 bit one, while not being as massive as a 24 bit or 32 bit. I've also added a dev mode assertion so it's easier to track down if it happens again in the future. Fixes angular#37876.
crisbeto
force-pushed
the
37876/host-binding-integer-overflow
branch
from
6a44bb0
to
a883497
Compare
crisbeto
added
area: core
There was a problem hiding this comment.
The change looks good and this is corner case but still it would be good to have a test for this particular scenario. Do you think it would make sense to generate a directive with a large number of host bindings (256+) and a template with 256+ usages of such directive? Or did you assume that it is impractical / not worth having a test for this case?
|
I thought that it would be impractical and it wouldn't provide much value. The basic logic is already covered by other test cases and all we'd be testing is higher and higher numbers. |
crisbeto
added
action: presubmit
AndrewKushnir
added
action: merge
AndrewKushnir
pushed a commit
that referenced
this pull request
Jul 17, 2020
… bindings (#38014) We currently use 16 bits to store information about nodes in a view. The 16 bits give us 65536 entries in the array, but the problem is that while the number is large, it can be reached by ~4300 directive instances with host bindings which could realistically happen is a very large view, as seen in #37876. Once we hit the limit, we end up overflowing which eventually leads to a runtime error. These changes bump to using 20 bits which gives us around 1048576 entries in the array or 16 times more than the current amount which could still technically be reached, but is much less likely and the user may start hitting browser limitations by that point. I picked the 20 bit number since it gives us enough buffer over the 16 bit one, while not being as massive as a 24 bit or 32 bit. I've also added a dev mode assertion so it's easier to track down if it happens again in the future. Fixes #37876. PR Close #38014
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
profanis
pushed a commit
to profanis/angular
that referenced
this pull request
Sep 5, 2020
… bindings (angular#38014) We currently use 16 bits to store information about nodes in a view. The 16 bits give us 65536 entries in the array, but the problem is that while the number is large, it can be reached by ~4300 directive instances with host bindings which could realistically happen is a very large view, as seen in angular#37876. Once we hit the limit, we end up overflowing which eventually leads to a runtime error. These changes bump to using 20 bits which gives us around 1048576 entries in the array or 16 times more than the current amount which could still technically be reached, but is much less likely and the user may start hitting browser limitations by that point. I picked the 20 bit number since it gives us enough buffer over the 16 bit one, while not being as massive as a 24 bit or 32 bit. I've also added a dev mode assertion so it's easier to track down if it happens again in the future. Fixes angular#37876. PR Close angular#38014
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
We currently use 16 bits to store information about nodes in a view. The 16 bits give us 65536 entries in the array, but the problem is that while the number is large, it can be reached by ~4300 directive instances with host bindings which could realistically happen in a very large view, as seen in #37876. Once we hit the limit, we end up overflowing which eventually leads to a runtime error.
These changes bump to using 20 bits which gives us around 1048576 entries in the array or 16 times more than the current amount which could still technically be reached, but is much less likely and the user may start hitting browser limitations by that point.
I picked the 20 bit number since it gives us enough buffer over the 16 bit one, while not being as massive as a 24 bit or 32 bit.
I've also added a dev mode assertion so it's easier to track down if it happens again in the future.
Fixes #37876.