Skip to content

Adopt the __counted_by bounds-safety model on core data buffers#3272

Open
SaudSatopay wants to merge 1 commit into
AOMediaCodec:mainfrom
SaudSatopay:counted-by-bounds-safety
Open

Adopt the __counted_by bounds-safety model on core data buffers#3272
SaudSatopay wants to merge 1 commit into
AOMediaCodec:mainfrom
SaudSatopay:counted-by-bounds-safety

Conversation

@SaudSatopay

Copy link
Copy Markdown

What

This adopts the C bounds-safety ("Safe Buffers" / -fbounds-safety) model on libavif's core data buffers: a feature-guarded AVIF_COUNTED_BY(N) macro applied to the data+size pairs whose pointer provably addresses exactly N elements:

  • avifROData.dataAVIF_COUNTED_BY(size)
  • avifRWData.dataAVIF_COUNTED_BY(size)
  • avifImage.propertiesAVIF_COUNTED_BY(numProperties)

Because avifROData/avifRWData are embedded by value across the decode path (icc/exif/xmp, item property payloads, decode/encode samples, the RO/RW stream raw buffers), annotating these two structs propagates bounds information through much of the parsing surface from a single, airtight invariant.

Safe by default

AVIF_COUNTED_BY(N) follows the same __has_attribute probe idiom as AVIF_NODISCARD, with one addition: it is also gated on __has_feature(bounds_safety). This matters — a stock Clang answers __has_attribute(__counted_by__) with 1 even in an ordinary build, then rejects the count expression as an undeclared identifier, so an __has_attribute-only guard would break normal C builds on recent Clang. Gating on the feature makes the annotation expand to nothing unless the TU is compiled with bounds-safety. Both the __counted_by__ and counted_by spellings are accepted.

The headers compile cleanly with -Wall -Wextra -Werror under -std=c11, -std=c2x, and -std=gnu11; the macro produces no tokens in a normal build.

Opt-in enforcement

A new CMake option AVIF_ENABLE_FBOUNDS_SAFETY (default OFF), modeled on the existing AVIF_ENABLE_COVERAGE block, appends -fbounds-safety to the library targets on Clang (and warns/skips on other compilers). Default builds are unaffected.

Compatibility

ABI- and source-compatible: the annotation is a type attribute that changes neither struct layout nor the public API, and is absent from normal builds. No behavior change unless -fbounds-safety is enabled.

Scope / intentionally excluded

Kept tight to airtight cases. Excluded: every AVIF_ARRAY_DECLARE dynamic array (over-allocated — pointer sized to capacity, not count), avifFileType.compatibleBrands (interior alias into the input stream), and the stride-based pixel/plane buffers (avifRGBImage.pixels/rowBytes, yuvPlanes/yuvRowBytes), whose size is rowBytes * height rather than a single adjacent field. These can follow once refactored into bounds-expressible shapes.

Ask

I can't exercise the enforcing build locally (it needs a Clang with -fbounds-safety). Could you run CI with -DAVIF_ENABLE_FBOUNDS_SAFETY=ON on a capable Clang to validate the enforcing path against the test corpus? Happy to adjust the macro guards or annotation set based on what your toolchain reports.

Introduce a feature-guarded AVIF_COUNTED_BY(N) macro and apply it to the core pointer+count buffers whose pointer provably addresses exactly N elements: avifROData.data, avifRWData.data, and avifImage.properties. Because avifROData/avifRWData are embedded by value across the decode path (icc/exif/xmp, item property payloads, decode/encode samples, the RO/RW stream raw buffers), this propagates bounds information through most of the parsing surface from a single, airtight invariant.

The macro is gated on __has_feature(bounds_safety) && __has_attribute(__counted_by__), so it expands to nothing in normal builds (a no-op; ABI- and source-compatible). A new opt-in CMake option AVIF_ENABLE_FBOUNDS_SAFETY (default OFF) applies -fbounds-safety on Clang to make the annotations enforceable.
@jzern

jzern commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

I can't exercise the enforcing build locally (it needs a Clang with -fbounds-safety). Could you run CI with -DAVIF_ENABLE_FBOUNDS_SAFETY=ON on a capable Clang to validate the enforcing path against the test corpus? Happy to adjust the macro guards or annotation set based on what your toolchain reports.

Note this is an experimental option with many more features. Enabling the flag will result in expectations for all pointers throughout the code base. You can't just apply __counted_by without further annotating the code, starting with __ptrcheck_abi_assume_unsafe_indexable().

include/avif/internal.h:244:43: error: parameter of array type 'const avifImage *[]' (aka 'const struct avifImage *[]') decays to a __single pointer, and will not allow arithmetic
  244 |                                     const avifImage * inputImageItems[],

The feature is currently being developed in https://github.com/swiftlang/llvm-project.git. You can try the stable/20250601 or stable/20260427 for testing. We use this toolchain to verify builds in libwebp. There an active issue to track the work in that library.

@SaudSatopay

Copy link
Copy Markdown
Author

Thanks James — that's really helpful, and you're right. I was treating -fbounds-safety as something you could enable narrowly alongside a few __counted_by annotations, but as you point out it's a whole-TU contract: once the flag is on, every pointer needs a bounds attribute (or an explicit __unsafe_indexable / __ptrcheck_abi_assume_unsafe_indexable() baseline), and the inputImageItems[] decay at internal.h:244 is exactly that. So the opt-in build option in this PR isn't usable as-is.

The right shape is the incremental migration you're doing in libwebp: declare the public headers __ptrcheck_abi_assume_unsafe_indexable() so the library builds under the flag with everything unsafe-indexable by default, then migrate module-by-module (annotating __counted_by/__sized_by, fixing decays like :244), validated by the swiftlang toolchain in CI.

Two questions before I redo this:

  1. Is a libavif -fbounds-safety migration already on your radar / is there a tracking issue I should follow or coordinate with, so I don't duplicate the libwebp-style effort?
  2. If you'd welcome it as an external contribution, I'm glad to start with a foundational PR — the AVIF_COUNTED_BY macro + the __ptrcheck_abi_assume_unsafe_indexable() baseline + the first migrated structs (avifROData/avifRWData) + the internal.h:244 fix — sized so it actually compiles under -DAVIF_ENABLE_FBOUNDS_SAFETY=ON, validated against the stable/20260427 build you linked.

Whichever is most useful — I don't want to duplicate effort you already have underway.

@jzern

jzern commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

cc: @wantehchang

This is something that long term would be nice to have, but isn't a high priority for libavif maintainers as CrabbyAvif has replaced libavif use in some cases that the team is concerned about.

Also, it requires a custom toolchain that would need to be added to the CI. I think it would be easier to do this work after the feature is merged and enabled in the main clang branch.

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.

2 participants