Subject: Re: bug#10472: [PATCH] canonicalize: fix // handling

From: Pádraig Brady <[email-masked]>
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 <[email-masked]>
> 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 <[email-masked]>
> ---
>  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  <[email-masked]>
> +
> +	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  <[email-masked]>
> 
>  	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.