Libguestfs - Apr 2016 - Re: [PATCH v3 libguestfs] launch: Implement a safer getumask.

On Thu, Apr 14, 2016 at 08:04:39AM -0600, Eric Blake
wrote:
> On 04/14/2016 07:57 AM, Richard W.M. Jones wrote:
> > On Thu, Apr 14, 2016 at 07:38:23AM -0600, Eric Blake wrote:
> >>> +  /* Read the umask. */
> >>> +  if (read (fd[0], &mask, sizeof mask) != sizeof mask) {
> >>> +    perrorf (g, "read");
> >>> +    close (fd[0]);
> >>> +    return -1;
> >>
> >> Oops - this strands a child process.  You have to reap the child,
even
> >> if the read() failed.
> > 
> > Bleah that was stupid.  Try attached version - the only difference is
> > I added a waitpid call to the error path above.
> 
> But without looping on EINTR...
Thanks - I pushed it with your suggested loop added.

BTW we don't loop in waitpid/EINTR anywhere else in libguestfs, but I
guess this is something we should be doing.  Can you tell us why it's
necessary?

Rich.
> 
> +
> +  /* Read the umask. */
> +  if (read (fd[0], &mask, sizeof mask) != sizeof mask) {
> +    perrorf (g, "read");
> +    close (fd[0]);
> +    waitpid (pid, NULL, 0);
> 
> I think it's enough to make this line:
> 
>     while (waitpid (pid, &status, 0) == -1 && errno == EINTR);
> 
> +    return -1;
> 
> as you've already reported the initial error, and are merely focused on
> cleanup of the child.
> 
> +  }
> +  close (fd[0]);
> +
> + again:
> +  if (waitpid (pid, &status, 0) == -1) {
> +    if (errno == EINTR) goto again;
> +    perrorf (g, "waitpid");
> +    return -1;
> 
> Anything else, like trying to reuse the again: loop, seems like it be
> more complicated control flow.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 04/14/2016 08:11 AM, Richard W.M. Jones wrote:
> On Thu, Apr 14, 2016 at 08:04:39AM -0600, Eric Blake wrote:
>> On 04/14/2016 07:57 AM, Richard W.M. Jones wrote:
>>> On Thu, Apr 14, 2016 at 07:38:23AM -0600, Eric Blake wrote:
>>>>> +  /* Read the umask. */
>>>>> +  if (read (fd[0], &mask, sizeof mask) != sizeof mask)
{
>>>>> +    perrorf (g, "read");
>>>>> +    close (fd[0]);
>>>>> +    return -1;
>>>>
>>>> Oops - this strands a child process.  You have to reap the
child, even
>>>> if the read() failed.
>>>
>>> Bleah that was stupid.  Try attached version - the only difference
is
>>> I added a waitpid call to the error path above.
>>
>> But without looping on EINTR...
> 
> Thanks - I pushed it with your suggested loop added.
> 
> BTW we don't loop in waitpid/EINTR anywhere else in libguestfs, but I
> guess this is something we should be doing.  Can you tell us why it's
> necessary?
Depending on what else the program linking with your library has done,
waitpid() has some scenarios where it is allowed to fail with EINTR (off
the top of my head, it has to do with whether a SIGCHLD handler was
installed with SA_RESTART or not).  When you are not in a library
context, you can control things so that system calls never fail with
EINTR (the OS does the retry for you); but in a library, you can't
assume that context.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

BTW this causes valgrind to fail in a particularly strange way.  I
filed a bug upstream with a minimal reproducer.  Do you have any
ideas?

  https://bugs.kde.org/show_bug.cgi?id=361810

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 04/15/2016 07:22 AM, Richard W.M. Jones wrote:
> 
> BTW this causes valgrind to fail in a particularly strange way.  I
> filed a bug upstream with a minimal reproducer.  Do you have any
> ideas?
> 
>   https://bugs.kde.org/show_bug.cgi?id=361810
Weird - it looks like valgrind is somehow calling lseek on stdin, wiping
out the buffer that it previously read.  Since it doesn't happen when
the program is run standalone, I'm thinking this is a valgrind bug.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org