Skip to content

Android JNI: reject invalid length at direct buffer entry points#3178

Merged
vigneshvg merged 1 commit intoAOMediaCodec:mainfrom
parasol-aser:fix/android-jni-signed-length-oob-3177
Apr 20, 2026
Merged

Android JNI: reject invalid length at direct buffer entry points#3178
vigneshvg merged 1 commit intoAOMediaCodec:mainfrom
parasol-aser:fix/android-jni-signed-length-oob-3177

Conversation

@parasol-aser
Copy link
Copy Markdown

Summary

Fixes #3177.

The public Android JNI decoder wrapper (android_jni/avifandroidjni/src/main/jni/libavif_jni.cc) accepts a caller-supplied signed int length at every direct-ByteBuffer entry point and forwards it, unchecked, into the native size_t size boundary of avifDecoderSetIOMemory(). A negative length (e.g. -1) or a negative-as-int oversize (e.g. 0x80000000) sign-extends to a near-SIZE_MAX value; the parser then trusts the inflated advertised size and reads past the real direct-buffer allocation. ASan reports a heap-buffer-overflow in avifROStreamRead (see the stack in the issue).

This PR enforces the contract at the JNI boundary — where the signed int enters — so that every downstream caller of avifDecoderSetIOMemory() stays free to pass any size_t it likes.

Changes

  • New ValidateDirectBuffer() helper in the libavif_jni.cc anonymous namespace that:
    • Rejects length < 0 before any cast.
    • Reads GetDirectBufferCapacity(encoded) once and rejects non-direct buffers (capacity < 0) and length > capacity (strict >).
    • Rejects a nullptr address from GetDirectBufferAddress().
    • Casts to size_t only after the checks pass; returns the validated (buffer, size_t size) pair.
  • Hardened all four public JNI entry points that consume a direct ByteBuffer and an explicit length: isAvifImage, getInfo, decode, and createDecoder. Each now early-exits with its clean-failure value (false / 0) when validation fails.
  • CreateDecoderAndParse() signature changed from int length to size_t length so the signed-to-unsigned sign-extension is gone from the internal helper too. All four call sites updated.
  • New Android instrumentation regression test AvifDecoderLengthValidationTest covering:
    • The two issue-named boundary values (length = -1, length = Integer.MIN_VALUE).
    • length = capacity + 1 and length = Integer.MAX_VALUE (positive-but-oversized).
    • Non-direct (heap-backed) buffers.
    • Boundary cases that must still succeed: length == capacity, length == 0, and a valid image through AvifDecoder.create() / AvifDecoder.getInfo().
    • Every case verifies the clean-failure contract (false / null) instead of a crash.

Scope and non-goals

  • src/read.c, src/stream.cunchanged. The crash manifests there, but the parser is trusting an already-bad size handed to it. Fixing it lower down would silently paper over other callers of avifDecoderSetIOMemory() that are free to pass a large size_t deliberately.
  • AvifDecoder.java public API surface — unchanged. The issue's secondary suggestion of deprecating the int length overloads in favor of encoded.remaining()-driven variants is deliberately not taken here; it would be a source-incompatible change, and the issue frames it as secondary hardening. Happy to do that as a follow-up if maintainers want it.
  • No new linker deps, no new JNI reflection lookups, no C++ standard bump.

Test plan

  • Repro confirmed pre-fix. Against b357f08 (current main), the 8-byte truncated-ftyp payload with length = -1 or length = 0x80000000 produces the ASan heap-buffer-overflow at avifROStreamReadavifParseFileTypeBoxavifParseavifDecoderParse (matches the issue).
  • Repro cleared post-fix at the JNI boundary. With ValidateDirectBuffer() in place, the same inputs through AvifDecoder.getInfo / decode / isAvifImage / create return the clean-failure value (false / null) without entering the parser. Verified with a simulated-JNI-validation harness that mirrors the new helper before calling avifDecoderSetIOMemory().
  • Parser contract unchanged. Calling avifDecoderSetIOMemory() directly with (size_t)-1 still crashes as before — expected; the fix is at the JNI boundary, not in the parser. This is intentionally not a behavior change for non-JNI callers.
  • Android instrumentation suite. Run AvifDecoderLengthValidationTest on an Android emulator/device, e.g.:
    cd android_jni && ./gradlew :avifandroidjni:connectedDebugAndroidTest \
      --tests org.aomedia.avif.android.AvifDecoderLengthValidationTest
    Every test case must pass. I don't have an Android device/emulator in this environment to run the instrumentation suite myself; maintainers with CI access should confirm. The tests reuse the existing avif/fox.profile0.8bpc.yuv420.avif asset already shipped under androidTest/assets for the happy-path cases.
  • Build hygiene. ./android_jni/gradlew :avifandroidjni:assembleDebug must continue to link cleanly after the CreateDecoderAndParse signature change. -Wsign-compare / -Wconversion remain clean across the edited block (the helper uses an explicit static_cast<jlong>(length) for the capacity comparison).

Boundary behavior verified by the test matrix

Entry point length Expected
getInfo -1 false, no crash
getInfo Integer.MIN_VALUE false, no crash
getInfo capacity + 1 (9) false, no crash
getInfo Integer.MAX_VALUE false, no crash
getInfo capacity (8, truncated ftyp) false (parser — truncated data control)
getInfo 0 (empty direct buffer) false (parser — not short-circuited at JNI)
getInfo valid image, honest length true (happy path guard)
decode -1 / MIN_VALUE / cap+1 / MAX_VALUE false, no crash
isAvifImage truncated ftyp / heap-backed / empty false, no crash
create truncated / heap-backed / empty null
create valid image non-null (happy path guard)

🤖 Generated with Claude Code

The public Android JNI decoder entry points accept a signed int length
and forward it, unchecked, into size_t boundaries. A negative length
(e.g. -1) sign-extends to a near-SIZE_MAX value, so the parser trusts
the inflated advertised size and reads past the real direct-buffer
allocation. See issue AOMediaCodec#3177 for the ASan heap-buffer-overflow
reproducer.

Add a ValidateDirectBuffer() helper that rejects negative lengths,
non-direct buffers, lengths larger than the direct buffer capacity,
and null addresses before any cast to size_t. Apply it at every
public JNI entry point consuming (ByteBuffer, int) — isAvifImage,
getInfo, decode, and createDecoder. Change the internal
CreateDecoderAndParse() helper to take size_t length so the
signed-to-unsigned sign-extension is gone from the internal path too.

Add an Android instrumentation regression test
(AvifDecoderLengthValidationTest) that covers length = -1,
length = Integer.MIN_VALUE, length = capacity + 1,
length = Integer.MAX_VALUE, heap-backed buffers, and the happy path
on a valid image, verifying every case returns the clean-failure
contract (false / null) instead of crashing.

Leaves src/read.c, src/stream.c, and the Java public API surface
(AvifDecoder.java) unchanged: the fix enforces the contract at the
JNI boundary where the signed length enters.

Fixes AOMediaCodec#3177

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jzern jzern requested a review from vigneshvg April 20, 2026 18:41
@vigneshvg vigneshvg merged commit b54eac5 into AOMediaCodec:main Apr 20, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] High: Unchecked signed Android JNI length causes parser OOB read

2 participants