lib: Move validation out of lcfs_node_add_child()

Unfortunately at least ostree does:

node = lcfs_node_new()
lcfs_node_add_child(parent, node);
lcfs_node_set_mode(node, ...)
So we basically have a completely uninitialized node
as part of the tree.

This failed our basic "validate the mode" logic.

We can't do any validation at all in `lcfs_node_add_child()` so
move it to the first time we walk the tree as part of
`lcfs_node_write_to()`.

Also as part of fixing this: Today we don't really have coverage
of the C library as it may be used by external consumers.
Add a unit test to verify this, and we can build on this more.

Signed-off-by: Colin Walters <walters@verbum.org>

.clang-format-include

@@ -1,2 +1,3 @@
 libcomposefs/**/*
 tools/**/*
+tests/**/*

libcomposefs/lcfs-internal.h

@@ -224,7 +224,7 @@ int lcfs_node_rename_xattr(struct lcfs_node_s *node, size_t index,
 			   const char *new_name);
 
 int lcfs_validate_mode(mode_t mode);
-int lcfs_node_last_ditch_validation(struct lcfs_node_s *node);
+int lcfs_node_validate(struct lcfs_node_s *node);
 
 /* lcfs-writer-erofs.c */
 

libcomposefs/lcfs-writer-erofs.c

@@ -1233,6 +1233,12 @@ static int rewrite_tree_node_for_erofs(struct lcfs_ctx_s *ctx,
 {
 	int ret;
 
+	// This is one of the first walks of the tree we do, so validate
+	// everything here.
+	ret = lcfs_node_validate(node);
+	if (ret < 0)
+		return ret;
+
 	ret = add_overlayfs_xattrs(ctx, node);
 	if (ret < 0)
 		return ret;

libcomposefs/lcfs-writer.c

@@ -390,12 +390,6 @@ int lcfs_write_to(struct lcfs_node_s *root, struct lcfs_write_options_s *options
 	struct lcfs_ctx_s *ctx;
 	int res;
 
-	// Verify the root; because lcfs_node_add_child also verifies children,
-	// we should have sanity checked all nodes.
-	if (lcfs_node_last_ditch_validation(root) < 0) {
-		return -1;
-	}
-
 	/* Check for unknown flags */
 	if ((options->flags & ~LCFS_FLAGS_MASK) != 0) {
 		errno = EINVAL;
@@ -1174,7 +1168,7 @@ struct lcfs_node_s *lcfs_node_get_hardlink_target(struct lcfs_node_s *node)
 // invalid states, but don't return errors; this will try
 // to perform validation when the node is passed to a function
 // that does return an error.
-int lcfs_node_last_ditch_validation(struct lcfs_node_s *node)
+int lcfs_node_validate(struct lcfs_node_s *node)
 {
 	// Hardlinks should have mode 0
 	if (node->link_to == NULL) {
@@ -1190,10 +1184,6 @@ int lcfs_node_add_child(struct lcfs_node_s *parent, struct lcfs_node_s *child,
 	struct lcfs_node_s **new_children;
 	size_t new_capacity;
 
-	if (lcfs_node_last_ditch_validation(child) < 0) {
-		return -1;
-	}
-
 	if ((parent->inode.st_mode & S_IFMT) != S_IFDIR) {
 		errno = ENOTDIR;
 		return -1;

tests/meson.build

@@ -63,6 +63,8 @@ foreach case : test_assets_should_fail
 endforeach
 test('check-should-fail', find_program('test-should-fail.sh'), args : should_fail_args)
 
+test('test-lcfs', executable('test-lcfs', 'test-lcfs.c', include_directories: '../libcomposefs', link_with: libcomposefs))
+
 # support running the tests under valgrind using 'meson test -C build --setup=valgrind'
 valgrind = find_program('valgrind', required : false)
 if valgrind.found()

tests/test-lcfs.c

@@ -0,0 +1,32 @@
+#define _GNU_SOURCE
+
+#include "lcfs-writer.h"
+#include <assert.h>
+
+static inline void lcfs_node_unrefp(struct lcfs_node_s **nodep)
+{
+	if (*nodep != NULL) {
+		lcfs_node_unref(*nodep);
+		*nodep = NULL;
+	}
+}
+#define cleanup_node __attribute__((cleanup(lcfs_node_unrefp)))
+
+static void test_add_uninitialized_child(void)
+{
+	cleanup_node struct lcfs_node_s *node = lcfs_node_new();
+	lcfs_node_set_mode(node, S_IFDIR | 0755);
+	// libostree today does this pattern of creating an empty (uninitialized)
+	// child and passing it to lcfs_node_add_child first. Verify this
+	// continues to work for the forseeable future.
+	cleanup_node struct lcfs_node_s *child = lcfs_node_new();
+	int r = lcfs_node_add_child(node, child, "somechild");
+	assert(r == 0);
+	// Adding child took ownership
+	child = NULL;
+}
+
+int main(int argc, char **argv)
+{
+	test_add_uninitialized_child();
+}