Adopt the __counted_by bounds-safety model on core data buffers#3272
Adopt the __counted_by bounds-safety model on core data buffers#3272SaudSatopay wants to merge 1 commit into
Conversation
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.
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 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. |
|
Thanks James — that's really helpful, and you're right. I was treating The right shape is the incremental migration you're doing in libwebp: declare the public headers Two questions before I redo this:
Whichever is most useful — I don't want to duplicate effort you already have underway. |
|
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. |
What
This adopts the C bounds-safety ("Safe Buffers" /
-fbounds-safety) model on libavif's core data buffers: a feature-guardedAVIF_COUNTED_BY(N)macro applied to the data+size pairs whose pointer provably addresses exactlyNelements:avifROData.data→AVIF_COUNTED_BY(size)avifRWData.data→AVIF_COUNTED_BY(size)avifImage.properties→AVIF_COUNTED_BY(numProperties)Because
avifROData/avifRWDataare embedded by value across the decode path (icc/exif/xmp, item property payloads, decode/encode samples, the RO/RW streamrawbuffers), 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_attributeprobe idiom asAVIF_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__andcounted_byspellings are accepted.The headers compile cleanly with
-Wall -Wextra -Werrorunder-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(defaultOFF), modeled on the existingAVIF_ENABLE_COVERAGEblock, appends-fbounds-safetyto 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-safetyis enabled.Scope / intentionally excluded
Kept tight to airtight cases. Excluded: every
AVIF_ARRAY_DECLAREdynamic array (over-allocated — pointer sized tocapacity, notcount),avifFileType.compatibleBrands(interior alias into the input stream), and the stride-based pixel/plane buffers (avifRGBImage.pixels/rowBytes,yuvPlanes/yuvRowBytes), whose size isrowBytes * heightrather 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=ONon 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.