[Libguestfs] [PATCH nbdkit 4/4] Add floppy plugin.

[Eric Blake]

On 10/30/18 11:42 AM, Richard W.M. Jones wrote:

> 
>>> +  errno = 0;
>>> +  while ((d = readdir (DIR)) != NULL) {
>>> +    if (strcmp (d->d_name, ".") == 0 ||
>>> +        strcmp (d->d_name, "..") == 0)
>>> +      continue;

strcmp() leaves errno alone (well, POSIX doesn't guarantee that, but no 
sane implementation of strcmp() would change errno during a string compare)

>>> +
>>> +    if (lstat (d->d_name, &statbuf) == -1) {
>>> +      nbdkit_error ("stat: %s/%s: %m", dir, d->d_name);
>>> +      goto error2;
>>> +    }

However, errno is undefined if lstat() succeeds. If it fails, you don't 
call readdir() again; but if it succeeds, you cannot rely on libc having 
left errno == 0.

>>> +
>>> +    /* Directory. */
>>> +    if (S_ISDIR (statbuf.st_mode)) {
>>> +      if (visit_subdirectory (dir, d->d_name, &statbuf, di, floppy) == -1)
>>> +        goto error2;
>>> +    }

And then you have to audit whether your visit_subdirectory() code left 
errno unchanged...

>>> +    /* Regular file. */
>>> +    else if (S_ISREG (statbuf.st_mode)) {
>>> +      if (visit_file (dir, d->d_name, &statbuf, di, floppy) == -1)
>>> +        goto error2;
>>> +    }

...and visit_file().  Looking at just visit_file():

> +static int
> +visit_file (const char *dir, const char *name,
> +            const struct stat *statbuf, size_t di,
> +            struct virtual_floppy *floppy)
> +{
> +  void *np;
> +  char *host_path;
> +  size_t fi, i;
> +
> +  if (asprintf (&host_path, "%s/%s", dir, name) == -1) {
> +    nbdkit_error ("asprintf: %m");
> +    return -1;
> +  }

asprintf() may have modified errno...

> +  /* Add to global list of files. */
> +  fi = floppy->nr_files;
> +  np = realloc (floppy->files, sizeof (struct file) * (fi+1));

Likewise a successful realloc()...

> +  if (np == NULL) {
> +    nbdkit_error ("realloc: %m");
> +    free (host_path);
> +    return -1;
> +  }
> +  floppy->files = np;
> +  floppy->nr_files++;
> +  floppy->files[fi].name = strdup (name);

even a successful strdup(). Etc.

In short, POSIX guarantees that errno is set on failure, but leaves 
errno undefined on success after many library functions (there are a 
few, like free(), where I have been working to get POSIX to explicitly 
state that errno is left unchanged - but those are mainly functions you 
use on cleanup paths, where it is easier to program if you can guarantee 
that your cleanup doesn't corrupt the original error).

>> Need to reset errno back to 0 before looping back to the next
>> readdir() call.
> 
> Hmm, really?  If so, this is a class of bugs we have made throughout
> libguestfs code.

Yes, this is a common bug, and it looks like libguestfs needs an audit 
fixing it.  The only SAFE way to call readdir() is to immediately set 
errno = 0 just beforehand, on every iteration (not just the first).

>  Why would errno be set in the loop?

Because you called into libc.

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