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 <david.oberhollenzer@sigma-star.at>

9 files changed, 104 insertions, 5 deletions

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
@@ -59,6 +59,18 @@
  */
 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.
 	 *
 	 * If this is not NULL, it gets called before a block is written to
@@ -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 <goliath@infraroot.at>
+ */
+#include "config.h"
+
+#include "sqfs/compressor.h"
+
+#include <assert.h>
+#include <stdlib.h>
+
+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;
+}