From 3f9105fbf183f7400021b081a3279c8feb6ea69e Mon Sep 17 00:00:00 2001 From: Joel Rosdahl Date: Thu, 6 Jun 2019 13:44:16 +0200 Subject: [PATCH] Improve error handling of (de)compressors MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Previously, some kinds of corruption were not detected by the zlib decompressor since it didn’t check that it had reached the end of the stream and therefore didn’t verify the Adler-32 checksum. --- src/ccache.c | 5 ++++- src/compr_none.c | 5 +++-- src/compr_zlib.c | 13 ++++++++++++- src/compression.h | 4 ++-- src/decompr_none.c | 6 ++++-- src/decompr_zlib.c | 23 ++++++++++++++++++++++- src/result.c | 19 +++++++++++++------ unittest/test_compr_zlib.c | 17 ++++++++--------- 8 files changed, 68 insertions(+), 24 deletions(-) diff --git a/src/ccache.c b/src/ccache.c index 903c4b428..4370b6e12 100644 --- a/src/ccache.c +++ b/src/ccache.c @@ -3889,7 +3889,10 @@ ccache_main_options(int argc, char *argv[]) case DUMP_RESULT: initialize(); - cache_dump(optarg, stdout); + if (!cache_dump(optarg, stdout)) { + fprintf(stderr, "Error: Corrupt result file\n"); + return 1; + } break; case HASH_FILE: diff --git a/src/compr_none.c b/src/compr_none.c index d16708d07..2aec2a22b 100644 --- a/src/compr_none.c +++ b/src/compr_none.c @@ -30,10 +30,11 @@ compr_none_write(struct compr_state *handle, const void *data, size_t size) return fwrite(data, 1, size, output) == size; } -static void +static bool compr_none_free(struct compr_state *handle) { - (void)handle; + FILE *output = (FILE *)handle; + return ferror(output) == 0; } struct compressor compr_none = { diff --git a/src/compr_zlib.c b/src/compr_zlib.c index 0462d50fe..83cc28e79 100644 --- a/src/compr_zlib.c +++ b/src/compr_zlib.c @@ -23,6 +23,7 @@ struct state { FILE *output; z_stream stream; + bool failed; }; static struct compr_state * @@ -33,6 +34,7 @@ compr_zlib_init(FILE *output, int level) state->stream.zalloc = Z_NULL; state->stream.zfree = Z_NULL; state->stream.opaque = Z_NULL; + state->failed = false; int ret = deflateInit(&state->stream, level); if (ret != Z_OK) { @@ -45,6 +47,9 @@ compr_zlib_init(FILE *output, int level) static bool compr_zlib_write(struct compr_state *handle, const void *data, size_t size) { + if (!handle) { + return false; + } struct state *state = (struct state *)handle; state->stream.next_in = (const Bytef *)data; @@ -62,6 +67,7 @@ compr_zlib_write(struct compr_state *handle, const void *data, size_t size) unsigned int compressed_bytes = sizeof(buffer) - state->stream.avail_out; if (fwrite(buffer, 1, compressed_bytes, state->output) != compressed_bytes || ferror(state->output)) { + state->failed = true; return false; } } while (state->stream.avail_out == 0); @@ -71,14 +77,19 @@ compr_zlib_write(struct compr_state *handle, const void *data, size_t size) return true; } -static void +static bool compr_zlib_free(struct compr_state *handle) { + if (!handle) { + return false; + } struct state *state = (struct state *)handle; compr_zlib_write(handle, NULL, 0); deflateEnd(&state->stream); + bool success = !state->failed; free(state); + return success; } struct compressor compr_zlib = { diff --git a/src/compression.h b/src/compression.h index a48ab0321..89bf1469f 100644 --- a/src/compression.h +++ b/src/compression.h @@ -8,7 +8,7 @@ struct compr_state; struct compressor { struct compr_state *(*init)(FILE *output, int compression_level); bool (*write)(struct compr_state *state, const void *data, size_t size); - void (*free)(struct compr_state *state); + bool (*free)(struct compr_state *state); }; struct decompr_state; @@ -16,7 +16,7 @@ struct decompr_state; struct decompressor { struct decompr_state *(*init)(FILE *input); bool (*read)(struct decompr_state *state, void *data, size_t size); - void (*free)(struct decompr_state *state); + bool (*free)(struct decompr_state *state); }; extern struct compressor compr_none; diff --git a/src/decompr_none.c b/src/decompr_none.c index aa96f0623..4087bb957 100644 --- a/src/decompr_none.c +++ b/src/decompr_none.c @@ -29,9 +29,11 @@ decompr_none_read(struct decompr_state *handle, void *data, size_t size) return fread(data, 1, size, input) == size; } -static void decompr_none_free(struct decompr_state *handle) +static bool +decompr_none_free(struct decompr_state *handle) { - (void)handle; + FILE *input = (FILE *)handle; + return ferror(input) == 0; } struct decompressor decompr_none = { diff --git a/src/decompr_zlib.c b/src/decompr_zlib.c index 3c97e67e3..6967f66dd 100644 --- a/src/decompr_zlib.c +++ b/src/decompr_zlib.c @@ -19,6 +19,12 @@ #include +enum stream_state { + STREAM_STATE_READING, + STREAM_STATE_FAILED, + STREAM_STATE_END +}; + struct state { FILE *input; @@ -26,6 +32,7 @@ struct state size_t input_size; size_t input_consumed; z_stream stream; + enum stream_state stream_state; }; static struct decompr_state * @@ -41,6 +48,7 @@ decompr_zlib_init(FILE *input) state->stream.opaque = Z_NULL; state->stream.avail_in = 0; state->stream.next_in = Z_NULL; + state->stream_state = STREAM_STATE_READING; int ret = inflateInit(&state->stream); if (ret != Z_OK) { @@ -53,6 +61,9 @@ decompr_zlib_init(FILE *input) static bool decompr_zlib_read(struct decompr_state *handle, void *data, size_t size) { + if (!handle) { + return false; + } struct state *state = (struct state *)handle; size_t bytes_read = 0; @@ -62,6 +73,7 @@ decompr_zlib_read(struct decompr_state *handle, void *data, size_t size) state->input_size = fread( state->input_buffer, 1, sizeof(state->input_buffer), state->input); if (state->input_size == 0) { + state->stream_state = STREAM_STATE_FAILED; return false; } state->input_consumed = 0; @@ -79,7 +91,11 @@ decompr_zlib_read(struct decompr_state *handle, void *data, size_t size) case Z_NEED_DICT: case Z_DATA_ERROR: case Z_MEM_ERROR: + state->stream_state = STREAM_STATE_FAILED; return false; + case Z_STREAM_END: + state->stream_state = STREAM_STATE_END; + break; } bytes_read = size - state->stream.avail_out; state->input_consumed = state->input_size - state->stream.avail_in; @@ -88,11 +104,16 @@ decompr_zlib_read(struct decompr_state *handle, void *data, size_t size) return true; } -static void decompr_zlib_free(struct decompr_state *handle) +static bool decompr_zlib_free(struct decompr_state *handle) { + if (!handle) { + return false; + } struct state *state = (struct state *)handle; inflateEnd(&state->stream); + bool success = state->stream_state == STREAM_STATE_END; free(handle); + return success; } struct decompressor decompr_zlib = { diff --git a/src/result.c b/src/result.c index 43db2b57a..c7cc5735c 100644 --- a/src/result.c +++ b/src/result.c @@ -196,6 +196,10 @@ read_cache(const char *path, struct filelist *l, FILE *dump_stream) } decompr_state = decompressor->init(f); + if (!decompr_state) { + cc_log("Failed to initialize decompressor"); + goto out; + } if (dump_stream) { const uint8_t compr_level = header[6]; @@ -287,8 +291,8 @@ out: if (subfile) { fclose(subfile); } - if (decompressor) { - decompressor->free(decompr_state); + if (decompressor && !decompressor->free(decompr_state)) { + success = false; } if (f) { fclose(f); @@ -401,10 +405,13 @@ bool cache_put(const char *cache_path, struct filelist *l, compressor = &compr_zlib; } - struct compr_state *compr_state = - compressor->init(f, compression_level); - bool ok = write_cache(l, compressor, compr_state); - compressor->free(compr_state); + struct compr_state *compr_state = compressor->init(f, compression_level); + if (!compr_state) { + cc_log("Failed to initialize compressor"); + goto out; + } + bool ok = write_cache(l, compressor, compr_state) + && compressor->free(compr_state); if (!ok) { cc_log("Failed to write cache file"); goto out; diff --git a/unittest/test_compr_zlib.c b/unittest/test_compr_zlib.c index ebbab2ada..f5bc1a142 100644 --- a/unittest/test_compr_zlib.c +++ b/unittest/test_compr_zlib.c @@ -28,7 +28,7 @@ TEST(zlib_small_roundtrip) CHECK(compr_zlib.write(c_state, "foobar", 6)); - compr_zlib.free(c_state); + CHECK(compr_zlib.free(c_state)); fclose(f); f = fopen("data.zlib", "r"); @@ -44,7 +44,8 @@ TEST(zlib_small_roundtrip) // Nothing left to read. CHECK(!decompr_zlib.read(d_state, buffer, 1)); - decompr_zlib.free(d_state); + // Error state is remembered. + CHECK(!decompr_zlib.free(d_state)); fclose(f); } @@ -60,7 +61,7 @@ TEST(zlib_large_compressible_roundtrip) CHECK(compr_zlib.write(c_state, data, sizeof(data))); } - compr_zlib.free(c_state); + CHECK(compr_zlib.free(c_state)); fclose(f); f = fopen("data.zlib", "r"); @@ -76,7 +77,8 @@ TEST(zlib_large_compressible_roundtrip) // Nothing left to read. CHECK(!decompr_zlib.read(d_state, buffer, 1)); - decompr_zlib.free(d_state); + // Error state is remembered. + CHECK(!decompr_zlib.free(d_state)); fclose(f); } @@ -93,7 +95,7 @@ TEST(zlib_large_uncompressible_roundtrip) CHECK(compr_zlib.write(c_state, data, sizeof(data))); - compr_zlib.free(c_state); + CHECK(compr_zlib.free(c_state)); fclose(f); f = fopen("data.zlib", "r"); @@ -104,10 +106,7 @@ TEST(zlib_large_uncompressible_roundtrip) CHECK(decompr_zlib.read(d_state, buffer, sizeof(buffer))); CHECK(memcmp(buffer, data, sizeof(data)) == 0); - // Nothing left to read. - CHECK(!decompr_zlib.read(d_state, buffer, 1)); - - decompr_zlib.free(d_state); + CHECK(decompr_zlib.free(d_state)); fclose(f); } -- 2.47.2