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.

• f81deb6 base: have empty_foo() functions use non-NULL ptr
• 8cfde80 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.

• wuffs/internal/cgen/base/memory-public.h

  Line 23 in f32bfe9

  // It returns an empty slice (containing a NULL ptr field) if (num_uxx *

---

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):

• b0e9eff Avoid (NULL + 0) in derived io_buffer variables

---

Coming back to today, Philippe Antoine [email protected] 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.