Empty slice manipulation triggers UBSAN by offsetting from a null pointer.

[sconybeare]

Line numbers and snippets are from commit ad7fda33dd5fa6243cf26df20bed1fbaccfa8601, which is the current head of main.

In C (as opposed to C++), It's undefined behavior to offset a null pointer by any offset at all, at least according to llvm's UBSAN. In at least one part of wuffs-v0.4.c, the end pointer of an empty slice is computed, triggering UB.

Here's the top of a stack trace from apple clang 14.0.3's UBSAN, on arm, when running a simple program that decodes a png and exits:

    #0 0x10001fd20 in wuffs_crc32__ieee_hasher__up__choosy_default wuffs-v0.4.c:33620
    #1 0x10002aa80 in wuffs_crc32__ieee_hasher__up wuffs-v0.4.c:33604
    #2 0x10001e190 in wuffs_crc32__ieee_hasher__update wuffs-v0.4.c:33575
    #3 0x10001e6a8 in wuffs_crc32__ieee_hasher__update_u32 wuffs-v0.4.c:33593
    #4 0x1001523fc in wuffs_png__decoder__decode_pass wuffs-v0.4.c:57676

My understanding of what's going on is that on line 57676, we do something with an empty slice:

v_checksum_have = wuffs_crc32__ieee_hasher__update_u32(&self->private_data.f_crc32, wuffs_base__utility__empty_slice_u8());

which ultimately leads to UB at line 33620 (4 stack frames deeper):

uint8_t* i_end0_p = v_p.ptr + (((i_slice_p.len - (size_t)(v_p.ptr - i_slice_p.ptr)) / 32) * 32);

in which v_p.ptr and the offset are both zero.

I'm not at all familiar with wuffs-the-language or its compiler, so I'm not sure what a proper fix would look like. However, I was able to eliminate the error in my own program by changing

static inline wuffs_base__slice_u8  //
wuffs_base__empty_slice_u8(void) {
  wuffs_base__slice_u8 ret;
  ret.ptr = NULL;
  ret.len = 0;
  return ret;
}

to

static inline wuffs_base__slice_u8  //
wuffs_base__empty_slice_u8(void) {
  static const uint8_t placeholder_object; // const so that accidental writes will hit read-only memory and crash
  wuffs_base__slice_u8 ret;
  ret.ptr = (uint8_t *) &placeholder_object;
  ret.len = 0;
  return ret;
}

in wuffs-v0.4.c.

[nigeltao]

Thanks for the bug report.

[nigeltao]

A Wuffs slice is a pointer-length pair. These two commits from last October (2023) changed Wuffs' representation of "an empty slice" from (null, 0) to (nonNull, 0), as suggested by the original bug report.

• https://github.com/google/wuffs/commit/f81deb6b6ed823d72301ea2487a0dddf12b80a4e base: have empty_foo() functions use non-NULL ptr
• https://github.com/google/wuffs/commit/8cfde80759598fc46147d5f0c2c729d708330a38 aux: fix for empty_foo() returning non-NULL ptr

The wuffs_base__empty_slice_u8 function has been around since version 0.2 (released in 2019), returning a null pointer at the time, but the notNull behavior change from those two commits debuted in version 0.4.0-alpha.3 (very recent; March 2024). This is just an 'alpha' release, not a 'final' 0.4.

The wuffs_base__malloc_slice_u8 function is actually documented to return a null pointer on failure. We changed the behavior (it still returns an empty slice) but did not update the doc comment.

• https://github.com/google/wuffs/blob/f32bfe95c6212bd1c9212da8778299a92e603714/internal/cgen/base/memory-public.h#L23

---

In C (but not in C++), it's Undefined Behavior to compute (ptr + len) when the pointer is null, even if the length is zero. The commit message from a 2022 Wuffs commit actually discusses this briefly (with reference to the C spec):

• https://github.com/google/wuffs/commit/b0e9efff3f44dc150edb22a27297bfef668035d2 Avoid (NULL + 0) in derived io_buffer variables

---

Coming back to today, Philippe Antoine p.antoine@catenacyber.fr notes that fuzz/c/fuzzlib/fuzzlib_image_decoder.c has been around since at least 2020 and calls free(workbuf.ptr), which is a no-op if workbuf.ptr is null but actually a crasher if workbuf.ptr is the placeholder nonNull pointer we now use for an empty slice.

I'm going to revert those top two commits, changing it back so that the idiomatic Wuffs representation of an empty slice is (null, 0) and that wuffs_base__malloc_slice_u8 returns (null, 0) so that free(workbuf.ptr) is legitmate.

I'll also patch the C code generation for Wuffs' iterate loops so that the original bug report doesn't re-appear.