Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deflate: Better Huffman.construct errors and error handling #9880

Merged
merged 2 commits into from Oct 4, 2021

Conversation

squeek502
Copy link
Collaborator

@squeek502 squeek502 commented Oct 2, 2021

This brings Huffman.construct error handling in line with puff.c (I'm assuming we're okay with treating puff.c as an oracle). The error names and logic are from puff.c (and zlib's inflate.c uses the same terminology).

Note: I am planning to add relevant test cases soon. EDIT: Done


This is the last piece of the puzzle for getting the deflate.zig <-> puff.c comparison fuzzer (which checks that all inputs either give the same inflated output or give errors in both implementations) to run cleanly (so far at least). Other required puzzle pieces:

cc @LemonBoy @mattbork

This brings construct error handling in line with puff.c
@squeek502 squeek502 marked this pull request as draft October 2, 2021 05:41
@squeek502
Copy link
Collaborator Author

Test cases added. Note that #9860 should almost certainly be merged before this (I tested locally with those changes included, so if this PR fails in CI then it's likely because #9860 needs to be merged first).

@squeek502 squeek502 marked this pull request as ready for review October 2, 2021 10:42
@@ -125,6 +127,9 @@ const Huffman = struct {

self.last_code = codes[PREFIX_LUT_BITS + 1];
self.last_index = offset[PREFIX_LUT_BITS + 1] - self.count[PREFIX_LUT_BITS + 1];

if (left > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, why not move the check below the loop that computes left?

Copy link
Collaborator Author

@squeek502 squeek502 Oct 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to potentially leave self.last_code/self.last_index uninitialized since error.IncompleteSet is not always treated as an error by the caller, and if decode uses those fields when uninitialized it can lead to index-out-of-bounds/UB.

@andrewrk andrewrk merged commit d4ebfa8 into ziglang:master Oct 4, 2021
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.

None yet

4 participants