Moves all argument checking of Vertices() and Vertices.raw() to asserts, Adds arg checking to all web renderers -Fixes #123085 #41766
Conversation
|
Amazing stuff, thanks a lot! Though, I would still insist on giving an option to remove the checks completely (via putting behind an assert, or parameter to disable them) because once the source of |
|
As it exists currently this PR is intended to optimize performance at zero change to the existing validity checks. I agree that wrapping them in an assert like you illustrate in flutter/flutter#123085 would be where the real performance gain would be. It sounds like at least some of the team is on board - but it does take away the check entirely. |
|
Can we get their opinion on this somehow? I am not aware of the internal Flutter team structure, unfortunately, and for me the change sounds simple enough to follow through with just a "let's go" or two from the internal team. |
…s, SkWasm doesnt check args
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. |
|
Is the engine resilient to bad data here, or will bad data cause a crash? cc @chinmaygarde for a final decision on landing this. We should either decide to go for it or close the PR. |
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Please update your tests so that if assertions are not enabled, you actually try to use the invalid objects.
I suspect flutter_tester will segfault in this case. If so, we have more to fix. If not we have coverage to show that it's safe to do this.
|
@timmaffett – we have a bit of distance to go to get this landed...I hope we do...but either way, this effort is AMAZING. Super thoughtful and thorough. "WE" (the Flutter ecosystem) are so lucky to have folks like you! |
Thank you. |
| throw 'Vertices.raw did not throw the expected error.'; | ||
| } on ArgumentError catch (e) { | ||
| expect('$e', 'Invalid argument(s): "positions" must have an even number of entries (each coordinate is an x,y pair).'); | ||
| ).dispose(); |
There was a problem hiding this comment.
Can you also add something in here that tries to actually use these objects on a Canvas when assertions are not enabled?
We want to make sure that these invalid objects won't cause a crash.
small tweak
|
From PR triage: The comments about tests from @dnfield are still relevant and need to be addressed before this PR can land. Adding myself as a reviewer to route to an engine team member when the tests are updated. |
|
Still looking at one comment in the test file by Dan. |
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
We shouldn't ship this. Replacing all of the bounds checks with asserts means mismatched positions/colors could cause segfaults or other UB in profile/release builds.
| } | ||
| }) : assert(textureCoordinates == null || textureCoordinates.length == positions.length,'"positions" and "textureCoordinates" lengths must match.'), | ||
| assert(colors == null || colors.length == positions.length,'"positions" and "colors" lengths must match.'), | ||
| assert(indices == null || indices.every((int i) => i >= 0 && i < positions.length),'"indices" values must be valid indices in the positions list.') { |
There was a problem hiding this comment.
We could definitely let the indices check be an assert. I think at most you'll get graphical corruption.
There was a problem hiding this comment.
actually, on iOS/Metal we have to unroll triangle fans. If we end up using the indices to look up positions an invalid index could still lead to a crash.
There was a problem hiding this comment.
@jonahwilliams Do you think I should go ahead and just close this ? - or would moving the checks to the c++ code closer where they are used make more sense ? (and possibly be faster?)
|
That depends on if it segfaults on real devices if the sizes or indices are mismatched. |
|
If the colors or texture coordinates is shorter than positions, then we'll read past the bounds of input data. This is a potential UB. We need to validate array lengths. https://github.com/flutter/engine/blob/main/lib/ui/painting/vertices.cc#L58-L64 If the lenght of colors or texture coordinates is too long then we just ignore the extras. I'm not able to trigger any crashes in the index code though, I think we might be safe to make that assert only. |
Would a compile time const to move these to asserts afford someone the option of "unsafe optimizations"? |
|
We removed the expensive runtime checks in #53558 . The bounds checks should remain as they are not expensive (compared to copying the data into the native heap) and are required for memory safety. |
10 participants
This PR (now) moves all argument checking of
Vertices()andVertices.raw()into asserts. It also implements the checking (within asserts) on all web renderers (previously the web renderers did not check the arguments).The consensus on #123085 seemed to be that this was a change worth doing and it will definitely reduce cpu load looping over all indices in release builds.
Originally this was only an optimization of the indices check, but after evaluating the code and noticing that all web renderers were never checking the arguments it seemed perfectly reasonable to move the argument checking to asserts and additionally adding the assert argument checking to all of the web renderers.
It also adds tests to all platforms to verify that the assert argument checking is working correctly.
This PR is the most conservative thing we can do to address some of flutter/flutter#123085This reduces the number of ops needed to check the
indices[n]*2 <= positions.lengthby avoiding the *2 by simply comparingindices[n] < halfPositionsLength.Additionally, this PR also fixes the indices[] checking within the canvaskit web_ui version of Vertices.raw() (CkVertices.raw())This routine was incorrectly testing
indices[n] < positions.lengthinstead ofindices[n]*2 < positions.length. It was also missing a check to verify that positions.length was even (because it holds pairs of positions for the raw()).One note is that the checks within CkVertice() and CkVertice.raw() are using indices.any() instead of looping, and their error messages are less informative their Vertices() counterparts because of this. They also check for indices<0 where Vertices() does not. Should these be changed to be identical to Vertices() ?
I also added tests of the CkVertices() and CkVertices.raw() parameter checking - this was completely missing and is how the ckVertices.raw() was getting away with doing incorrect parameter checking. (These are essentially identical to the test that are done against Vertices() withintesting\dart\painting_test.dartwith the exception of the less informative error messages mentioned above).flutter/flutter#123085 is further discussing placing all of the Vertices.raw() parameter checks behind an assert() . Removing all checks for non-debug mode has additional considerations, while this change only optimizes operations with no change to the verification being performed.I would be happy to add the additional assert() wrapper discussed in flutter/flutter#123085 to the checks for a real additional performance gain 😉This has now been implemented.Pre-launch Checklist
///).