Verify shader bundle FlatBuffer before access in flutter_gpu ShaderLibrary#188252
Verify shader bundle FlatBuffer before access in flutter_gpu ShaderLibrary#188252adilburaksen wants to merge 1 commit into
Conversation
…brary ParseShaderBundle in lib/gpu/shader_library.cc checks the "IPSB" file identifier and then calls GetShaderBundle() and reads fields from the buffer without first running a flatbuffers::Verifier. A buffer with a valid identifier but corrupt internal offsets is therefore read out of bounds. Add a VerifyShaderBundleBuffer() check after the identifier check and before GetShaderBundle(), mirroring the verification recently added for the runtime stage and shader archive loaders.
|
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 introduces structural verification of the FlatBuffer in ParseShaderBundle before accessing its fields to prevent potential out-of-bounds reads. The review feedback recommends removing a redundant reinterpret_cast on payload->GetMapping() to simplify the verifier initialization.
| flatbuffers::Verifier verifier( | ||
| reinterpret_cast<const uint8_t*>(payload->GetMapping()), | ||
| payload->GetSize()); |
There was a problem hiding this comment.
Since fml::Mapping::GetMapping() already returns const uint8_t*, the reinterpret_cast is redundant. Removing it simplifies the code and improves readability.
flatbuffers::Verifier verifier(payload->GetMapping(), payload->GetSize());References
- Avoid redundant casts to keep the code clean and readable, adhering to general C++ best practices and the Google C++ Style Guide. (link)
Description
ParseShaderBundleinengine/src/flutter/lib/gpu/shader_library.ccchecks the"IPSB"file identifier and then immediately callsGetShaderBundle()and readsfields (
format_version(), the shaders vector, names, mappings) without firstrunning a
flatbuffers::Verifier. A buffer with a valid identifier but corruptinternal offsets is therefore read out of bounds.
This is the same structural-verification gap that was recently fixed for the two
sibling loaders in #187878:
impeller/runtime_stage/runtime_stage.cc(VerifyRuntimeStagesBuffer)impeller/shader_archive/shader_archive.cc(VerifyShaderArchiveBuffer)The
flutter_gpushader-bundle loader (IPSB), reachable viaShaderLibrary.fromAsset/MakeFromFlatbuffer, was not covered by that change.Change
Add a
VerifyShaderBundleBuffer()check after the identifier check and beforeGetShaderBundle(), mirroring the sibling loaders. The generatedshader_bundle_flatbuffers.h(already included) provides the verifier; no newinclude is required. On verification failure the loader returns an empty shader
map, consistent with the function's existing early-return behavior.
I'm happy to add a unit test mirroring
RejectsCorruptBufferWithValidIdentifierfrom #187878 if a maintainer prefers it wired in.