Add avifCalloc() and harden allocation call sites#3210
Open
metsw24-max wants to merge 2 commits into
Open
Conversation
wantehchang
reviewed
May 19, 2026
| // allocation failure, if either argument is 0, or if count * size overflows size_t. | ||
| // Using avifCalloc() instead of avifAlloc() + memset() ensures the multiplication | ||
| // is overflow-checked at a single, audited site. | ||
| AVIF_API void * avifCalloc(size_t count, size_t size); |
Member
There was a problem hiding this comment.
Please move the declaration of avifCalloc() to include/avif/internal.h and remove AVIF_API.
The avifAlloc() and avifFree() functions were exported in the public interface unintentionally. These functions are for internal use by libavif.
|
|
||
| void * avifCalloc(size_t count, size_t size) | ||
| { | ||
| // Match the contract documented in avif.h: reject zero or overflow rather than |
Member
There was a problem hiding this comment.
(Please see my comment at line 24 below.) Remove "or overflow" from this comment.
| if (count == 0 || size == 0) { | ||
| return NULL; | ||
| } | ||
| if (count > SIZE_MAX / size) { |
Member
There was a problem hiding this comment.
I think we can just call calloc(count, size) here.
If you agree, also remove #include <stdint.h> and #include <string.h>.
wantehchang
reviewed
May 19, 2026
| // Allocates count * size bytes and zero-initializes them. Returns NULL on memory | ||
| // allocation failure, if either argument is 0, or if count * size overflows size_t. | ||
| // Using avifCalloc() instead of avifAlloc() + memset() ensures the multiplication | ||
| // is overflow-checked at a single, audited site. |
Member
There was a problem hiding this comment.
Nit: Please change this comment block to the following:
// Allocates count * size bytes and zero-initializes them. Returns NULL on memory
// allocation failure, including the case when count * size overflows size_t.
| goto cleanup; | ||
| } | ||
| uvPlanes = avifAlloc(uvSize); | ||
| uvPlanes = avifCalloc(1, uvSize); |
Member
There was a problem hiding this comment.
Nit: this should be
uvPlanes = avifCalloc(uvSize, 1);
where 1 could be changed to sizeof(uint8_t).
| const int maxNumBuckets = 10000; | ||
| const int numBuckets = AVIF_MIN((int)ceilf((max - min) / bucketSize), maxNumBuckets); | ||
| int * histogram = avifAlloc(sizeof(int) * numBuckets); | ||
| int * histogram = avifCalloc((size_t)numBuckets, sizeof(int)); |
Member
There was a problem hiding this comment.
Nit: The size_t cast shouldn't be necessary. Did you get a compiler warning without the size_t cast?
Addressed: - Move avifCalloc() declaration from the public avif.h to internal.h and drop AVIF_API; avifAlloc()/avifFree() were exported in the public interface unintentionally and the new helper is for internal use only. - Update the declaration comment to describe behavior (incl. the overflow case) rather than the rationale for the helper. - Simplify the body to a single call to libc calloc(), which already handles count*size overflow per C11. Drop the now-unused <stdint.h> and <string.h> includes. - codec_svt.c: pass uvSize as the element count, sizeof(uint8_t) as the element size, matching the count-then-size convention. - gainmap.c: drop the redundant (size_t) cast on numBuckets.
Contributor
Author
|
Addressed:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce avifCalloc() as a centralized overflow-checked, zero-initializing allocation helper and convert existing open-coded allocation patterns to use it.
This change replaces repeated patterns of:
with a single reusable helper that performs allocation arithmetic validation and zero-initialization in one place.
Changes
Add new public allocation helper:
avifCalloc(size_t count, size_t size)Centralize:
Convert existing allocation sites across multiple files from:
avifAlloc(...) + memset(...)avifAlloc(count * sizeof(...))to:
avifCalloc(...)Security / Hardening Impact
This is a defensive hardening change and does not modify codec, parsing, or image-processing logic.
The patch reduces reliance on open-coded allocation arithmetic and removes repeated allocation+memset patterns throughout the codebase.
Several converted call sites previously performed
count * sizeof(T)calculations directly before allocation. These allocations are now validated centrally throughavifCalloc(), preventing unchecked size multiplication from propagating to allocation routines.