Reasoning:
The `sorted` parameter in `BuildHuffmanTable`
(`src/utils/huffman_utils.c:87`) is annotated with
`WEBP_COUNTED_BY_OR_NULL(code_lengths_size)`. Analysis of the
access patterns (lines 137, 177, 207) shows that the indices used
are bounded by `code_lengths_size`. Since `sorted` can be NULL,
`_OR_NULL` is used. When compiling, calls
to `BuildHuffmanTable` in `VP8LBuildHuffmanTable` (line 272) required forging bounds because the `sorted` buffer, allocated
via `WebPSafeMalloc` or on the stack, was treated as unsafe.
`WEBP_UNSAFE_FORGE_BIDI_INDEXABLE` is used at the call sites to
provide the necessary bounds information.
Bug: 432511821
Change-Id: I6fea3ac5d77cb56139f9748ba0277a4f0ad21737
Reasoning:
The `palette` parameter in `GetColorPalette` (src/utils/palette.c:100)
was annotated with `WEBP_COUNTED_BY_OR_NULL(MAX_PALETTE_SIZE)` to fix
an array subscript error at src/utils/palette.c:146. This annotation
reflects the function's contract, documented in src/utils/palette.h,
which states that if `palette` is not NULL, it must point to memory
allocated for at least `MAX_PALETTE_SIZE` elements. To make
`MAX_PALETTE_SIZE` visible, `src/webp/format_constants.h` was included
in `src/utils/palette.h`. Consequently, the wrapper function
`WebPGetColorPalette` (src/utils/utils.c:273) and its declaration in
`src/utils/utils.h` were also annotated similarly, requiring the
inclusion of `src/webp/format_constants.h` in `src/utils/utils.h` as
well. This resolved a compilation error caused by the type mismatch
when calling the annotated `GetColorPalette` from `WebPGetColorPalette`.
Bug: 432511821
Change-Id: Iceebf2facf9558dd49889f056f028d9a3fb22d41
Reasoning:
The struct `VP8LBitWriter` in `src/utils/bit_writer_utils.h` uses
`buf`, `cur`, and `end` pointers to manage a dynamic buffer. Pointer
arithmetic on `buf` and `cur` caused bounds safety errors.
Initial attempts using `WEBP_ENDED_BY(end)` for both `buf` and `cur`
failed due to compiler limitations (one field cannot be the upper
bound for multiple other fields). Using `WEBP_INDEXABLE` for `buf` and
`cur` resolved the arithmetic errors but caused ABI incompatibility
issues, as `VP8LBitWriter` is used in multiple compilation units with
different settings.
The final approach annotates `buf` with `WEBP_ENDED_BY(end)` and `cur`
with `WEBP_UNSAFE_INDEXABLE`. This resolves the arithmetic errors for
both pointers without changing the ABI. `WEBP_UNSAFE_INDEXABLE` is
used for `cur` as a workaround for the `WEBP_ENDED_BY` limitation and
ABI constraints.
Additionally, the `VP8LBitWriterResize` function in
`src/utils/bit_writer_utils.c` was modified:
- The assignments to `bw->buf`, `bw->end`, and `bw->cur` (lines
231-233) were reordered and updated to use the local
`allocated_buf` variable on the right-hand side. This satisfies the
consecutive assignment requirement imposed by `WEBP_ENDED_BY(end)`
on `bw->buf`.
- The local variable `allocated_buf` (line 207) was annotated with
`WEBP_BIDI_INDEXABLE`.
- The allocation for `allocated_buf` (line 222) now uses
`WEBP_UNSAFE_FORGE_BIDI_INDEXABLE` to create a safe, bounded
pointer from the result of `WebPSafeMalloc`, fixing potential type
mismatches when `suppress_fbounds_errors=yes`.
Bug: 432511821
Change-Id: I603a6a7d3ff3bf2a8edf9749a9898ea227c32982
Reasoning:
Analysis of `GetLE32` and its called function `GetLE16`
(src/utils/utils.h:95) showed that `GetLE32` requires access to 4
bytes starting from `data`. `GetLE16` accesses 2 bytes.
Therefore, `WEBP_COUNTED_BY(4)` was added to the `data` parameter of
`GetLE32`. Similarly, `WEBP_COUNTED_BY(2)` was added to `GetLE16`.
During this analysis, related functions `GetLE24`, `PutLE16`,
`PutLE24`, and `PutLE32` (src/utils/utils.h lines 99-123) were also
identified as needing similar annotations. They were annotated with
`WEBP_COUNTED_BY(3)`, `WEBP_COUNTED_BY(2)`, `WEBP_COUNTED_BY(3)`, and
`WEBP_COUNTED_BY(4)` respectively, based on the number of bytes they
access or modify.
Bug: 432511821
Change-Id: I5783392bc8dcaa2f346a81928ef496fc52da3a30
Reasoning:
The `pool` parameter in the static function `SetBitDepths`
(`src/utils/huffman_encode_utils.c:140`) is indexed as an array. Since
the function is static and the exact size is not available in the
signature, `pool` was annotated as `__bidi_indexable`. This required
updating the caller, `GenerateOptimalTree` (also static), where the
corresponding argument `tree_pool` is derived from the `tree` parameter.
Consequently, both the `tree` parameter and the local variable
`tree_pool` in `GenerateOptimalTree`
(`src/utils/huffman_encode_utils.c:170`) were also annotated as
`__bidi_indexable`. Finally, the call to `GenerateOptimalTree` from the
ABI-visible function `VP8LCreateHuffmanTree`
(`src/utils/huffman_encode_utils.c:417`) required adapting the
`huff_tree` pointer. Tracing the allocation in `src/enc/vp8l_enc.c:473`
revealed that `huff_tree` is allocated with enough space for
`3 * max_num_symbols`. Therefore, `__unsafe_forge_bidi_indexable` was
used to cast `huff_tree` to `__bidi_indexable` with the calculated
bounds (`3 * num_symbols`).
Bug: 432511821
Change-Id: I53c6965c3fd708256241a225ec9223085a1ec2d4
Reasoning:
The errors reported for `tokens` in `CodeRepeatedValues`
(src/utils/huffman_encode_utils.c, lines 274, 283, 289, 294)
indicated pointer arithmetic on a pointer assumed to be `__single`.
Since `CodeRepeatedValues` is static and uses `tokens` as an iterator,
its signature was changed to use `HuffmanTreeToken* __indexable` for
both the parameter and return type. A similar static function,
`CodeRepeatedZeros`, was also updated to use `__indexable` pointers.
The caller, `VP8LCreateCompressedHuffmanTree`, passes a buffer `tokens`
with size `max_tokens`. Its signature (in .c and .h files) was
annotated to reflect this using `__counted_by(max_tokens)`. Because
`__counted_by` requires the size parameter to be updated alongside the
pointer if the pointer is modified, the implementation of
`VP8LCreateCompressedHuffmanTree` was refactored. Instead of modifying
the `tokens` parameter directly, a local iterator variable
`current_token` was introduced and explicitly annotated as
`__indexable` to ensure correct type propagation, especially when
`WEBP_ASSUME_UNSAFE_INDEXABLE_ABI` is active. This local iterator is
used for calls to `CodeRepeatedValues` and `CodeRepeatedZeros`.
Bug: 432511821
Change-Id: I07fe553341a613a0ea4d5284817098c31f3aefeb
Reasoning:
Analysis of `VP8LColorCacheInit`
(src/utils/color_cache_utils.c:29) revealed that `colors` is allocated
using `WebPSafeCalloc` with a size of `1 << hash_bits`. Therefore,
`colors` was annotated with `WEBP_COUNTED_BY_OR_NULL(1u << hash_bits)`.
To support this, `WebPSafeCalloc` (src/utils/utils.h:59,
src/utils/utils.c:214) was annotated to return
`WEBP_SIZED_BY_OR_NULL(nmemb * size)`. Since `WebPSafeCalloc` returns a
local pointer that defaults to unsafe when bounds safety is suppressed,
`WEBP_UNSAFE_FORGE_BIDI_INDEXABLE` was used on the return value
(src/utils/utils.c:222). Similarly, `VP8LColorCacheInit` required
`WEBP_UNSAFE_FORGE_BIDI_INDEXABLE` when assigning the allocated pointer
to the struct field (src/utils/color_cache_utils.c:47). Finally,
`VP8LColorCacheInit` and `VP8LColorCacheClear` were modified to perform
side-by-side assignments to `colors` and `hash_bits` as required by the
`WEBP_COUNTED_BY_OR_NULL` annotation, using self-assignment for
`hash_bits` when necessary to maintain functional equivalence with the
original code.
Bug: 432511821
Change-Id: I63cb46909d883a2e8932043ac3117b05b37e8d40
Reasoning:
The compiler reported an error in `src/utils/huffman_utils.c` at line
230 because the array parameter `code_lengths` decayed to a `__single`
pointer, which disallows pointer arithmetic. The parameter
`code_lengths_size` represents the size of the `code_lengths` array.
To fix this, the `code_lengths` parameter in the definition of
`VP8LBuildHuffmanTable` (line 230) and its declaration in
`src/utils/huffman_utils.h` (line 108) were annotated with
`WEBP_COUNTED_BY(code_lengths_size)`.
Bug: 432511821
Change-Id: Id0b32ca283bba5938320abbdbe61846e2d538c5d
Reasoning:
Analysis of initialization (`VP8BitReaderSetBuffer`) and usage sites
(`src/utils/bit_reader_utils.c:59-61`,
`src/utils/bit_reader_inl_utils.h:87`,
`src/utils/bit_reader_utils.c:102`) showed:
- `buf` is the current read pointer, iterating towards `buf_end`.
- `buf_end` points one past the end of valid data (`start + size`).
- `buf_max` points to the last position for a safe read of `lbit_t`.
- All three pointers relate to the same buffer and are adjusted together
in `VP8RemapBitReader`.
Annotations applied:
- `buf` is annotated `__ended_by(buf_end)` as it iterates up to
`buf_end`.
- Self-assignments `br->buf_end = br->buf_end;` are added where
`br->buf` is modified to satisfy the compiler check for `__ended_by`
when the pointer is updated but the end bound remains the same.
- `buf_max` is annotated `__unsafe_indexable` because its arithmetic in
`VP8RemapBitReader` couldn't be easily expressed with other
annotations without ABI changes, and it's mainly adjusted during
setup/remapping.
- The `start` parameters in `VP8BitReaderSetBuffer`
(`src/utils/bit_reader_utils.h:116`,
`src/utils/bit_reader_utils.c:34`) `VP8InitBitReader`
(`src/utils/bit_reader_utils.h:113`, and
`src/utils/bit_reader_utils.c:45`) are annotated `__counted_by(size)`
because they receive a buffer start and size, and `start` using
`size` within `VP8BitReaderSetBuffer`. This resolved errors
arithmetic is performed on within the function and a subsequent
warning when calling it from `VP8InitBitReader`.
Bug: 432511821
Change-Id: Ica436d9764b5bd1f7dcf1b54a280b6f60ecef4df
Reasoning:
The `fbounds-safety` compiler extension reported out-of-bounds accesses
on the `buf` member of the `VP8BitWriter` struct (defined in
`src/utils/bit_writer_utils.h`, line 36). These occurred in
`src/utils/bit_writer_utils.c` at lines 70, 74, 76, and 189, where
`buf` was used with array indexing or pointer arithmetic despite being a
`__single` pointer by default.
To fix this, the `buf` member was annotated as
`__sized_by_or_null(max_pos)` in `src/utils/bit_writer_utils.h`
(line 36), associating it with the `max_pos` member which stores the
buffer size.
This annotation introduced a new build error in the `BitWriterResize`
function (`src/utils/bit_writer_utils.c`, line 55) when assigning
the result of `WebPSafeMalloc` (an `__unsafe_indexable` pointer) to the
now-annotated `bw->buf`. This was resolved by:
1. Using `bw->buf = __unsafe_forge_bidi_indexable(uint8_t*, new_buf,
new_size);` (line 55) to create a properly bounded pointer from the
`malloc` result (`new_buf`) using its size (`new_size`) before
assigning it to `bw->buf`.
Bug: 432511821
Change-Id: I1a24a9a432388ccce53a5e9b3b5e58d22aa61d0c
This reverts commit 61e5c391d6.
When `WEBP_USE_THREAD` is not defined the assignments of *_SSE and their
unsuffixed counterparts may race. Assigning *_SSE directly rather than
relying on the unsuffixed values avoids a case where the *_SSE variants
may refer to the calling function (i.e., AVX2) resulting in infinite
recursion.
Defining `WEBP_USE_THREAD` is recommended when decode/encode calls can
be made from different threads.
In the previous commit (2246828b) not all indices of
`VP8LPredictorsAdd_SSE[]` were assigned a value, namely 14 and 15. These
are used in handling invalid bitstreams to avoid a branch in a hot
function. The indices are now assigned to `PredictorAdd0_SSE2` which
mimics `VP8LPredictorsSub[]` in lossless_enc_sse2.c.
Bug: 435213378, 438295348, 438294044, 438264629, 438294033
Change-Id: I3623717597f0ac6b0d60429adfbb20c611fe6742
WEBP_SELF_ASSIGN exists to clearly designate when a self-assignment is
intentional for the purposes of -fbounds-safety support. It also exists
to silence any compiler warnings that may exist when building without
-fboudnds-safety.
Sample showing behavior across the three major toolchains (clang, gcc,
msvc): https://godbolt.org/z/6r99dncW7
Change-Id: Ibbad5c0c3348dd6e2c4a70297b3e9cb6cf0d2817
Reasoning:
Analysis of the `VP8LBitReader` struct and its initialization function
`VP8LInitBitReader` confirmed that the `len` member holds the size of
the buffer pointed to by `buf`. Therefore, `__counted_by(len)` was added
to `buf` in the struct definition.
This introduced new errors related to function parameters being treated
as `__single` and the `__counted_by` side-by-side assignment rule:
1. In `VP8LInitBitReader`, the `start` parameter was indexed
(`start[i]`) but was implicitly `__single`. It was annotated with
`__counted_by(length)` in both the definition and prototype.
2. `br->buf = start` was moved immediately after `br->len = length`.
3. In `VP8LBitReaderSetBuffer`, the `buf` parameter was assigned to
`br->buf`, which requires the parameter to also be annotated.
`__counted_by(len)` was added to the `buf` parameter in the
definition and `__counted_by(length)` to the `buffer` parameter in
the prototype.
Bug: 432511821
Change-Id: Ie2d5b7b321fb02e8c6b6d3cbd933d056b1bb82cf
Reasoning:
Analysis showed `bit_depths` is passed from `VP8LCreateHuffmanTree` (as
`huff_code->code_lengths`) to `GenerateOptimalTree` (as `bit_depths`
with size `histogram_size` = `huff_code->num_symbols`) and then to
`SetBitDepths`. The `HuffmanTreeCode` struct stores `code_lengths` and
`codes` pointers, both sized by `num_symbols`. These arrays are
allocated in `GetHuffBitLengthsAndCodes` (called by
`EncodeImageInternal`) based on `num_symbols`.
The fix involves:
- Annotating `HuffmanTreeCode::code_lengths` and
`HuffmanTreeCode::codes` with `__counted_by(num_symbols)` in
`src/utils/huffman_encode_utils.h`.
- Annotating the `bit_depths` parameter in `GenerateOptimalTree` with
`__counted_by(histogram_size)` in `src/utils/huffman_encode_utils.c`.
- Annotating the `bit_depths` parameter in `SetBitDepths` with
`__indexable` in `src/utils/huffman_encode_utils.c`, as the size
parameter (`histogram_size`) is not directly available but indexing is
known to be safe based on caller logic (indices `tree->value` are
within `[0, histogram_size - 1]`).
Bug: 432511821
Change-Id: Icfd32f15d0744983b5912d527e5bc59ac58343a5
Reasoning:
The function `VP8LInitBitReader` in `src/utils/bit_reader_utils.c`
takes a pointer `start` and a `length`. Inside the function, `start`
is accessed in a loop (lines 167-168) with index `i` ranging from 0
up to a potentially modified `length` (capped at `sizeof(br->val)` on
lines 164-165). The original `length` parameter accurately describes
the intended size of the buffer pointed to by `start` before this
capping occurs. Therefore, `start` is annotated with
`__counted_by(length)` in both its definition
(src/utils/bit_reader_utils.c:151) and declaration
(src/utils/bit_reader_utils.h:157) to reflect this relationship and
resolve the array subscript error.
Bug: 432511821
Change-Id: Ibefe213e8011ca9b0f6ea4f22651b866261153c5
This reverts commit 2246828be3.
Reason for revert: NULL dereferences in the fuzzers.
The `VP8LPredictorsAdd_SSE` table is not completely initialized (index
14 and 15) which may be accessed with an invalid bitstream.
Bug: 435213378
Original change's description:
> dsp/lossless{,_enc}_sse2.c: reorder *_SSE assignments
>
> When `WEBP_USE_THREAD` is not defined the assignments of *_SSE and their
> unsuffixed counterparts may race. Assigning *_SSE directly rather than
> relying on the unsuffixed values avoids a case where the *_SSE variants
> may refer to the calling function (i.e., AVX2) resulting in infinite
> recursion.
>
> Defining `WEBP_USE_THREAD` is recommended when decode/encode calls can
> be made from different threads.
>
> Bug: 435213378
> Change-Id: Id5549730cb72be99b3014ed8e4e355f3ea988659
Bug: 435213378, 438295348, 438294044, 438264629, 438294033
Change-Id: I3299d6fbb29c45872e2ea1f8f1c3d0ebbda64a69
When `WEBP_USE_THREAD` is not defined the assignments of *_SSE and their
unsuffixed counterparts may race. Assigning *_SSE directly rather than
relying on the unsuffixed values avoids a case where the *_SSE variants
may refer to the calling function (i.e., AVX2) resulting in infinite
recursion.
Defining `WEBP_USE_THREAD` is recommended when decode/encode calls can
be made from different threads.
Bug: 435213378
Change-Id: Id5549730cb72be99b3014ed8e4e355f3ea988659
SRWLOCK is faster than CRITICAL_SECTION. MSVC implements the C11 mtx_t
and C++ std::mutex using SRWLOCK.
Change-Id: I144d2f53aeaa135030ea8c057a2ae8543e6c91b8
Assume the CONDITION_VARIABLE added in Windows Vista is available.
Remove an unneeded WaitForSingleObject() macro that converts
WaitForSingleObject() calls to WaitForSingleObjectEx() calls with
bAlertable=FALSE. The WaitForSingleObject() function does not enter an
alertable wait state, so it is equivalent to WaitForSingleObjectEx()
with bAlertable=FALSE.
Remove code for Windows older than Vista in src/dsp/cpu.h.
Change-Id: I7df95557713923e05a7bfb62e095ec6172cfd708
Trying to compile libwebp with CMAKE_OSX_ARCHITECTURES="x86_64;arm64"
(also called as universal binary), will always fail due to SIMD
intrinsics check. CMake passes `-arch x86_64 -arch arm64` into the
compiler during `check_c_source_compiles`, which will always fail for
any flag, because either x86_64 or arm64 won't have it. This also
results in you not being able to compile the library due to errors like
"SSE2 register return with SSE2 disabled".
Change-Id: I32a41e4c64ce8c6a043083023a4018d512be124b
Import bounds_safety.h across all of webputils, with one exception being
dsp.h, since it's imported by webputils.h in one place. Also prepend
WEBP_ASSUME_UNSAFE_INDEXABLE_ABI to every webputil file to indicate to
the compiler that every pointer should be treated as __unsafe_indexable.
We also need to replace memcpy/memset/memmove with the unsafe variants
WEBP_UNSAFE_*, as memcpy/memset/memmove require bounded/sized pointers.
With this change, all of libwebputils (and libwebp) should build with
-DWEBP_ENABLE_FBOUNDS_SAFETY=true
Change-Id: Iad87be0455182d534c074ef6dc1a30fa66b74b6c
A slim reader/writer (SRW) lock can be initialized statically with the
constant SRWLOCK_INIT. It is the only Windows synchronization object I
can find with this property.
Note: On old Windows versions that don't have SRWLOCK, use the fallback,
thread-unsafe implementation.
Change a NOLINT comment to a NOLINTNEXTLINE comment to prevent
clang-format from aligning the #else and #endif comments in undesired
way.
Bug: 435213378
Change-Id: Iecff615a14a1905aedd2c05ad9444889f711cc17
shellapi.h must be included after windows.h; fixes:
/Windows Kits/10/Include/10.0.26100.0/um/shellapi.h|68 col 1| error:
unknown type name 'EXTERN_C'
when building using VS 2022 after:
44257cb8 apply clang-format
Bug: 433996651
Change-Id: I8f340cdaf6d6b315f1b7535710847db786599da1
Function static variables are initialized on the first call to the
function. In C the initialization of function static variables is not
thread-safe. Use file static variables instead in the
WEBP_DSP_INIT_FUNC() macro.
Remove the volatile qualifier for the pthread version of the
func##_last_cpuinfo_used variable because the variable is only accessed
while holding the mutex.
Change-Id: I1237904a49d2467d7ce79fc53f9e7f966aa7a5c1
Google style, with no overrides. This may help automatic formatting in
some tools that don't pass `--style` by default.
Bug: 433996651
Change-Id: I5fb28f43bd3dd1862aaf89b73a24dfb63f80a193
