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.

diff --git a/src/liblzma/api/lzma/filter.h b/src/liblzma/api/lzma/filter.h
index 8c85931476588c65fd7e2c38a650410c371a4707..04825c655eab9535e3b9f05400128c161b68d259 100644 (file)
--- 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 9ad5d5d8e2af7373e56807f5ddb87a5e5c037895..590be7303febdccab0d23a30bfa21b2cc57afe91 100644 (file)
--- 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;
 }