Zig: Zig compiled miniz stack overflows

[mitkus]

Hi,

Complete Zig noob here so tell me if I'm doing something completely wrong. I'm trying out the language and seamless mixing of C and Zig would be hugely important to me. For my first program, I want to write a png file using miniz library. Here's what I have:

var pix = try alloc.alloc(u8, width * height * 4);
defer alloc.free(pix)
// <...> Skipped writing pixel values
var s: usize = 0;
var png = c.tdefl_write_image_to_png_file_in_memory(@ptrCast(*const c_void, pix.ptr), width, height, 4, &s);

That compiles, but crashes at runtime both in Windows 10 and Arch Linux x64:

Illegal instruction at address 0x7ff773af79d5
C:\Projects\ztrace\c\miniz.c:1085:0: 0x7ff773af79d5 in tdefl_compress_lz_codes (miniz.obj)
    mz_uint8 *pOutput_buf = d->m_pOutput_buf;

???:?:?: 0x140d59051e2 in ??? (???)
The following command exited with error code 2147483651 (expected 0):
cd C:\Projects\ztrace && C:\Projects\ztrace\zig-cache\bin\ztrace.exe

Even though the crash says "illegal instruction", using a debugger I see Windows segfault handler catching stack overflow in zig/std/debug.zig:

switch (info.ExceptionRecord.ExceptionCode) {
   ...
   windows.EXCEPTION_STACK_OVERFLOW => handleSegfaultWindowsExtra(info, 0, "Stack Overflow"),
   ...
}

Equivalent C program works ok and produces no warnings with clang's -fsanitize=address:

byte *pix = malloc(width * height * 4);
// <...>
size_t s = 0;
void* png = tdefl_write_image_to_png_file_in_memory(pix, width, height, 4, &s);

Here's my repo with complete Zig and C sources: https://git.sr.ht/~mitkus/ztrace

[andrewrk]

Illegal instruction

I wrote this FAQ entry just now: https://github.com/ziglang/zig/wiki/FAQ#why-do-i-get-illegal-instruction-when-using-with-zig-cc-to-build-c-code

[LemonBoy]

Quick and dirty patch to make the code compile and run again, UB is nasty.

diff --git a/c/miniz.c b/c/miniz.c
index 9ded4c7..971c88b 100644
--- a/c/miniz.c
+++ b/c/miniz.c
@@ -1102,7 +1102,12 @@ static mz_bool tdefl_compress_lz_codes(tdefl_compressor *d)
         if (flags & 1)
         {
             mz_uint s0, s1, n0, n1, sym, num_extra_bits;
-            mz_uint match_len = pLZ_codes[0], match_dist = *(const mz_uint16 *)(pLZ_codes + 1);
+            mz_uint match_len = pLZ_codes[0], match_dist;
+            {
+                mz_uint16 tmp;
+                memcpy(&tmp, pLZ_codes + 1, sizeof(mz_uint16));
+                match_dist = tmp;
+            }
             pLZ_codes += 3;

             MZ_ASSERT(d->m_huff_code_sizes[0][s_tdefl_len_sym[match_len]]);
@@ -1147,7 +1152,7 @@ static mz_bool tdefl_compress_lz_codes(tdefl_compressor *d)
         if (pOutput_buf >= d->m_pOutput_buf_end)
             return MZ_FALSE;

-        *(mz_uint64 *)pOutput_buf = bit_buffer;
+        memcpy(pOutput_buf, &bit_buffer, sizeof(bit_buffer));
         pOutput_buf += (bits_in >> 3);
         bit_buffer >>= (bits_in & ~7);
         bits_in &= 7;
@@ -1723,7 +1728,7 @@ static mz_bool tdefl_compress_normal(tdefl_compressor *d)
             mz_uint dst_pos = (d->m_lookahead_pos + d->m_lookahead_size) & TDEFL_LZ_DICT_SIZE_MASK, ins_pos = d->m_lookahead_pos + d->m_lookahead_size - 2;
             mz_uint hash = (d->m_dict[ins_pos & TDEFL_LZ_DICT_SIZE_MASK] << TDEFL_LZ_HASH_SHIFT) ^ d->m_dict[(ins_pos + 1) & TDEFL_LZ_DICT_SIZE_MASK];
             mz_uint num_bytes_to_process = (mz_uint)MZ_MIN(src_buf_left, TDEFL_MAX_MATCH_LEN - d->m_lookahead_size);
-            const mz_uint8 *pSrc_end = pSrc + num_bytes_to_process;
+            const mz_uint8 *pSrc_end = pSrc ? pSrc + num_bytes_to_process : NULL;
             src_buf_left -= num_bytes_to_process;
             d->m_lookahead_size += num_bytes_to_process;
             while (pSrc != pSrc_end)

[andrewrk]

I'm sure upstream would be interested in that patch as well. We can be good open source citizens and send it their way.

[andrewrk]

This appears to be Undefined Behavior in the C code; nothing to do on Zig's end. (See the FAQ entry linked earlier for how to disable UBSAN if you want to go that route.)

Please let me know if there is something for Zig to do and I'll re-open.

[mitkus]

Thanks! UB in miniz was indeed the problem. It's neat that Zig helps to catch problems like that.

By default miniz has #define MINIZ_USE_UNALIGNED_LOADS_AND_STORES 1, which suggests that UB-free code wasn't a goal. If I define that to 0 and use latest master branch miniz it works ok with no UB.