[Libguestfs] [nbdkit PATCH v2 4/8] api: Add nbdkit_str[n]dup_intern helper

[Richard W.M. Jones]

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