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