Hi,
Quoth Andrea Calligaris <ac89.hk.public_AT_gmail.com>:
> As requested, I resend my old patch for fixing the crashing while archiving
> with names longer than 100 characters.
First, thank you so much to resend it and sorry for the delay, I am
a bit busy lately.
> + bsname = basename((char *)path);
> + estrlcpy(tmp_prefix, path, PATH_MAX);
> + dirname(tmp_prefix);
> + // Could still be too long to fit in the fields.
While I was doing the review this cast bugged me, and then I
followed the code and I saw that we cannot pass path directly
to basename(3) because it modifies the parameter, and the following
print would fail:
eprintf("filename too long: %s\n", path);
and in the code calling archive().
static void
c(int dirfd, const char *name, struct stat *st, void *data, struct recursor *r)
{
archive(r->path);
if (vflag)
puts(r->path);
if (S_ISDIR(st->st_mode))
recurse(dirfd, name, NULL, r);
}
I propose these changes to your patch (not tested yet):
diff --git a/tar.c b/tar.c
index e4701dc..c6417c9 100644
--- a/tar.c
+++ b/tar.c
_AT_@ -179,7 +179,6 @@ putoctal(char *dst, unsigned num, int size)
static int
archive(const char *path)
{
- char b[BLKSIZ];
struct group *gr;
struct header *h;
struct passwd *pw;
_AT_@ -187,6 +186,7 @@ archive(const char *path)
size_t chksum, i;
ssize_t l, r;
int fd = -1;
+ char b[BLKSIZ], tmp[PATH_MAX], *base, *dir;
if (lstat(path, &st) < 0) {
weprintf("lstat %s:", path);
_AT_@ -202,25 +202,25 @@ archive(const char *path)
h = (struct header *)b;
memset(b, 0, sizeof(b));
- if (strlen(path) > 255) {
- const char *reason = "path exceeds 255 character limit";
- eprintf("malformed tar archive: %s\n", reason);
- } else if (strlen(path) >= 100) {
- size_t prefix_len = 155;
- const char *last_slash = strrchr(path, '/');
-
- if (last_slash && last_slash < path + prefix_len) {
- prefix_len = last_slash - path + 1;
+ if (strlen(path) >= sizeof(h->name)) {
+ /*
+ * Cover case where path name is too long (in which case we
+ * need to split it to prefix and name).
+ */
+ estrlcpy(tmp, path, PATH_MAX);
+ base = basename(tmp);
+ dir = dirname(tmp);
+
+ /* Could still be too long to fit in the fields */
+ if (strlen(base) >= sizeof(h->name) ||
+ strlen(dir) >= sizeof(h->prefix)) {
+ eprintf("filename too long: %s\n", path);
+ } else {
+ strlcpy(h->name, base, sizeof(h->name));
+ strlcpy(h->prefix, dir, sizeof(h->prefix));
}
-
- /* strlcpy is fine here - for path ONLY -,
- * since we're splitting the path.
- * It's not an issue if the prefix can't hold
- * the full path — name will take the rest. */
- strlcpy(h->prefix, path, prefix_len);
- estrlcpy(h->name, path + prefix_len, sizeof(h->name));
} else {
- estrlcpy(h->name, path, sizeof(h->name));
+ strlcpy(h->name, path, sizeof(h->name));
}
putoctal(h->mode, (unsigned)st.st_mode & 0777, sizeof(h->mode));
There are some small cosmetic changes, but the biggest change is
to copy the parameter to the temporary buffer first, and then use
basename and dirname in that temporary buffer that would not affect
the original buffer. As we are explicitely checking the sizes we can
avoid the estrlcpy calls and use strlcpy (we could use memcpy directly
too, but it is worthless).
What do you think?
Regards,
Received on Tue Mar 18 2025 - 07:06:21 CET