Package: coreutils;
Reported by: Mike Frysinger <vapier <at> gentoo.org>
Date: Tue, 10 Jan 2012 20:17:02 UTC
Severity: normal
Done: Pádraig Brady <P <at> draigBrady.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Pádraig Brady <P <at> draigBrady.com> To: Eric Blake <eblake <at> redhat.com> Cc: bug-gnulib <at> gnu.org, 10472 <at> debbugs.gnu.org Subject: bug#10472: [PATCH] canonicalize: fix // handling Date: Wed, 08 Feb 2012 10:13:01 +0000
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 <eblake <at> 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 <eblake <at> 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 <eblake <at> 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 <bruno <at> 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.