[Impeller] Fix crash trying to check for duplicate vertices in shadow_path code#180920
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request fixes a crash that occurs when checking for duplicate vertices on an empty vector. The fix correctly adds a check to ensure the vectors are not empty before accessing their last elements. My review includes a suggestion to simplify the implementation of this check for better readability and conciseness.
| if (index > 0u) { | ||
| FML_DCHECK(!gaussians_.empty() && !vertices_.empty()); | ||
| if (gaussian == gaussians_.back() && vertex == vertices_.back()) { | ||
| return index - 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
The if (index > 0u) check on line 1362 already guarantees that vertices_ and gaussians_ are not empty. This is because index is initialized to vertices_.size(), and the FML_DCHECK on line 1359 ensures gaussians_ has the same size.
Therefore, the FML_DCHECK on line 1363 is redundant.
You can simplify this logic by combining the conditions into a single if statement. C++'s short-circuit evaluation for && will prevent .back() from being called on empty vectors.
if (index > 0u && gaussian == gaussians_.back() && vertex == vertices_.back()) {
return index - 1;
}There was a problem hiding this comment.
I realize this. It's a check so that tests run with debug_unopt builds will catch any future issues.
Belt-and-suspenders kind of thing and I'm not worried about performance on debug_unopt...
gaaclarke
left a comment
There was a problem hiding this comment.
I wish there was a way we could make this crash in our tests too. Is there a linter flag for this? Can we also enable the flags that g3 is using?
| FML_DCHECK(index <= std::numeric_limits<uint16_t>::max()); | ||
| if (gaussian == gaussians_.back() && vertex == vertices_.back()) { | ||
| return index - 1; | ||
| if (index > 0u) { |
There was a problem hiding this comment.
nit: I think if (!gaussians_.empty() && !vertices_.empty()) is more clear instead of branching on the derivative.
There was a problem hiding this comment.
The thing is that index is sufficient without making 2 method calls. We've already asserted with DCHECK that the 2 vectors are the same length and index contains the size of the vectors. I only added the DCHECK so tests would fail. I tested the unit tests without the index check and the DCHECK crashed and then I added the index check and they passed just fine.
There was a problem hiding this comment.
It looks like at least one of our CI runs is on a debug_unopt build and Jason and John confirmed that it does run the impeller tests. I confirmed before I fixed it that the DCHECK fails without the fix and passes with it, so I think this is OK. Though, as a general rule, I'd rather that all CI tasks that run unit tests run them on a debug_unopt build.
|
Maybe it's ASAN? |
It is believed that the G3 test runs have it enabled, but we don't. |
|
test-exempt: new DCHECK changes behavior of existing tests |
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…_path code (#180920) The new general convex path shadow code was checking for duplicate vertices without actually checking if the vectors contained any vertices. Thus, <vector>.back() was being called on empty vectors, which is bad. This led to a crash in G3 as their code is being run with a vector implementation that protects against this, but apparently we do not have such a vector implementation in our local building and testing. No tests because this covered by existing test cases and a new FML_DCHECK, however I'm not sure if we have DCHECKs enabled in CI...?
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
|
Failed to create CP due to merge conflicts. |
…_path code (flutter#180920) The new general convex path shadow code was checking for duplicate vertices without actually checking if the vectors contained any vertices. Thus, <vector>.back() was being called on empty vectors, which is bad. This led to a crash in G3 as their code is being run with a vector implementation that protects against this, but apparently we do not have such a vector implementation in our local building and testing. No tests because this covered by existing test cases and a new FML_DCHECK, however I'm not sure if we have DCHECKs enabled in CI...?
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…_path code (flutter#180920) The new general convex path shadow code was checking for duplicate vertices without actually checking if the vectors contained any vertices. Thus, <vector>.back() was being called on empty vectors, which is bad. This led to a crash in G3 as their code is being run with a vector implementation that protects against this, but apparently we do not have such a vector implementation in our local building and testing. No tests because this covered by existing test cases and a new FML_DCHECK, however I'm not sure if we have DCHECKs enabled in CI...?
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
…n shadow_path code (flutter/flutter#180920)
… shadow_path code (#180920) (#181095) The new general convex path shadow code was checking for duplicate vertices without actually checking if the vectors contained any vertices. Thus, <vector>.back() was being called on empty vectors, which is bad. This led to a crash in G3 as their code is being run with a vector implementation that protects against this, but apparently we do not have such a vector implementation in our local building and testing. No tests because this covered by existing test cases and a new FML_DCHECK. <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> Fixes #180827 Impacted users: Any user who uses shadows (pretty much every Flutter app) on a platform that has address sanitization built into its `std::vector` implementation, if they use Impeller (always used on iOS and the default on Android) Impact Description: Every shadow render operation in Impeller backend will access `back()` on an empty `std::vector` which will crash immediately if the vector is using sanitization on that platform. Workaround: Potentially look into disabling address sanitization? On platforms that still support Skia, it can be used as a workaround, but that is not an option on iOS Risk: The fix was obvious and non-controversial. Test Coverage: Existing tests cover this case really well and there is now an FML_DCHECK (engine assert) to check if we trigger the condition under testing. Validation: The original issue indicates an app that will trigger the sanitization crash, but only in G3. None of the platforms we support directly through flutter perform this sanitization. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.
Fixes #180827
The new general convex path shadow code was checking for duplicate vertices without actually checking if the vectors contained any vertices. Thus, .back() was being called on empty vectors, which is bad. This led to a crash in G3 as their code is being run with a vector implementation that protects against this, but apparently we do not have such a vector implementation in our local building and testing.
No tests because this covered by existing test cases and a new FML_DCHECK, however I'm not sure if we have DCHECKs enabled in CI...?