On Tuesday, September 13, 2016 4:08:30 PM CEST Matthew White wrote: > On Tue, 13 Sep 2016 09:29:27 +0200 > > Tim Ruehsen <[email protected]> wrote: > > On Tuesday, September 13, 2016 5:13:10 AM CEST Matthew White wrote: > > > On Mon, 12 Sep 2016 21:20:54 +0200 > > > > > > Tim Rühsen <[email protected]> wrote: > > > > On Montag, 12. September 2016 20:18:30 CEST Eli Zaretskii wrote: > > > > > > From: Tim Ruehsen <[email protected]> > > > > > > Date: Mon, 12 Sep 2016 13:00:32 +0200 > > > > > > > > > > > > > + char *basename = name; > > > > > > > + > > > > > > > + while ((name = strstr (basename, "/"))) > > > > > > > + basename = name + 1; > > > > > > > > > > > > Could you use strrchr() ? something like > > > > > > > > > > > > char *basename = strrchr (name, '/'); > > > > > > > > > > > > if (basename) > > > > > > > > > > > > basename += 1; > > > > > > > > > > > > else > > > > > > > > > > > > basename = name; > > > > > > > > > > I think we want to use ISSEP, no? Otherwise Windows file names with > > > > > backslashes will misfire. > > > > > > > > Good point. What about device names ? > > > > > > > > So maybe base_name() from Gnulib module 'dirname' is the right choice > > > > !? > > > > See https://www.gnu.org/software/gnulib/manual/html_node/basename.html > > > > > > What if Gnulib's base_name() returns "./<basename>"? > > > > > > libmetalink's metalink_check_safe_path() rejects relative paths: > > > https://tools.ietf.org/html/rfc5854#section-4.1.2.1 > > > > > > Also, basename is used to point to an existing memory location, > > > base_name() > > > instead allocates new space. This is not a biggy, but we should keep it > > > in > > > mind to amend properly. > > > > > > lib/basename.c (base_name) > > > -------------------------- > > > > > > /* On systems with drive letters, "a/b:c" must return "./b:c" rather > > > > > > than "b:c" to avoid confusion with a drive letter. On systems > > > with pure POSIX semantics, this is not an issue. */ > > > > > > -------------------------- > > > > > > Suggestions? > > > > ISSEP is "homebrewed" and incomplete but doesn't need a memory allocation. > > base_name() is "complete" (the macros check more than just WINDOWS) and we > > automatically get improvements from upstream - but it calls malloc(). > > > > There is also last_component() which returns a pointer to the basename > > within your filename. This is basically what you do. > > > > Anyways, this last component (basename) may still hold a device prefix - > > you have to check that with either HAS_DEVICE() (only defined in certain > > environments, needs to guarded by #ifdef) or by FILE_SYSTEM_PREFIX_LEN() > > which gives 2 if your basename has a leading device prefix. And you > > should do this check in a loop to catch names like 'C:D:xxx'. If you > > don't do, we likely get a CVE assigned ;-) > > Thanks Tim! Yours is a super smart solution! > > I rewrote src/metalink.c (get_metalink_basename) to skip prefix drive > letters on the basename (the declaration of last_component() is in > src/metalink.h): > > #include "dosname.h" > > char * > get_metalink_basename (char *name) > { > char *basename; > > if (!name) > return NULL; > > basename = last_component (name); > > while (FILE_SYSTEM_PREFIX_LEN (basename)) > basename += 2; > > return metalink_check_safe_path (basename) ? basename : NULL; > } > > [make syntax-check is ok, make check-valgrind is ok, contrib/check-hard is > ok] > > WDYT?
Looks great ! To be absolutely future ready, don't assume FILE_SYSTEM_PREFIX_LEN to return 0 or 2 (even if the current code is like that). while ((n = FILE_SYSTEM_PREFIX_LEN (basename))) basename += n; and you are on the safe side. Regards, Tim
signature.asc
Description: This is a digitally signed message part.
