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