10:53 a.m.
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