Re: bhyve nvlist constness bug

On Sun, Oct 23, 2022 at 02:18:54PM -0400, Mark Johnston wrote:
> I'm going through compiler warnings in the bhyve code with the aim of
> bumping WARNS, since the current setting hides bugs.  There's one in the
> config code that looks a bit tricky to resolve and I was hoping for some
> guidance on the right way to do that.
> 
> The basic problem is _lookup_config_node() may return an nvlist via
> nvlist_get_nvlist(), but nvlist_get_nvlist() returns a const nvlist_t
> *, so _lookup_config_node() is discarding the const qualifier.  And
> indeed, some callers will modify the returned node.  The use of
> nvlist_move_nvlist() in _lookup_config_node() has a similar problem:
> nvlist_move_* "consumes" the passed value and is not supposed to be
> mutated after.
> 
> I'm pretty sure this is actually a non-issue with our nvlist
> implementation; when adding an nvlist value to an nvlist, it doesn't
> store anything except for the pointer itself, so you can mutate it
> safely.  Note, this is not true for all value types, specifically
> arrays.  However, there are multiple nvlist implementations out there,
> and ours might change such that bhyve's config code becomes incorrect.
> 
> The bug seems annoying to fix because consumers can do this:
> 
>     nvlist_t *nvl = find_config_node(path);
>     set_config_value_node(nvl, "foo", "bar");

Probalby not entirely relevant, but FWIW, there are comments in the
OpenZFS nvpair implementation that basically say you can't pass a const
nvlist (or string or array) to nvlist_add_*.

	/*
	 * This discards the const from nvp, so all callers for these
	 * types must not accept const nvpairs.
	 */

-- Brooks