On 9/1/20 9:24 AM, Richard W.M. Jones wrote:
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.
Sounds good. I'm also finding that nbdkit_printf_intern(const char
*fmt, ...) and nbdkit_vprintf_intern(const char *, va_list) are going to
be useful, so I'll add those, get this in, then post a v3 of the rest of
the series with further doc cleanups.
Another use for this new function (capturing the essence of an IRC
discussion): we have several language plugins that mention that things
like .config_help is not overrideable. The full set of struct plugin
strings that would be nicer as functions:
.longname
.description
.config_help
.magic_config_key
(Maybe .name or .version as well, but I see less reason to want to make
those dynamic)
Having a callback function that returns a const char * would let
language bindings return a description or similar as set by the script;
it would also allow us to alter the magic config key in a stateful
manner according to what previous config keys have been parsed. So, for
example, we could allow:
nbdkit python file.py myfile
as shorthand for
nbdkit python script=file.py file=myfile
by having file.py provide an overload for a new .get_magic_config_key()
function.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
_______________________________________________
Libguestfs mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libguestfs
