Re: bin/56080 (tar(1) dumps core on nfs volume)

[lukem%NetBSD.org@localhost]

Synopsis: tar(1) dumps core on nfs volume

Responsible-Changed-From-To: bin-bug-people->lukem
Responsible-Changed-By: lukem%NetBSD.org@localhost
Responsible-Changed-When: Sun, 28 May 2023 19:52:15 +0000
Responsible-Changed-Why:


State-Changed-From-To: open->analyzed
State-Changed-By: lukem%NetBSD.org@localhost
State-Changed-When: Sun, 28 May 2023 19:52:15 +0000
State-Changed-Why:

I read the thread about this issue, including RVP's proposed fix to
ensure that the struct filesystem.name_max field isn't 0 to fallback
to NAME_MAX.

I think libarchive's implementation for NetBSD could be simplified to
just allocate the backing store for sizeof(struct dirent) when using
readdir_r() on platforms where dirent.d_name has a known size (e.g.,
d_name).  Or use the existing logic modified to check at startup to use
NAME_MAX if defined and pathconf as a fallback?

(I see that macOS/Darwin and Linux/glibc both claim readdir_r() is
deprecated because NAME_MAX may be undefined, but NetBSD doesn't claim
that it's deprecated, and AFAICT, neither does POSIX.1-2017).

Looking into more detail in how libarchive uses the filesystem.name_max field:

- Relevant parts of struct filesystem:
        struct filesystem {
        // ...
        #if defined(USE_READDIR_R)
                size_t          name_max;
        #endif
        // ...
        };

- There's a lot of system-specific complexity to determine name_max
  as the length of a path name.

- Relevant parts of struct tree:
        struct tree {
        //...
        #if defined(USE_READDIR_R)
                struct dirent           *dirent;
                size_t                   dirent_allocated;
        #endif
        //...
        };

- In tree_dir_next_posix(), filesystem.name_max is used to determine
  how much space to allocate for struct tree.dirent:
        #if defined(USE_READDIR_R)
                dirent_size = offsetof(struct dirent, d_name) +
                  t->filesystem_table[t->current->filesystem_id].name_max + 1;
                if (t->dirent == NULL || t->dirent_allocated < dirent_size) {
                        free(t->dirent);
                        t->dirent = malloc(dirent_size);


Why does libarchive go to so much effort to determine how much space to
use for the backing store for readdir_r()'s struct dirent in a
system-specific manner?

Why isn't libarchive just allocating sizeof(struct dirent) for those
systems where struct dirent is known to be a fixed size (where
dirent.d_name is fixed), and avoid this complicated
per-current-filesystem statvfs (etc) logic?

It strikes me that this complex logic could potentially fail with memory
overruns if a kernel returns the wrong value for "maximum length of a
path for that filesystem", and given dirent has a fixed upper size at
compile time on most platforms (see above).