Libguestfs - Jun 2020 - Re: [PATCH nbdkit 3/5] vddk: Miscellaneous improvements to reexec code.

On 6/2/20 7:27 AM, Richard W.M. Jones wrote:
> Use an extensible buffer (a vector<char>) when reading
> /proc/self/cmdline.
> 
> Tidy up some error messages.
> ---
>   plugins/vddk/reexec.c | 57 ++++++++++++++++++++++++++-----------------
>   1 file changed, 35 insertions(+), 22 deletions(-)
> 
> @@ -80,42 +95,40 @@ perform_reexec (const char *env, const char *prepend)
>      * until we get a short read.  This assumes nbdkit did not alter its
>      * original argv[].
>      */
> -  fd = open ("/proc/self/cmdline", O_RDONLY);
> +  fd = open (cmdline_file, O_RDONLY|O_CLOEXEC);
>     if (fd == -1) {
> -    nbdkit_debug ("failure to parse original argv: %m");
> +    nbdkit_debug ("open: %s: %m", cmdline_file);
>       return;
>     }
>   
> -  do {
> -    char *p = realloc (buf, buflen * 2);
> +  for (;;) {
>       ssize_t r;
>   
> -    if (!p) {
> -      nbdkit_debug ("failure to parse original argv: %m");
> +    if (buffer_reserve (&buf, 512) == -1) {
> +      nbdkit_debug ("realloc: %m");
>         return;
>       }
Pre-existing bug, which you did not fix here.  If we failed here, we are 
leaking fd.  You slightly improved the situation by marking the leaked 
fd O_CLOEXEC, but that really doesn't matter if we properly fix the code 
to close(fd) before any early return, at which point the lifetime of fd 
is only during single-threaded execution and O_CLOEXEC doesn't matter.

Rest of the patch looks fine.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Jun 02, 2020 at 09:22:49AM -0500, Eric Blake
wrote:
> On 6/2/20 7:27 AM, Richard W.M. Jones wrote:
> >Use an extensible buffer (a vector<char>) when reading
> >/proc/self/cmdline.
> >
> >Tidy up some error messages.
> >---
> >  plugins/vddk/reexec.c | 57
++++++++++++++++++++++++++-----------------
> >  1 file changed, 35 insertions(+), 22 deletions(-)
> >
> 
> >@@ -80,42 +95,40 @@ perform_reexec (const char *env, const char
*prepend)
> >     * until we get a short read.  This assumes nbdkit did not alter
its
> >     * original argv[].
> >     */
> >-  fd = open ("/proc/self/cmdline", O_RDONLY);
> >+  fd = open (cmdline_file, O_RDONLY|O_CLOEXEC);
> >    if (fd == -1) {
> >-    nbdkit_debug ("failure to parse original argv: %m");
> >+    nbdkit_debug ("open: %s: %m", cmdline_file);
> >      return;
> >    }
> >-  do {
> >-    char *p = realloc (buf, buflen * 2);
> >+  for (;;) {
> >      ssize_t r;
> >-    if (!p) {
> >-      nbdkit_debug ("failure to parse original argv: %m");
> >+    if (buffer_reserve (&buf, 512) == -1) {
> >+      nbdkit_debug ("realloc: %m");
> >        return;
> >      }
> 
> Pre-existing bug, which you did not fix here.  If we failed here, we
> are leaking fd.  You slightly improved the situation by marking the
> leaked fd O_CLOEXEC, but that really doesn't matter if we properly
> fix the code to close(fd) before any early return, at which point
> the lifetime of fd is only during single-threaded execution and
> O_CLOEXEC doesn't matter.
I was wondering if we should define conditions where we want reexec to
be skipped (probably if /proc/self/... files don't exist, since it's
likely that it isn't a "sufficiently Linux-like" platform).  We
would
hard fail for all the other errors such as buffer_reserve failing
above.  What do you think?

Note in the next patch I actually did do some hard failing for
operations that shouldn't fail and are hard to recover from.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 6/2/20 9:33 AM, Richard W.M. Jones wrote:
> On Tue, Jun 02, 2020 at 09:22:49AM -0500, Eric Blake wrote:
>> On 6/2/20 7:27 AM, Richard W.M. Jones wrote:
>>> Use an extensible buffer (a vector<char>) when reading
>>> /proc/self/cmdline.
>>>
>>> Tidy up some error messages.
>>> ---
>>>   plugins/vddk/reexec.c | 57
++++++++++++++++++++++++++-----------------
>>>   1 file changed, 35 insertions(+), 22 deletions(-)
>>>
>> Pre-existing bug, which you did not fix here.  If we failed here, we
>> are leaking fd.  You slightly improved the situation by marking the
>> leaked fd O_CLOEXEC, but that really doesn't matter if we properly
>> fix the code to close(fd) before any early return, at which point
>> the lifetime of fd is only during single-threaded execution and
>> O_CLOEXEC doesn't matter.
> 
> I was wondering if we should define conditions where we want reexec to
> be skipped (probably if /proc/self/... files don't exist, since
it's
> likely that it isn't a "sufficiently Linux-like" platform). 
We would
> hard fail for all the other errors such as buffer_reserve failing
> above.  What do you think?
Hard fail for things like malloc failure make sense (if we fail to 
re-exec but continue on in spite of a malloc failure, we'll probably die 
real soon at our next malloc, at which point dying asap is saner)

Although vddk already assumes a Linux system, you are correct that 
/proc/self might not be properly mounted, which is one case where soft 
fail (returning without re-exec, at which point the caller probably 
exits but with a nicer error message about being unable to locate the 
vddk .so libraries than what we can give here).  So even if we don't fix 
the fd leak, it's unlikely that the overall nbdkit process is going to 
be long-running where the open fd remains open.  I guess it's mostly a 
quality-of-error situation, when re-exec fails.
> 
> Note in the next patch I actually did do some hard failing for
> operations that shouldn't fail and are hard to recover from.
Yes, and those made sense, so adding more here is fine too.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org