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

[Richard W.M. Jones]

On Tue, Oct 30, 2018 at 11:54:39AM -0500, Eric Blake wrote:
> 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).

Fair enough.  Looks like a long day of auditing ahead ...

Rich.

> > 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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top