Control: tags -1 + fixed-upstream Control: forwarded -1 https://github.com/sebastinas/yafc/issues/48
Hi On 2015-04-08 20:58:18, Celelibi wrote: > 2015-04-07 13:12 UTC+02:00, Sebastian Ramacher <[email protected]>: > > On 2015-04-06 22:33:28, Celelibi wrote: > >> Package: yafc > >> Version: 1.3.5-2 > >> Severity: important > >> Tags: upstream patch > >> > >> Dear maintainer, > >> > >> There is a bug in the path shortener that makes yafc crash on some > >> specific input. > >> > >> When it tries to replace a short directory name with "..." it tries to > >> push the remaining of the string farther away, without reallocating a > >> larger buffer. > >> > >> For instance: > >> /w/some/very/long/pathname/whichis/really/too/long/andshouldbeshortened.html > >> is likely to cause the bug. > >> > >> The actual bug is in src/libmhe/shortpath.c when using strpush. > >> > >> I propose here a patch that simplifies the path shortening code so that > >> it has no loop, no complex condition, no temporary memory allocation, no > >> complex string handling. > >> > >> In short: half the size, twice the fun. :) > >> > >> The patch hasn't been extensively tested, but I guess it should be > >> bug-free and behave like the original code. > > > > The behavior of the new code differs from the old code. Previously, > > "averylongdirectoryname/filename.extension" was shortened to > > "...me/filename.extension". The new code returns > > "averylongdirectoryname/...t". > > True, this patch was also buggy in some cases. :) > > > Also, "/usr/bin/averylongfilenamethatistoolongindeed.tar.gz = > > ...stoolongindeed.tar.gz" was shortened to "...stoolongindeed.tar.gz" and > > now > > it's "/...stoolongindeed.tar.gz". > > That's true too. > I'll fix that, but notice that this is inconsistent with the comment > above the current code that say: > /usr/bin/averylongfilenamethatistoolongindeed.tar.gz => > /...oolongindeed.tar.gz > > > Since I could decide exactly what behavior was expected, it's not 1, > not 2, but 3 patches to choose from that I send there. :) > > - The first one (0001-shortpath-exact.patch) just try to mimic the > behavior of the original code. The only difference in behavior that I > could observe is that it doesn't generate some silly "....../file.txt" > that the current code does sometime when a previous "..." collide with > the new one. > > - The second one (0001-shortpath-short-dir.patch) improves a bit the > first one by handling better short directory names. For instance, > "longdirectory/a/file.txt" doesn't get shortened to > "...ngdir/.../thefile.txt" but to "...longdir/a/thefile.txt". Same > goes for "a/some/dirs/goes/here/file.txt" which never gets shortened > to ".../.../here/file.txt", but to "a/.../goes/here/file.txt". > > - The third one (0001-shortpath-simple.patch) completely remove that > double "..." that can appear (like in the previous examples). This > help a lot with code clarity and simplicity. Thanks for the patches. If applied the third one with small modifications (see below). This should also fix https://github.com/sebastinas/yafc/issues/48. > From 2ed5951e3b8b0ba465fe2d343e19bd94548e6233 Mon Sep 17 00:00:00 2001 > From: Celelibi <[email protected]> > Date: Wed, 8 Apr 2015 19:58:24 +0200 > Subject: [PATCH] shortpath: simpler code > > Code shorter, simple, without loops, handles short directories better. > Doesn't insert multiples "...". > > Signed-off-by: Celelibi <[email protected]> > --- > src/libmhe/shortpath.c | 61 > +++++++++++++++++--------------------------------- > 1 file changed, 21 insertions(+), 40 deletions(-) > > diff --git a/src/libmhe/shortpath.c b/src/libmhe/shortpath.c > index 12a46d7..1bce60a 100644 > --- a/src/libmhe/shortpath.c > +++ b/src/libmhe/shortpath.c > @@ -22,54 +22,35 @@ > * /usr/bin/averylongfilenamethatistoolongindeed.tar.gz => > /...oolongindeed.tar.gz > */ > > -static char* chop(const char *str, size_t maxlen) > +static char *_shortpath(char *path, size_t maxlen) > { > - const size_t len = strlen(str); > + size_t len = strlen(path); > + char *result = NULL; > + const char *copy_from = NULL; > + const char *first_slash = NULL; > + size_t start_len = 0; > + > if (len <= maxlen) > - return xstrdup(str); > + return xstrdup(path); > > - char* result = malloc(maxlen + 1); > - strncpy(result, "...", 3); > - strncpy(result + 3, str+(len-maxlen+3), maxlen - 3); > - result[maxlen] = '\0'; > - return result; > -} > + result = xmalloc(maxlen + 1); > + result[0] = '\0'; xmalloc calls memset(..., 0, ...) on the the allocated memory block. I've removed the explicit assignment. > > -static char* dirtodots(const char *path, size_t maxlen) > -{ > - const char* first_slash = strchr(path, '/'); > - if (!first_slash) > - return chop(path, maxlen); > + first_slash = strchr(path, '/'); > + if (first_slash) > + start_len = first_slash - path + 1; > > - const char* end_slash = NULL; > - if (strncmp(first_slash+1, "...", 3) == 0) > - end_slash = strchr(first_slash+5, '/'); > + if (maxlen - start_len - 3 > 0) > + copy_from = strchr(path + len - (maxlen - start_len - 3), '/'); This causes invalid reads if 0 <= maxlen - start_len < 3. Changing the condition to maxlen - start_len > 3 fixes that. > + > + if (copy_from) > + strncpy(result, path, start_len); > else > - end_slash = strchr(first_slash+1, '/'); > - > - if (!end_slash) > - return chop(path, maxlen); > - > - size_t first = first_slash - path, > - end = end_slash - path; > - char* result = xstrdup(path); > - if (end - first < 4) /* /fu/ */ > - strpush(result + first +1, 4 - (end - first)); > - else /* /foobar/ */ > - strcpy(result + first + 4, end_slash); > - strncpy(result + first + 1, "...", 3); > - return result; > -} > + copy_from = path + len - (maxlen - 3); > > -static char *_shortpath(char *path, size_t maxlen) > -{ > - const size_t len = strlen(path); > - if (len <= maxlen) > - return xstrdup(path); > + strcat(result, "..."); > + strcat(result, copy_from); strlcat is available, so I've changed both calls to strlcat(..., maxlen + 1). Cheers -- Sebastian Ramacher
signature.asc
Description: Digital signature
