From f94a3e34603c56c55777056bb5412bfd0e948f0b Mon Sep 17 00:00:00 2001 From: Lasse Collin Date: Wed, 23 Nov 2022 21:26:21 +0200 Subject: [PATCH] liblzma: Fix invalid free() after memory allocation failure. The bug was in the single-threaded .xz Stream encoder in the code that is used for both re-initialization and for lzma_filters_update(). To trigger it, an application had to either re-initialize an existing encoder instance with lzma_stream_encoder() or use lzma_filters_update(), and then one of the 1-4 tiny allocations in lzma_filters_copy() (called from stream_encoder_update()) must fail. An error was correctly reported but the encoder state was corrupted. This is related to the recent fix in f8ee61e74eb40600445fdb601c374d582e1e9c8a which is good but it wasn't enough to fix the main problem in stream_encoder.c. --- src/liblzma/common/stream_encoder.c | 39 +++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/src/liblzma/common/stream_encoder.c b/src/liblzma/common/stream_encoder.c index 4dfe88cb..b15229c3 100644 --- a/src/liblzma/common/stream_encoder.c +++ b/src/liblzma/common/stream_encoder.c @@ -233,6 +233,13 @@ stream_encoder_update(void *coder_ptr, const lzma_allocator *allocator, const lzma_filter *reversed_filters) { lzma_stream_coder *coder = coder_ptr; + lzma_ret ret; + + // Make a copy to a temporary buffer first. This way it is easier + // to keep the encoder state unchanged if an error occurs with + // lzma_filters_copy(). + lzma_filter temp[LZMA_FILTERS_MAX + 1]; + return_if_error(lzma_filters_copy(filters, temp, allocator)); if (coder->sequence <= SEQ_BLOCK_INIT) { // There is no incomplete Block waiting to be finished, @@ -240,31 +247,47 @@ stream_encoder_update(void *coder_ptr, const lzma_allocator *allocator, // trying to initialize the Block encoder with the new // chain. This way we detect if the chain is valid. coder->block_encoder_is_initialized = false; - coder->block_options.filters = (lzma_filter *)(filters); - const lzma_ret ret = block_encoder_init(coder, allocator); + coder->block_options.filters = temp; + ret = block_encoder_init(coder, allocator); coder->block_options.filters = coder->filters; if (ret != LZMA_OK) - return ret; + goto error; coder->block_encoder_is_initialized = true; } else if (coder->sequence <= SEQ_BLOCK_ENCODE) { // We are in the middle of a Block. Try to update only // the filter-specific options. - return_if_error(coder->block_encoder.update( + ret = coder->block_encoder.update( coder->block_encoder.coder, allocator, - filters, reversed_filters)); + filters, reversed_filters); + if (ret != LZMA_OK) + goto error; } else { // Trying to update the filter chain when we are already // encoding Index or Stream Footer. - return LZMA_PROG_ERROR; + ret = LZMA_PROG_ERROR; + goto error; } - // Free the copy of the old chain and make a copy of the new chain. + // Free the options of the old chain. for (size_t i = 0; coder->filters[i].id != LZMA_VLI_UNKNOWN; ++i) lzma_free(coder->filters[i].options, allocator); - return lzma_filters_copy(filters, coder->filters, allocator); + // Copy the new filter chain in place. + size_t j = 0; + do { + coder->filters[j].id = temp[j].id; + coder->filters[j].options = temp[j].options; + } while (temp[j++].id != LZMA_VLI_UNKNOWN); + + return LZMA_OK; + +error: + for (size_t i = 0; temp[i].id != LZMA_VLI_UNKNOWN; ++i) + lzma_free(temp[i].options, allocator); + + return ret; }