From f8ee61e74eb40600445fdb601c374d582e1e9c8a Mon Sep 17 00:00:00 2001 From: Lasse Collin Date: Fri, 9 Sep 2022 13:51:57 +0300 Subject: [PATCH] liblzma: lzma_filters_copy: Keep dest[] unmodified if an error occurs. lzma_stream_encoder() and lzma_stream_encoder_mt() always assumed this. Before this patch, failing lzma_filters_copy() could result in free(invalid_pointer) or invalid memory reads in stream_encoder.c or stream_encoder_mt.c. To trigger this, allocating memory for a filter options structure has to fail. These are tiny allocations so in practice they very rarely fail. Certain badness in the filter chain array could also make lzma_filters_copy() fail but both stream_encoder.c and stream_encoder_mt.c validate the filter chain before trying to copy it, so the crash cannot occur this way. --- src/liblzma/api/lzma/filter.h | 4 +++- src/liblzma/common/filter_common.c | 18 ++++++++++++------ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/liblzma/api/lzma/filter.h b/src/liblzma/api/lzma/filter.h index 8c859314..04825c65 100644 --- a/src/liblzma/api/lzma/filter.h +++ b/src/liblzma/api/lzma/filter.h @@ -108,7 +108,9 @@ extern LZMA_API(lzma_bool) lzma_filter_decoder_is_supported(lzma_vli id) * need to be initialized by the caller in any way. * * If an error occurs, memory possibly already allocated by this function - * is always freed. + * is always freed. liblzma versions older than 5.2.7 may modify the dest + * array and leave its contents in an undefined state if an error occurs. + * liblzma 5.2.7 and newer only modify the dest array when returning LZMA_OK. * * \return - LZMA_OK * - LZMA_MEM_ERROR diff --git a/src/liblzma/common/filter_common.c b/src/liblzma/common/filter_common.c index 9ad5d5d8..590be730 100644 --- a/src/liblzma/common/filter_common.c +++ b/src/liblzma/common/filter_common.c @@ -122,12 +122,16 @@ static const struct { extern LZMA_API(lzma_ret) -lzma_filters_copy(const lzma_filter *src, lzma_filter *dest, +lzma_filters_copy(const lzma_filter *src, lzma_filter *real_dest, const lzma_allocator *allocator) { - if (src == NULL || dest == NULL) + if (src == NULL || real_dest == NULL) return LZMA_PROG_ERROR; + // Use a temporary destination so that the real destination + // will never be modied if an error occurs. + lzma_filter dest[LZMA_FILTERS_MAX + 1]; + lzma_ret ret; size_t i; for (i = 0; src[i].id != LZMA_VLI_UNKNOWN; ++i) { @@ -173,18 +177,20 @@ lzma_filters_copy(const lzma_filter *src, lzma_filter *dest, } // Terminate the filter array. - assert(i <= LZMA_FILTERS_MAX + 1); + assert(i < LZMA_FILTERS_MAX + 1); dest[i].id = LZMA_VLI_UNKNOWN; dest[i].options = NULL; + // Copy it to the caller-supplied array now that we know that + // no errors occurred. + memcpy(real_dest, dest, (i + 1) * sizeof(lzma_filter)); + return LZMA_OK; error: // Free the options which we have already allocated. - while (i-- > 0) { + while (i-- > 0) lzma_free(dest[i].options, allocator); - dest[i].options = NULL; - } return ret; }