From 0c210ca7f489e971e94e1ddc72b0b0806e3c7935 Mon Sep 17 00:00:00 2001 From: Lasse Collin Date: Fri, 6 Jan 2023 22:53:38 +0200 Subject: [PATCH] Tests: test_filter_flags: Clean up minor issues. Here are the list of the most significant issues addressed: - Avoid using internal common.h header. It's not good to copy the constants like this but common.h cannot be included for use outside of liblzma. This is the quickest thing to do that could be fixed later. - Omit the INIT_FILTER macro. Initialization should be done with just regular designated initializers. - Use start_offset = 257 for BCJ tests. It demonstrates that Filter Flags encoder and decoder don't validate the options thoroughly. 257 is valid only for the x86 filter. This is a bit silly but not a significant problem in practice because the encoder and decoder initialization functions will catch bad alignment still. Perhaps this should be fixed but it's not urgent and doesn't need to be in 5.4.x. - Various tweaks to comments such as filter id -> Filter ID --- tests/test_filter_flags.c | 153 +++++++++++++++++++------------------- 1 file changed, 78 insertions(+), 75 deletions(-) diff --git a/tests/test_filter_flags.c b/tests/test_filter_flags.c index 4ddffa7f..a0916ab9 100644 --- a/tests/test_filter_flags.c +++ b/tests/test_filter_flags.c @@ -12,46 +12,41 @@ /////////////////////////////////////////////////////////////////////////////// #include "tests.h" -// Including the internal header file for access to the -// LZMA_FILTER_RESERVED_START macro -#include "common/common.h" - -// Used to create filters and easily to set id and options -#define INIT_FILTER(_id, _options) {\ - .id = _id, \ - .options = _options \ -} +// FIXME: This is from src/liblzma/common/common.h but it cannot be +// included here. This constant is needed in only a few files, perhaps +// move it to some other internal header or create a new one? +#define LZMA_FILTER_RESERVED_START (LZMA_VLI_C(1) << 62) #if defined(HAVE_ENCODERS) // No tests are run without encoders, so init the global filters // only when the encoders are enabled. -static lzma_filter lzma1_filter = INIT_FILTER(LZMA_FILTER_LZMA1, NULL); -static lzma_filter lzma2_filter = INIT_FILTER(LZMA_FILTER_LZMA2, NULL); -static lzma_filter delta_filter = INIT_FILTER(LZMA_FILTER_DELTA, NULL); +static lzma_filter lzma1_filter = { LZMA_FILTER_LZMA1, NULL }; +static lzma_filter lzma2_filter = { LZMA_FILTER_LZMA2, NULL }; +static lzma_filter delta_filter = { LZMA_FILTER_DELTA, NULL }; static lzma_filter bcj_filters_encoders[] = { #ifdef HAVE_ENCODER_X86 - INIT_FILTER(LZMA_FILTER_X86, NULL), + { LZMA_FILTER_X86, NULL }, #endif #ifdef HAVE_ENCODER_POWERPC - INIT_FILTER(LZMA_FILTER_POWERPC, NULL), + { LZMA_FILTER_POWERPC, NULL }, #endif #ifdef HAVE_ENCODER_IA64 - INIT_FILTER(LZMA_FILTER_IA64, NULL), + { LZMA_FILTER_IA64, NULL }, #endif #ifdef HAVE_ENCODER_ARM - INIT_FILTER(LZMA_FILTER_ARM, NULL), + { LZMA_FILTER_ARM, NULL }, #endif #ifdef HAVE_ENCODER_ARM64 - INIT_FILTER(LZMA_FILTER_ARM64, NULL), + { LZMA_FILTER_ARM64, NULL }, #endif #ifdef HAVE_ENCODER_ARMTHUMB - INIT_FILTER(LZMA_FILTER_ARMTHUMB, NULL), + { LZMA_FILTER_ARMTHUMB, NULL }, #endif #ifdef HAVE_ENCODER_SPARC - INIT_FILTER(LZMA_FILTER_SPARC, NULL), + { LZMA_FILTER_SPARC, NULL }, #endif }; @@ -62,25 +57,25 @@ static lzma_filter bcj_filters_encoders[] = { #ifdef HAVE_DECODERS static lzma_filter bcj_filters_decoders[] = { #ifdef HAVE_DECODER_X86 - INIT_FILTER(LZMA_FILTER_X86, NULL), + { LZMA_FILTER_X86, NULL }, #endif #ifdef HAVE_DECODER_POWERPC - INIT_FILTER(LZMA_FILTER_POWERPC, NULL), + { LZMA_FILTER_POWERPC, NULL }, #endif #ifdef HAVE_DECODER_IA64 - INIT_FILTER(LZMA_FILTER_IA64, NULL), + { LZMA_FILTER_IA64, NULL }, #endif #ifdef HAVE_DECODER_ARM - INIT_FILTER(LZMA_FILTER_ARM, NULL), + { LZMA_FILTER_ARM, NULL }, #endif #ifdef HAVE_DECODER_ARM64 - INIT_FILTER(LZMA_FILTER_ARM64, NULL), + { LZMA_FILTER_ARM64, NULL }, #endif #ifdef HAVE_DECODER_ARMTHUMB - INIT_FILTER(LZMA_FILTER_ARMTHUMB, NULL), + { LZMA_FILTER_ARMTHUMB, NULL }, #endif #ifdef HAVE_DECODER_SPARC - INIT_FILTER(LZMA_FILTER_SPARC, NULL), + { LZMA_FILTER_SPARC, NULL }, #endif }; #endif @@ -121,8 +116,8 @@ test_lzma_filter_flags_size(void) assert_true(size != 0 && size < LZMA_BLOCK_HEADER_SIZE_MAX); } - // Test invalid filter ids - lzma_filter bad_filter = INIT_FILTER(2, NULL); + // Test invalid Filter IDs + lzma_filter bad_filter = { 2, NULL }; assert_lzma_ret(lzma_filter_flags_size(&size, &bad_filter), LZMA_OPTIONS_ERROR); @@ -142,38 +137,40 @@ test_lzma_filter_flags_size(void) // Avoid data -> encode -> decode -> compare to data. // Instead create expected encoding and compare to result from // lzma_filter_flags_encode. -// Filter flags for xz are encoded as: +// Filter Flags in .xz are encoded as: // |Filter ID (VLI)|Size of Properties (VLI)|Filter Properties| #if defined(HAVE_ENCODERS) && defined(HAVE_DECODERS) static void verify_filter_flags_encode(lzma_filter *filter, bool should_encode) { uint32_t size = 0; - // First calculate the size of filter flags to know how much - // memory to allocate to hold the filter flags encoded + + // First calculate the size of Filter Flags to know how much + // memory to allocate to hold the encoded Filter Flags assert_lzma_ret(lzma_filter_flags_size(&size, filter), LZMA_OK); uint8_t *encoded_out = tuktest_malloc(size * sizeof(uint8_t)); size_t out_pos = 0; - if(!should_encode) { + if (!should_encode) { assert_false(lzma_filter_flags_encode(filter, encoded_out, &out_pos, size) == LZMA_OK); return; } - // Next encode the filter flags for the provided filter + // Next encode the Filter Flags for the provided filter assert_lzma_ret(lzma_filter_flags_encode(filter, encoded_out, &out_pos, size), LZMA_OK); assert_uint_eq(size, out_pos); - // Next decode the vli for the filter ID and verify it matches - // the expected filter id + + // Next decode the VLI for the Filter ID and verify it matches + // the expected Filter ID size_t filter_id_vli_size = 0; lzma_vli filter_id = 0; assert_lzma_ret(lzma_vli_decode(&filter_id, NULL, encoded_out, &filter_id_vli_size, size), LZMA_OK); assert_uint_eq(filter->id, filter_id); - // Next decode the size of properites and ensure it equals - // the expected size + // Next decode the Size of Properites and ensure it equals + // the expected size. // Expected size should be: // total filter flag length - size of filter id VLI + size of // property size VLI @@ -196,67 +193,72 @@ test_lzma_filter_flags_encode(void) #if !defined(HAVE_ENCODERS) || !defined(HAVE_DECODERS) assert_skip("Encoder or decoder support disabled"); #else - // No test for LZMA1 since the xz format does not support LZMA1 + // No test for LZMA1 since the .xz format does not support LZMA1 // and so the flags cannot be encoded for that filter if (lzma_filter_encoder_is_supported(LZMA_FILTER_LZMA2)) { // Test with NULL options that should fail lzma_options_lzma *options = lzma2_filter.options; lzma2_filter.options = NULL; verify_filter_flags_encode(&lzma2_filter, false); + // Place options back in the filter, and test should pass lzma2_filter.options = options; verify_filter_flags_encode(&lzma2_filter, true); } + // NOTE: Many BCJ filters require that start_offset is a multiple + // of some power of two. The Filter Flags encoder and decoder don't + // completely validate the options and thus 257 passes the tests + // with all BCJ filters. It would be caught when initializing + // a filter chain encoder or decoder. lzma_options_bcj bcj_options = { - .start_offset = 200 + .start_offset = 257 }; for (uint32_t i = 0; i < ARRAY_SIZE(bcj_filters_encoders); i++) { // NULL options should pass for bcj filters verify_filter_flags_encode(&bcj_filters_encoders[i], true); - lzma_filter bcj_with_options = INIT_FILTER( - bcj_filters_encoders[i].id, &bcj_options); + lzma_filter bcj_with_options = { + bcj_filters_encoders[i].id, &bcj_options }; verify_filter_flags_encode(&bcj_with_options, true); } if (lzma_filter_encoder_is_supported(LZMA_FILTER_DELTA)) { - lzma_options_delta delta_ops_below_min = { + lzma_options_delta delta_opts_below_min = { .type = LZMA_DELTA_TYPE_BYTE, .dist = LZMA_DELTA_DIST_MIN - 1 }; - lzma_options_delta delta_ops_above_max = { + lzma_options_delta delta_opts_above_max = { .type = LZMA_DELTA_TYPE_BYTE, .dist = LZMA_DELTA_DIST_MAX + 1 }; verify_filter_flags_encode(&delta_filter, true); - lzma_filter delta_filter_bad_options = INIT_FILTER( - LZMA_FILTER_DELTA, &delta_ops_below_min); + lzma_filter delta_filter_bad_options = { + LZMA_FILTER_DELTA, &delta_opts_below_min }; // Next test error case using minimum - 1 delta distance verify_filter_flags_encode(&delta_filter_bad_options, false); // Next test error case using maximum + 1 delta distance - delta_filter_bad_options.options = &delta_ops_above_max; + delta_filter_bad_options.options = &delta_opts_above_max; verify_filter_flags_encode(&delta_filter_bad_options, false); - // Next test null case + // Next test NULL case delta_filter_bad_options.options = NULL; verify_filter_flags_encode(&delta_filter_bad_options, false); } // Test expected failing cases - lzma_filter bad_filter = INIT_FILTER(LZMA_FILTER_RESERVED_START, - NULL); + lzma_filter bad_filter = { LZMA_FILTER_RESERVED_START, NULL }; size_t out_pos = 0; size_t out_size = LZMA_BLOCK_HEADER_SIZE_MAX; uint8_t out[LZMA_BLOCK_HEADER_SIZE_MAX]; - // Filter id outside of valid range + // Filter ID outside of valid range assert_lzma_ret(lzma_filter_flags_encode(&bad_filter, out, &out_pos, out_size), LZMA_PROG_ERROR); out_pos = 0; @@ -265,7 +267,7 @@ test_lzma_filter_flags_encode(void) out_size), LZMA_PROG_ERROR); out_pos = 0; - // Invalid filter id + // Invalid Filter ID bad_filter.id = 2; assert_lzma_ret(lzma_filter_flags_encode(&bad_filter, out, &out_pos, out_size), LZMA_OPTIONS_ERROR); @@ -294,6 +296,7 @@ test_lzma_filter_flags_encode(void) // Invalid options if (lzma_filter_encoder_is_supported(LZMA_FILTER_DELTA)) { bad_filter.id = LZMA_FILTER_DELTA; + // First test with NULL options assert_lzma_ret(lzma_filter_flags_encode(&bad_filter, out, &out_pos, out_size), LZMA_PROG_ERROR); @@ -325,10 +328,12 @@ verify_filter_flags_decode(lzma_filter *filter_in, lzma_filter *filter_out) assert_lzma_ret(lzma_filter_flags_size(&total_size, filter_in), LZMA_OK); + assert_uint(total_size, >, 0); uint8_t *filter_flag_buffer = tuktest_malloc(total_size); uint32_t properties_size = 0; - size_t out_pos = 0, in_pos = 0; + size_t out_pos = 0; + size_t in_pos = 0; assert_lzma_ret(lzma_properties_size(&properties_size, filter_in), LZMA_OK); assert_lzma_ret(lzma_vli_encode(filter_in->id, NULL, @@ -353,12 +358,11 @@ test_lzma_filter_flags_decode(void) assert_skip("Encoder or decoder support disabled"); #else // For each filter, only run the decoder test if both the encoder - // and decoder is enabled. This is because verify_filter_flags_decode - // uses lzma_filter_flags_size, which requires the encoder. + // and decoder are enabled. This is because verify_filter_flags_decode + // uses lzma_filter_flags_size which requires the encoder. if (lzma_filter_decoder_is_supported(LZMA_FILTER_LZMA2) && lzma_filter_encoder_is_supported(LZMA_FILTER_LZMA2)) { - lzma_filter lzma2_decoded = INIT_FILTER(LZMA_FILTER_LZMA2, - NULL); + lzma_filter lzma2_decoded = { LZMA_FILTER_LZMA2, NULL }; verify_filter_flags_decode(&lzma2_filter, &lzma2_decoded); @@ -376,11 +380,11 @@ test_lzma_filter_flags_decode(void) for (uint32_t i = 0; i < ARRAY_SIZE(bcj_filters_decoders); i++) { if (lzma_filter_encoder_is_supported( bcj_filters_decoders[i].id)) { - lzma_filter bcj_decoded = INIT_FILTER( - bcj_filters_decoders[i].id, NULL); + lzma_filter bcj_decoded = { + bcj_filters_decoders[i].id, NULL }; - lzma_filter bcj_encoded = INIT_FILTER( - bcj_filters_decoders[i].id, NULL); + lzma_filter bcj_encoded = { + bcj_filters_decoders[i].id, NULL }; // First test without options verify_filter_flags_decode(&bcj_encoded, @@ -389,24 +393,22 @@ test_lzma_filter_flags_decode(void) // Next test with offset lzma_options_bcj options = { - .start_offset = 200 + .start_offset = 257 }; bcj_encoded.options = &options; verify_filter_flags_decode(&bcj_encoded, &bcj_decoded); - lzma_options_bcj *decoded_ops = bcj_decoded.options; - assert_uint_eq(decoded_ops->start_offset, + lzma_options_bcj *decoded_opts = bcj_decoded.options; + assert_uint_eq(decoded_opts->start_offset, options.start_offset); - free(decoded_ops); + free(decoded_opts); } - } if (lzma_filter_decoder_is_supported(LZMA_FILTER_DELTA) && lzma_filter_encoder_is_supported(LZMA_FILTER_DELTA)) { - lzma_filter delta_decoded = INIT_FILTER(LZMA_FILTER_DELTA, - NULL); + lzma_filter delta_decoded = { LZMA_FILTER_DELTA, NULL }; verify_filter_flags_decode(&delta_filter, &delta_decoded); lzma_options_delta *expected = delta_filter.options; @@ -421,7 +423,7 @@ test_lzma_filter_flags_decode(void) uint8_t bad_encoded_filter[LZMA_BLOCK_HEADER_SIZE_MAX]; lzma_filter bad_filter; - // Filter outside of valid range + // Filter ID outside of valid range lzma_vli bad_filter_id = LZMA_FILTER_RESERVED_START; size_t bad_encoded_out_pos = 0; size_t in_pos = 0; @@ -437,7 +439,7 @@ test_lzma_filter_flags_decode(void) bad_encoded_out_pos = 0; in_pos = 0; - // Invalid filter Id + // Invalid Filter ID bad_filter_id = 2; bad_encoded_out_pos = 0; in_pos = 0; @@ -446,11 +448,12 @@ test_lzma_filter_flags_decode(void) bad_encoded_filter, &bad_encoded_out_pos, LZMA_BLOCK_HEADER_SIZE_MAX), LZMA_OK); - // Next encode propery size of 0 + // Next encode Size of Properties with the value of 0 assert_lzma_ret(lzma_vli_encode(0, NULL, bad_encoded_filter, &bad_encoded_out_pos, LZMA_BLOCK_HEADER_SIZE_MAX), LZMA_OK); - // Decode should fail on bad filter id + + // Decode should fail on bad Filter ID assert_lzma_ret(lzma_filter_flags_decode(&bad_filter, NULL, bad_encoded_filter, &in_pos, LZMA_BLOCK_HEADER_SIZE_MAX), LZMA_OPTIONS_ERROR); @@ -458,7 +461,7 @@ test_lzma_filter_flags_decode(void) in_pos = 0; // Outsize too small - // Encode the lzma2 filter normally, but then set + // Encode the LZMA2 filter normally, but then set // the out size when decoding as too small if (lzma_filter_encoder_is_supported(LZMA_FILTER_LZMA2) && lzma_filter_decoder_is_supported(LZMA_FILTER_LZMA2)) { @@ -471,8 +474,8 @@ test_lzma_filter_flags_decode(void) LZMA_BLOCK_HEADER_SIZE_MAX), LZMA_OK); assert_lzma_ret(lzma_filter_flags_decode(&bad_filter, NULL, - bad_encoded_filter, &in_pos, - filter_flag_size - 1), LZMA_DATA_ERROR); + bad_encoded_filter, &in_pos, + filter_flag_size - 1), LZMA_DATA_ERROR); } #endif } @@ -499,7 +502,7 @@ main(int argc, char **argv) sizeof(lzma_options_lzma)); lzma_lzma_preset(options, LZMA_PRESET_DEFAULT); lzma2_filter.options = options; - } + } if (lzma_filter_encoder_is_supported(LZMA_FILTER_DELTA)) { lzma_options_delta *options = tuktest_malloc(