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

[Eric Blake]

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 <address@hidden>
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 <address@hidden>
---
 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  <address@hidden>
+
+       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  <address@hidden>

        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
                 {
-- 
1.7.7.6



-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature