Skip to content

GitLab

T
tangled
Commit 962cbbe8 authored by Hans Wennborg's avatar Hans Wennborg Committed by Commit Bot
Browse files
Options

[zlib] Zero-initialize the window used for deflation 

Otherwise MSan complains about use-of-uninitialized values in the window.
This happens in both regular deflate's longest_match and deflate_rle.

Before crrev.com/822755 we used to suppress those reports, but it seems
better to fix it properly. That will also allow us to catch other
potential issues with MSan in these functions.

The instances of this that we've seen only reproduce with
fill_window_sse(), not with the regular fill_window() function. Since
the former doesn't exist in upstream zlib, I'm not planning to send this
patch upstream.

Bug: 1137613, 1144420
Change-Id: I2b1801cd2a63fef48a0072b2d2c8fc1f8a7bb920
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2517520
Commit-Queue: Adenilson Cavalcanti <cavalcantii@chromium.org>
Reviewed-by: default avatarAdenilson Cavalcanti <cavalcantii@chromium.org>
Reviewed-by: default avatarChris Blume <cblume@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823845}
parent aa7e9790
Hide whitespace changes
Inline Side-by-side
Showing with 144 additions and 0 deletions
third_party/zlib/contrib/tests/utils_unittest.cc
View file @ 962cbbe8
......@@ -418,3 +418,104 @@ TEST(ZlibTest, CheckMatchCrash) {
memcmp(checkMatchCrashData, decompressed, sizeof(checkMatchCrashData)),
0);
}
TEST(ZlibTest, DeflateRLEUninitUse) {
// MSan would complain about use of uninitialized values in deflate_rle if the
// window isn't zero-initialized. See crbug.com/1137613. Similar problems
// exist in other places in zlib, e.g. longest_match (crbug.com/1144420) but
// we don't have as nice test cases.
int level = 9;
int windowBits = 9;
int memLevel = 8;
int strategy = Z_RLE;
const std::vector<uint8_t> src{
0x31, 0x64, 0x38, 0x32, 0x30, 0x32, 0x30, 0x36, 0x65, 0x35, 0x38, 0x35,
0x32, 0x61, 0x30, 0x36, 0x65, 0x35, 0x32, 0x66, 0x30, 0x34, 0x38, 0x37,
0x61, 0x31, 0x38, 0x36, 0x37, 0x37, 0x31, 0x39, 0x0a, 0x65, 0x62, 0x00,
0x9f, 0xff, 0xc6, 0xc6, 0xc6, 0xff, 0x09, 0x00, 0x62, 0x00, 0x9f, 0xff,
0xc6, 0xc6, 0xc6, 0xff, 0x09, 0x00, 0x62, 0x00, 0x9f, 0xff, 0xc6, 0xc6,
0xc6, 0xff, 0x09, 0x00, 0x62, 0x00, 0x9f, 0xff, 0xc6, 0xc6, 0xc6, 0x95,
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0d, 0x0e, 0x0a, 0x54, 0x52,
0x58, 0x56, 0xab, 0x26, 0x13, 0x53, 0x5a, 0xb5, 0x30, 0xbb, 0x96, 0x44,
0x80, 0xe6, 0xc5, 0x0a, 0xd0, 0x47, 0x7a, 0xa0, 0x4e, 0xbe, 0x30, 0xdc,
0xa1, 0x08, 0x54, 0xe1, 0x51, 0xd1, 0xea, 0xef, 0xdb, 0xa1, 0x2d, 0xb4,
0xb9, 0x58, 0xb1, 0x2f, 0xf0, 0xae, 0xbc, 0x07, 0xd1, 0xba, 0x7f, 0x14,
0xa4, 0xde, 0x99, 0x7f, 0x4d, 0x3e, 0x25, 0xd9, 0xef, 0xee, 0x4f, 0x38,
0x7b, 0xaf, 0x3f, 0x6b, 0x53, 0x5a, 0xcb, 0x1f, 0x97, 0xb5, 0x43, 0xa3,
0xe8, 0xff, 0x09, 0x00, 0x62, 0x00, 0x9f, 0xff, 0xc6, 0xc6, 0xc6, 0xff,
0x09, 0x00, 0x62, 0x00, 0x9f, 0xff, 0xc6, 0xc6, 0xc6, 0xff, 0x09, 0x00,
0x62, 0x00, 0x9f, 0xff, 0xc6, 0xc6, 0xc6, 0xff, 0x09, 0x00, 0x62, 0x00,
0x9f, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x3c,
0x73, 0x70, 0x23, 0x87, 0xec, 0xf8, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0xc1, 0x00, 0x00, 0x9f, 0xc6, 0xc6, 0xff, 0x09, 0x00, 0x62, 0x00, 0x9f,
0xff, 0xc6, 0xc6, 0xc6, 0xff, 0x09, 0x00, 0x62, 0x00, 0x9f, 0xff, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00,
};
z_stream stream;
stream.zalloc = Z_NULL;
stream.zfree = Z_NULL;
// Compress the data one byte at a time to exercise the streaming code.
int ret =
deflateInit2(&stream, level, Z_DEFLATED, windowBits, memLevel, strategy);
ASSERT_EQ(ret, Z_OK);
std::vector<uint8_t> compressed(src.size() * 2 + 1000);
stream.next_out = compressed.data();
stream.avail_out = compressed.size();
for (uint8_t b : src) {
stream.next_in = &b;
stream.avail_in = 1;
ret = deflate(&stream, Z_NO_FLUSH);
ASSERT_EQ(ret, Z_OK);
}
stream.next_in = Z_NULL;
stream.avail_in = 0;
ret = deflate(&stream, Z_FINISH);
ASSERT_EQ(ret, Z_STREAM_END);
deflateEnd(&stream);
}
third_party/zlib/deflate.c
View file @ 962cbbe8
......@@ -321,6 +321,9 @@ int ZEXPORT deflateInit2_(strm, level, method, windowBits, memLevel, strategy,
s->window = (Bytef *) ZALLOC(strm,
s->w_size + window_padding,
2*sizeof(Byte));
/* Avoid use of unitialized values in the window, see crbug.com/1137613 and
* crbug.com/1144420 */
zmemzero(s->window, (s->w_size + window_padding) * (2 * sizeof(Byte)));
s->prev = (Posf *) ZALLOC(strm, s->w_size, sizeof(Pos));
/* Avoid use of uninitialized value, see:
* https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11360
......
third_party/zlib/patches/0007-zero-init-deflate-window.patch 0 → 100644
View file @ 962cbbe8
From 92537ee19784e0e545f06d89b7d89ab532a18cff Mon Sep 17 00:00:00 2001
From: Hans Wennborg <hans@chromium.org>
Date: Tue, 3 Nov 2020 15:54:09 +0100
Subject: [PATCH] [zlib] Zero-initialize the window used for deflation
Otherwise MSan complains about use-of-uninitialized values in the
window.
This happens in both regular deflate's longest_match and deflate_rle.
Before crrev.com/822755 we used to suppress those reports, but it seems
better to fix it properly. That will also allow us to catch other
potential issues with MSan in these functions.
The instances of this that we've seen only reproduce with
fill_window_sse(), not with the regular fill_window() function. Since
the former doesn't exist in upstream zlib, I'm not planning to send this
patch upstream.
Bug: 1137613, 1144420
---
third_party/zlib/deflate.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/third_party/zlib/deflate.c b/third_party/zlib/deflate.c
index 8bf93e524875..fc7ae45905ff 100644
--- a/third_party/zlib/deflate.c
+++ b/third_party/zlib/deflate.c
@@ -321,6 +321,9 @@ int ZEXPORT deflateInit2_(strm, level, method, windowBits, memLevel, strategy,
s->window = (Bytef *) ZALLOC(strm,
s->w_size + window_padding,
2*sizeof(Byte));
+ /* Avoid use of unitialized values in the window, see crbug.com/1137613 and
+ * crbug.com/1144420 */
+ zmemzero(s->window, (s->w_size + window_padding) * (2 * sizeof(Byte)));
s->prev = (Posf *) ZALLOC(strm, s->w_size, sizeof(Pos));
/* Avoid use of uninitialized value, see:
* https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11360
--
2.29.1.341.ge80a0c044ae-goog
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment