From a68eb730558fb90bc8d9524564d8f9e58f3ccac1 Mon Sep 17 00:00:00 2001 From: David Oberhollenzer Date: Mon, 7 Oct 2019 11:20:55 +0200 Subject: Improve ABI backward & forward compatibillity - Padd the compressor config union - 128 bytes aught to be enough for everyone, i.e. future compressors. - Insist that the padding space is initialized to 0. If a field gets added to an existing compressor, it can test for 0 as a sentinel value. - Add a size field to the hook structure, aka "the Microsoft way". - The explanation is in the comment. - Don't make the Microsoft mistake of checking for >=, insist on *exact* size match. Future users will need a fallback if their hooks are rejected. But at least they will be rejected instead of silently not being used. - Add an unsupported flag check to the dir tree reader. - Add a basic abi unit test that, for now, checks the size of the compressor config struct fields. Signed-off-by: David Oberhollenzer --- include/sqfs/compressor.h | 15 +++++++++++++++ include/sqfs/data_writer.h | 19 +++++++++++++++++-- include/sqfs/dir_reader.h | 2 ++ lib/sqfs/comp/compressor.c | 30 ++++++++++++++++++++++++++++++ lib/sqfs/data_writer/common.c | 8 ++++++-- lib/sqfs/read_tree.c | 3 +++ lib/sqfshelper/statistics.c | 1 + tests/Makemodule.am | 6 +++++- tests/abi.c | 25 +++++++++++++++++++++++++ 9 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 tests/abi.c diff --git a/include/sqfs/compressor.h b/include/sqfs/compressor.h index 46bdf58..a53fbc1 100644 --- a/include/sqfs/compressor.h +++ b/include/sqfs/compressor.h @@ -100,6 +100,11 @@ struct sqfs_compressor_t { * @struct sqfs_compressor_config_t * * @brief Configuration parameters for instantiating a compressor backend. + * + * The unused fields MUST be set to 0. The easiest way to do this is by always + * clearing the struct using memset before setting anything, or using + * @ref sqfs_compressor_config_init to set defaults and then modify the struct + * from there. */ struct sqfs_compressor_config_t { /** @@ -138,6 +143,8 @@ struct sqfs_compressor_config_t { * Default is 15, i.e. 32k window. */ sqfs_u16 window_size; + + sqfs_u32 padd0[3]; } gzip; /** @@ -150,6 +157,8 @@ struct sqfs_compressor_config_t { * Default is 15. */ sqfs_u16 level; + + sqfs_u16 padd0[7]; } zstd; /** @@ -174,6 +183,8 @@ struct sqfs_compressor_config_t { * Defaults to 9, i.e. best compression. */ sqfs_u16 level; + + sqfs_u32 padd0[3]; } lzo; /** @@ -190,7 +201,11 @@ struct sqfs_compressor_config_t { * block size. */ sqfs_u32 dict_size; + + sqfs_u32 padd0[3]; } xz; + + sqfs_u64 padd0[2]; } opt; }; diff --git a/include/sqfs/data_writer.h b/include/sqfs/data_writer.h index 210a08d..4623493 100644 --- a/include/sqfs/data_writer.h +++ b/include/sqfs/data_writer.h @@ -58,6 +58,18 @@ * The callbacks can be individually set to NULL to disable them. */ struct sqfs_block_hooks_t { + /** + * @brief Set this to the size of the struct. + * + * This is required for future expandabillity while maintaining ABI + * compatibillity. At the current time, the implementation of + * @ref sqfs_data_writer_set_hooks rejects any hook struct where this + * isn't the exact size. If new hooks are added in the future, the + * struct grows and the future implementation can tell by the size + * whether the application uses the new version or the old one. + */ + size_t size; + /** * @brief Gets called before writing a block to disk. * @@ -283,10 +295,13 @@ int sqfs_data_writer_write_fragment_table(sqfs_data_writer_t *proc, * @param proc A pointer to a data writer object. * @param user_ptr A user pointer to pass to the callbacks. * @param hooks A structure containing the hooks. + * + * @return Zero on success, @ref SQFS_ERROR_UNSUPPORTED if the size field of + * the hooks doesn't match any size knwon to the library. */ SQFS_API -void sqfs_data_writer_set_hooks(sqfs_data_writer_t *proc, void *user_ptr, - const sqfs_block_hooks_t *hooks); +int sqfs_data_writer_set_hooks(sqfs_data_writer_t *proc, void *user_ptr, + const sqfs_block_hooks_t *hooks); #ifdef __cplusplus } diff --git a/include/sqfs/dir_reader.h b/include/sqfs/dir_reader.h index 8377fdd..ec84d98 100644 --- a/include/sqfs/dir_reader.h +++ b/include/sqfs/dir_reader.h @@ -99,6 +99,8 @@ typedef enum { * of parent nodes with the subtree stored at the end. */ SQFS_TREE_STORE_PARENTS = 0x40, + + SQFS_TREE_ALL_FLAGS = 0x7F, } E_SQFS_TREE_FILTER_FLAGS; /** diff --git a/lib/sqfs/comp/compressor.c b/lib/sqfs/comp/compressor.c index bfc20b7..902ce92 100644 --- a/lib/sqfs/comp/compressor.c +++ b/lib/sqfs/comp/compressor.c @@ -87,12 +87,42 @@ bool sqfs_compressor_exists(E_SQFS_COMPRESSOR id) sqfs_compressor_t *sqfs_compressor_create(const sqfs_compressor_config_t *cfg) { + sqfs_u8 padd0[sizeof(cfg->opt)]; + int ret; + if (cfg == NULL || cfg->id < SQFS_COMP_MIN || cfg->id > SQFS_COMP_MAX) return NULL; if (compressors[cfg->id] == NULL) return NULL; + memset(padd0, 0, sizeof(padd0)); + + switch (cfg->id) { + case SQFS_COMP_XZ: + ret = memcmp(cfg->opt.xz.padd0, padd0, + sizeof(cfg->opt.xz.padd0)); + break; + case SQFS_COMP_LZO: + ret = memcmp(cfg->opt.lzo.padd0, padd0, + sizeof(cfg->opt.lzo.padd0)); + break; + case SQFS_COMP_ZSTD: + ret = memcmp(cfg->opt.zstd.padd0, padd0, + sizeof(cfg->opt.zstd.padd0)); + break; + case SQFS_COMP_GZIP: + ret = memcmp(cfg->opt.gzip.padd0, padd0, + sizeof(cfg->opt.gzip.padd0)); + break; + default: + ret = memcmp(cfg->opt.padd0, padd0, sizeof(cfg->opt.padd0)); + break; + } + + if (ret != 0) + return NULL; + return compressors[cfg->id](cfg); } diff --git a/lib/sqfs/data_writer/common.c b/lib/sqfs/data_writer/common.c index 059ef41..a19eb6d 100644 --- a/lib/sqfs/data_writer/common.c +++ b/lib/sqfs/data_writer/common.c @@ -157,9 +157,13 @@ int sqfs_data_writer_write_fragment_table(sqfs_data_writer_t *proc, return 0; } -void sqfs_data_writer_set_hooks(sqfs_data_writer_t *proc, void *user_ptr, - const sqfs_block_hooks_t *hooks) +int sqfs_data_writer_set_hooks(sqfs_data_writer_t *proc, void *user_ptr, + const sqfs_block_hooks_t *hooks) { + if (hooks->size != sizeof(*hooks)) + return SQFS_ERROR_UNSUPPORTED; + proc->hooks = hooks; proc->user_ptr = user_ptr; + return 0; } diff --git a/lib/sqfs/read_tree.c b/lib/sqfs/read_tree.c index 46c7ec0..3a43769 100644 --- a/lib/sqfs/read_tree.c +++ b/lib/sqfs/read_tree.c @@ -199,6 +199,9 @@ int sqfs_dir_reader_get_full_hierarchy(sqfs_dir_reader_t *rd, const char *ptr; int ret; + if (flags & ~SQFS_TREE_ALL_FLAGS) + return SQFS_ERROR_UNSUPPORTED; + ret = sqfs_dir_reader_get_root_inode(rd, &inode); if (ret) return ret; diff --git a/lib/sqfshelper/statistics.c b/lib/sqfshelper/statistics.c index 3f9f595..fb70615 100644 --- a/lib/sqfshelper/statistics.c +++ b/lib/sqfshelper/statistics.c @@ -55,6 +55,7 @@ static void notify_fragment_discard(void *user, const sqfs_block_t *block) } static const sqfs_block_hooks_t hooks = { + .size = sizeof(hooks), .post_block_write = post_block_write, .pre_fragment_store = pre_fragment_store, .notify_blocks_erased = notify_blocks_erased, diff --git a/tests/Makemodule.am b/tests/Makemodule.am index d7f8013..8a2241f 100644 --- a/tests/Makemodule.am +++ b/tests/Makemodule.am @@ -70,6 +70,9 @@ test_str_table_SOURCES = tests/str_table.c test_str_table_LDADD = libutil.la test_str_table_CPPFLAGS = $(AM_CPPFLAGS) -DTESTPATH=$(top_srcdir)/tests +test_abi_SOURCES = tests/abi.c +test_abi_LDADD = libsquashfs.la + fstree_fuzz_SOURCES = tests/fstree_fuzz.c fstree_fuzz_LDADD = libfstree.a libutil.la @@ -83,6 +86,7 @@ check_PROGRAMS += test_fstree_from_file test_fstree_init check_PROGRAMS += test_tar_ustar test_tar_pax test_tar_gnu test_tar_sparse_gnu check_PROGRAMS += test_tar_sparse_gnu1 test_tar_sparse_gnu2 check_PROGRAMS += test_tar_xattr_bsd test_tar_xattr_schily test_str_table +check_PROGRAMS += test_abi noinst_PROGRAMS += fstree_fuzz tar_fuzz @@ -92,6 +96,6 @@ TESTS += test_add_by_path test_get_path test_fstree_sort test_fstree_from_file TESTS += test_fstree_init test_tar_ustar test_tar_pax TESTS += test_tar_gnu test_tar_sparse_gnu test_tar_sparse_gnu1 TESTS += test_tar_sparse_gnu2 test_tar_xattr_bsd test_tar_xattr_schily -TESTS += test_str_table +TESTS += test_str_table test_abi EXTRA_DIST += $(top_srcdir)/tests/tar $(top_srcdir)/tests/words.txt diff --git a/tests/abi.c b/tests/abi.c new file mode 100644 index 0000000..1601e0a --- /dev/null +++ b/tests/abi.c @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-3.0-or-later */ +/* + * abi.c + * + * Copyright (C) 2019 David Oberhollenzer + */ +#include "config.h" + +#include "sqfs/compressor.h" + +#include +#include + +int main(void) +{ + sqfs_compressor_config_t cfg; + + assert(sizeof(cfg.opt.gzip) == sizeof(cfg.opt)); + assert(sizeof(cfg.opt.zstd) == sizeof(cfg.opt)); + assert(sizeof(cfg.opt.lzo) == sizeof(cfg.opt)); + assert(sizeof(cfg.opt.xz) == sizeof(cfg.opt)); + assert(sizeof(cfg.opt.padd0) == sizeof(cfg.opt)); + + return EXIT_SUCCESS; +} -- cgit v1.2.3