lib: Move validation out of lcfs_node_add_child()

containers / composefs

The reliability of disk images, the flexibility of files

Other

461 stars 36 forks source link

lib: Move validation out of lcfs_node_add_child() #341

Closed cgwalters closed 2 months ago

cgwalters commented 2 months ago

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.

cgwalters commented 2 months ago

Ah yeah of course since this is C a ~5 line unit test has a double-unref, thankfully caught by ASAN.

cgwalters commented 2 months ago

Yeah, agree. Here's one thing I've been thinking about as followups to all this:

Add lcfs_node_new_symlink() lcfs_node_new_inline_file() lcfs_node_new_dev() etc that take their required properties as constructors (very much like how things are modeled on the Rust side)
These APIs also set a node->strict = true property that enforces more validation
When `lcfs_node_add_child() is invoked on a node with strict = true, any further mutation APIs return an error?

Actually we could just add lcfs_node_new_dev() now and not ship lcfs_node_set_rdev64 I guess.

guess that would more or less be an ABI break and need an soname bump, right?

Right, I don't want to scope that into this at this time. It would be painful.

allisonkarlitskaya commented 2 months ago

Add lcfs_node_new_symlink() lcfs_node_new_inline_file() lcfs_node_new_dev() etc that take their required properties as constructors (very much like how things are modeled on the Rust side)

This sounds nice.

Is your plan to land this PR, add those APIs, then back out the rdev64 setter, then deprecate the existing _new() API as a way to more or less remove the ability to create ->strict == false nodes? And then port ostree over? Because it's sort of at cross purposes to this PR to move validation to 'later' because of node mutability if we intend to head in a direction where the nodes are no longer mutable...

cgwalters commented 2 months ago

My hope was to land this PR basically as is alongside https://github.com/containers/composefs/pull/342 and get a new release out and leave other things to followup.

cgwalters commented 2 months ago

Adding a new API is going to need more tests, and we'd need to rework the fuzzing entrypoint to support both the old and new API, which is quite doable but also not trivial, so I'd rather get fixes out for what we have now as a release.