On Wed, Aug 26, 2020 at 09:16:48PM -0500, Eric Blake wrote:
Implementing .default_export with its 'const char *' return
is tricky
in the sh plugin: we must return dynamic memory, but must avoid a
use-after-free. And we don't want to change the return type of
.default_export to 'char *', because that would make our choice of
malloc()/free() part of the API, preventing either nbdkit or a plugin
from experimenting with an alternate allocator implementation.
Elsewhere, we have done things like nbdkit_extents_new(), so that even
though the client is directing allocation, the actual call to malloc()
is done by nbdkit proper, so that nbdkit later calling free() does not
tie the plugin's hands, and nbdkit could change its underlying
allocation without breaking ABI. (Note that nbdkit_realpath() and
nbdkit_absolute_path() require the caller to use free(), but as they
are used during .config, it's less of a burden for plugins to take
care of that in .unload.)
We could document that the burden is on the plugin to avoid the memory
leak, by making sh track all strings it returns, then finally clean
them up during .unload. But this is still a memory leak in the case
of .default_export, which can be called multiple times when there are
multiple clients, and presumably with different values per client
call; better would be freeing the memory at .close when each client
goes away. Except that .default_export is called before .open, and
therefore before we have access to the handle struct that will
eventually make it to .close.
So, the solution is to let nbdkit track an alternate string on our
behalf, where nbdkit then cleans it up as the client goes away.
There are several places in the existing code base that can also take
advantage of this copying functionality, including in filters that
want to pass altered strings to .config while still obeying lifetime
rules.
We should probably just push this one straight away, with
one small change:
diff --git a/server/public.c b/server/public.c
index d10d466e..512e9caa 100644
--- a/server/public.c
+++ b/server/public.c
This file is probably going to need:
#include "strndup.h"
somewhere near the top because unbelievably Win32 lacks this function.
Rich.
--
Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
Read my programming and virtualization blog:
http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html