Skip to content

Leave the store's size and pointers alone in ZopfliCleanLZ77Store, as C does - #211

Open
kunpai wants to merge 1 commit into
google:masterfrom
kunpai:clean-store-keeps-size
Open

kunpai wants to merge 1 commit into
google:masterfrom
kunpai:clean-store-keeps-size

Conversation

@kunpai

@kunpai kunpai commented Oct 28, 2026 •

Copy link
Copy Markdown

Leave the store's size and pointers alone in ZopfliCleanLZ77Store, as C does

Problem

ZopfliCleanLZ77Store (C, Rust) frees a store's buffers. C just frees each pointer and leaves size and the (now dangling) pointers as they were. The port frees each buffer, nulls the pointer, and sets size to 0. So after Clean, the struct a caller still holds reads differently. For example, cleaning a store of 289 commands:

C Rust
store.size after Clean still 289 0
its pointers after Clean the 5 old (dangling) addresses all null

Question

Which behaviour do we want: C's or the Rust port's? Specifically, after ZopfliCleanLZ77Store, should the struct keep its old size and (freed) pointers (like the C source), or be reset to size 0 with null pointers (like the Rust port)?

This PR implements C's behaviour.

Fix

Rust_ZopfliCleanLZ77Store frees each buffer with libc::free without nulling it, and drops the store.size = 0 line:

-            free_c_ptr(&mut store.litlens);
-            free_c_ptr(&mut store.dists);
-            free_c_ptr(&mut store.pos);
-            free_c_ptr(&mut store.ll_counts);
-            free_c_ptr(&mut store.d_counts);
+            libc::free(store.litlens as *mut libc::c_void);
+            libc::free(store.dists as *mut libc::c_void);
+            libc::free(store.pos as *mut libc::c_void);
+            libc::free(store.ll_counts as *mut libc::c_void);
+            libc::free(store.d_counts as *mut libc::c_void);
         }
-        store.size = 0;

Replayed on the 289-command record, before and after:

ZopfliCleanLZ77Store.size C Rust main Rust patched
compared value 289 0 289

The compared size now matches C, and the divergence closes.

Downstream check. The normal path is untouched: ZopfliGzipCompress on the repository's README gives the same 831-byte stream from the port before and after this patch, and gzip.decompress restores the file. C's stream is also 831 bytes but differs in a few bytes, for a separate reason (#213).

Notes for review:

  • This patch reproduces C's undefined behaviour and must not be merged. Leaving the pointers in place after free makes them dangling, and leaving size non-zero means a later read of the struct walks freed memory. The port's reset (null pointers, size 0) is the memory-safe choice and catches a double-free or use-after-free; C's version does not. The PR exists only to show the divergence and ask which post-Clean state is intended, not to ship.

…7Store

C frees each buffer and leaves size and the freed pointers as they
were. The port nulled each pointer and set size to 0. This reproduces
C's dangling state and is not meant to be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant