On 02/04/2012 06:38 PM, Eric Blake wrote: > On 02/04/2012 10:59 AM, Eric Blake wrote: >> On 02/04/2012 09:56 AM, Eric Blake wrote: >>> On Cygwin, and other platforms where // is detected as distinct >>> from / at configure time, the canonicalize routines were incorrectly >>> treating all instances of multiple leading slashes as //. >>> See also coreutils bug http://debbugs.gnu.org/10472 >>> >>> * lib/canonicalize.c (canonicalize_filename_mode): Don't convert >>> /// to //, since only // is special. >>> > >> >> which meant this was reading uninitialized memory, and depending on what >> was in the heap, might canonicalize "///" to "/" or "//". I'm pushing >> this additional fix to both files: >> >> diff --git i/lib/canonicalize-lgpl.c w/lib/canonicalize-lgpl.c >> index a61bef9..08e76fe 100644 >> --- i/lib/canonicalize-lgpl.c >> +++ w/lib/canonicalize-lgpl.c >> @@ -158,6 +158,7 @@ __realpath (const char *name, char *resolved) >> dest = rpath + 1; >> if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] != >> '/') >> *dest++ = '/'; >> + *dest = '\0'; >> } > > Still not right. If you have a symlink at //some/path whose contents is > /, then that would canonicalize to '//' without triggering any valgrind > complaints, because I missed the code that resets rpath on encountering > absolute symlink contents. Meanwhile, pre-assigning *dest is a > pessimization on platforms where // and / are identical. I'm pushing > this instead. > > From d1f3998942236194f1894c45804ec947d07ed134 Mon Sep 17 00:00:00 2001 > From: Eric Blake <ebl...@redhat.com> > Date: Sat, 4 Feb 2012 11:11:40 -0700 > Subject: [PATCH] canonicalize: avoid uninitialized memory use > > When DOUBLE_SLASH_IS_DISTINCT_ROOT is non-zero, then we were > reading the contents of rpath[1] even when we had never written > anything there, which meant that "///" would usually canonicalize > to "/" but sometimes to "//" if a '/' was leftover in the heap. > This condition could also occur via 'ln -s / //some/path' and > canonicalizing //some/path, where we rewind rpath but do not > clear out the previous round. Platforms where "//" and "/" are > equivalent do not suffer from this read-beyond-written bounds. > > * lib/canonicalize-lgpl.c (__realpath): Avoid possibility of > random '/' left in dest. > * lib/canonicalize.c (canonicalize_filename_mode): Likewise. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > ChangeLog | 7 +++++++ > lib/canonicalize-lgpl.c | 17 ++++++++++++----- > lib/canonicalize.c | 17 ++++++++++++----- > 3 files changed, 31 insertions(+), 10 deletions(-) > > diff --git a/ChangeLog b/ChangeLog > index 8f08543..aeea7c8 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,10 @@ > +2012-02-04 Eric Blake <ebl...@redhat.com> > + > + canonicalize: avoid uninitialized memory use > + * lib/canonicalize-lgpl.c (__realpath): Avoid possibility of > + random '/' left in dest. > + * lib/canonicalize.c (canonicalize_filename_mode): Likewise. > + > 2012-02-04 Bruno Haible <br...@clisp.org> > > spawn-pipe tests: Fix a NULL program name in a diagnostic. > diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c > index a61bef9..7aa2d92 100644 > --- a/lib/canonicalize-lgpl.c > +++ b/lib/canonicalize-lgpl.c > @@ -156,8 +156,12 @@ __realpath (const char *name, char *resolved) > { > rpath[0] = '/'; > dest = rpath + 1; > - if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] != > '/') > - *dest++ = '/'; > + if (DOUBLE_SLASH_IS_DISTINCT_ROOT) > + { > + if (name[1] == '/' && name[2] != '/') > + *dest++ = '/'; > + *dest = '\0'; > + } > } > > for (start = end = name; *start; start = end) > @@ -298,9 +302,12 @@ __realpath (const char *name, char *resolved) > if (buf[0] == '/') > { > dest = rpath + 1; /* It's an absolute symlink */ > - if (DOUBLE_SLASH_IS_DISTINCT_ROOT > - && buf[1] == '/' && buf[2] != '/') > - *dest++ = '/'; > + if (DOUBLE_SLASH_IS_DISTINCT_ROOT) > + { > + if (buf[1] == '/' && buf[2] != '/') > + *dest++ = '/'; > + *dest = '\0'; > + } > } > else > { > diff --git a/lib/canonicalize.c b/lib/canonicalize.c > index ed094b7..583c1a4 100644 > --- a/lib/canonicalize.c > +++ b/lib/canonicalize.c > @@ -145,8 +145,12 @@ canonicalize_filename_mode (const char *name, > canonicalize_mode_t can_mode) > rname_limit = rname + PATH_MAX; > rname[0] = '/'; > dest = rname + 1; > - if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] != > '/') > - *dest++ = '/'; > + if (DOUBLE_SLASH_IS_DISTINCT_ROOT) > + { > + if (name[1] == '/' && name[2] != '/') > + *dest++ = '/'; > + *dest = '\0'; > + } > } > > for (start = name; *start; start = end) > @@ -267,9 +271,12 @@ canonicalize_filename_mode (const char *name, > canonicalize_mode_t can_mode) > if (buf[0] == '/') > { > dest = rname + 1; /* It's an absolute symlink */ > - if (DOUBLE_SLASH_IS_DISTINCT_ROOT > - && buf[1] == '/' && buf[2] != '/') > - *dest++ = '/'; > + if (DOUBLE_SLASH_IS_DISTINCT_ROOT) > + { > + if (buf[1] == '/' && buf[2] != '/') > + *dest++ = '/'; > + *dest = '\0'; > + } > } > else > {
Thanks for handling this Eric. I was wondering if you had seen this and what overlap there is? http://lists.gnu.org/archive/html/bug-gnulib/2012-01/msg00253.html cheers, Pádraig.
