Date: Sun, 23 Oct 2022 14:18:54 -0400 From: Mark Johnston <markj@freebsd.org> To: jhb@freebsd.org, freebsd-virtualization@freebsd.org Subject: bhyve nvlist constness bug Message-ID: <Y1WFjpZ8zzVOrhTK@nuc>
next in thread | raw e-mail | index | archive | help
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");
So, if we naively changed find_config_node() to return a copy of the
nvlist at "path", set_config_value_node() would have to somehow figure
out where in the config tree to insert the updated nvlist, but it
doesn't have the path anymore.
To fix it properly could change find_config_node() to return some opaque
type which contains the source path of the node so that we can DTRT when
mutating the node. IMO it's better if the config node type is opaque to
consumers. Or, make each nvlist node store its source path by storing
it in the nvlist itself. e.g., the nvlist node describing the device
model at "pci.0.1.2" would have a "__path" key with value "pci.0.1.2",
so that set_config_value_node() can figure out where in the tree to
replace an existing copy of "pci.0.1.2". Or is there a simpler way to
fix this that I missed?
Any thoughts? I'm happy to implement the suggestions above but didn't
want to do that work without some buy-in.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Y1WFjpZ8zzVOrhTK>