Verify shader bundle FlatBuffer before access in flutter_gpu ShaderLibrary

[adilburaksen]

Description

ParseShaderBundle in engine/src/flutter/lib/gpu/shader_library.cc checks the
"IPSB" file identifier and then immediately calls GetShaderBundle() and reads
fields (format_version(), the shaders vector, names, mappings) without first
running a flatbuffers::Verifier. A buffer with a valid identifier but corrupt
internal 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_gpu shader-bundle loader (IPSB), reachable via
ShaderLibrary.fromAsset / MakeFromFlatbuffer, was not covered by that change.

Change

Add a VerifyShaderBundleBuffer() check after the identifier check and before
GetShaderBundle(), mirroring the sibling loaders. The generated
shader_bundle_flatbuffers.h (already included) provides the verifier; no new
include 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 RejectsCorruptBufferWithValidIdentifier
from #187878 if a maintainer prefers it wired in.

[flutter-dashboard]

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.

[gemini-code-assist]

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.

[gemini-code-assist]

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

1. Avoid redundant casts to keep the code clean and readable, adhering to general C++ best practices and the Google C++ Style Guide. ^(link)

[gaaclarke]

@bdero we just landed a similar PR for the flat buffers for shaders in impeller. This seems like something we'd want to do. I believe we had tests for our PR though. Can you give this a look?

@adilburaksen I don't have the PR off hand but can you look at the one that added this verification for impeller and see how to potentially add some tests?

[adilburaksen]

Thanks @gaaclarke! I added unit tests in lib/gpu/shader_library_unittests.cc that mirror the corrupt-buffer tests in the impeller runtime stage and shader archive loaders. They drive ShaderLibrary::MakeFromFlatbuffer and check that a corrupt buffer with a valid IPSB identifier, a truncated buffer, a buffer without the identifier, and a null payload are all rejected (null library), plus a positive control that a structurally valid bundle passes VerifyShaderBundleBuffer. I also added the shader_bundle_flatbuffers dep to the gpu_unittests target for the flatbuffer symbols. PTAL.

[bdero]

LGTM

[andywolff]

LGTM except for one small change requested to fix compilation failure

[andywolff]

When I tried running these tests locally on my mac, compilation of gpu_unittests with et build failed with error:

error: invalid operands to binary expression ('const fml::RefPtrflutter::gpu::ShaderLibrary' and 'const std::nullptr_t')

This occurs because fml::RefPtr does not define operator== with std::nullptr_t. The tests compile and pass for me if we use EXPECT_FALSE(library) instead of EXPECT_EQ(library, nullptr)

[adilburaksen]

Thanks @andywolff! Switched all five EXPECT_EQ(library, nullptr) to EXPECT_FALSE(library) since fml::RefPtr has no operator== with nullptr_t. Pushed.

[auto-submit]

autosubmit label was removed for flutter/flutter/188252, because - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

[bdero]

Ah, it's best to re-request review after previous reviews go stale due to a push. Otherwise the PR won't end up in people's review queues and may get lost in the ocean of activity going on around Flutter.