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

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