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:
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
(Debian clang-format version 19.1.7 (3+build4)) with `--style=Google`.
Manual changes:
* clang-format disabled around macros with stringification (mostly
assembly)
* some inline assembly strings were adjusted to avoid awkward line
breaks
* trailing commas, `//` or suffixes (`ull`) added to help array
formatting
* thread_utils.c: parameter comments were changed to the more common
/*...=*/ style to improve formatting
The automatically generated code under swig/ was skipped.
Bug: 433996651
Change-Id: Iea3f24160d78d2a2653971cdf13fa932e47ff1b3
Rather than encoding all candidates then finding the smallest output,
only keep the frame candidate leading to the shortest bitstream at all
times. Some memory is released slightly sooner when there are more
than two frame candidates.
The simple assignments in PickBestCandidate() are now done for each
new best candidate so far instead of once for the one best overall
candidate, but they are cheap and overwritten. This change should
have no impact on encoded animation files.
Change-Id: Ie540f104a70de7a84535f8f2c49d1a71fa5b4bfa
`ok &= ...` is more common in the codebase than `ok = ... && ok` when
accumulating a result while unconditionally executing functions. (`ok =
ok &&` is used in cases that should short circuit.) In this case
multiple checks may fail and their error messages may aid in debugging.
This will also improve the formatting when clang-format is applied to
the codebase.
Bug: 433996651
Change-Id: Ie4e2908b857122d90f6e93f06b10cb48dc86b18e
in calls to `LOCAL_FREE()`: `(W_CHAR** const) -> (W_CHAR**)`. This will
improve formatting when clang-format is applied.
Bug: 433996651
Change-Id: I54e62206c83473e9369b8c67fb8d6c44d7808d52
When the next frame is forced to be a keyframe by setting
enc->best_delta to DELTA_INFINITY, setting enc->keyframe to NONE means
the current frame will never be converted from a candidate keyframe to
a subframe. So enc->prev_candidate_undecided should be set to 0.
This is a no-op change because enc->prev_candidate_undecided is only
used to update prev_rect and dispose method, which is of no value when
the next frame is a key frame.
Change-Id: Ie51c7e3b6ba797b75af5e06439e8568d8185693f
