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

[LZ4_compress_destSize] Fix rare data corruption bug #756

Merged
merged 3 commits into from
Jul 17, 2019

Conversation

terrelln
Copy link
Contributor

@terrelln terrelln commented Jul 17, 2019

  • Allow 2 extra match code bytes (line 1033).
  • Fix a rare data corruption bug in LZ4_compress_destSize

When we had to shorten the match to fit in the output buffer, and the new match is before the ip where we found the match because of the "catch up" there will be positions in the table that are beyond the new ip. We must zero those positions so we don't corrupt the hash table.

Credit to OSS-Fuzz

@@ -1038,6 +1066,8 @@ LZ4_FORCE_INLINE int LZ4_compress_generic(
} else
*token += (BYTE)(matchCode);
}
/* Ensure we have enough space for the last literals. */
assert(!(outputDirective == fillOutput && op + 1 + LASTLITERALS > olimit));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This assert isn't quite correct (or is exposing a bug) so I'll look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is triggered because the condition on line 1030 isn't quite right. As I understand it matchCode>>8 has an off by one error in the division. Also, we could need one extra zero byte.

I think we need something like (matchCode+240)/255.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This assert was failing when matchCode == 7665. matchCode >> 8 == 29, but it actually needs 31 bytes since (matchCode + 240) / 255 == 31 (the extra byte is needed because 7665 - 15 is a multiple of 255).

@Cyan4973 Cyan4973 changed the title [LZ4_compress_destSize] Fix rare data corruption bug [LZ4_compress_destSize + multi-blocks streaming] Fix rare data corruption bug Jul 17, 2019
@Cyan4973
Copy link
Member

Looks good to me !
It's great you made this new test tool able to stress LZ4_compress_destSize() more thoroughly.

@Cyan4973 Cyan4973 merged commit 19b0999 into lz4:dev Jul 17, 2019
@terrelln terrelln changed the title [LZ4_compress_destSize + multi-blocks streaming] Fix rare data corruption bug [LZ4_compress_destSize] Fix rare data corruption bug Jul 17, 2019
@terrelln terrelln mentioned this pull request Jul 18, 2019
4 tasks
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

2 participants