GNU bug report logs - #10472
`realpath --relative-to=<path> /` outputs inconsistent trailing slash

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 10472 in the body.
You can then email your comments to 10472 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mike Frysinger <vapier <at> gentoo.org>
To: bug-coreutils <at> gnu.org
Subject: `realpath --relative-to=<path> /` outputs inconsistent trailing slash
Date: Tue, 10 Jan 2012 15:15:57 -0500

[Message part 1 (text/plain, inline)]

first some examples that look fine ...

these all output the same thing:
	realpath --relative-to=/usr /usr/bin
	realpath --relative-to=/usr/ /usr/bin
	realpath --relative-to=/usr /usr/bin/
	realpath --relative-to=/usr/ /usr/bin/
which is to say, they show:
	bin

as does these:
	realpath --relative-to=/usr/bin /usr
	realpath --relative-to=/usr/bin/ /usr
	realpath --relative-to=/usr/bin /usr/
	realpath --relative-to=/usr/bin/ /usr/
which is to say, they show:
	..

as does these:
	realpath --relative-to=/ /usr
	realpath --relative-to=/ /usr/
which is to say, they show:
	..

however, if the last argument is just the root path:
	realpath --relative-to=/usr /
	realpath --relative-to=/usr/ /
we end up with a trailing slash:
	../

for consistency, i don't think that should be the case

(reported by Ulrich Müller via https://bugs.gentoo.org/398339)
-mike

[signature.asc (application/pgp-signature, inline)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mike Frysinger <vapier <at> gentoo.org>
To: bug-coreutils <at> gnu.org
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Tue, 10 Jan 2012 16:53:41 -0500

[Message part 1 (text/plain, inline)]

On Tuesday 10 January 2012 15:15:57 Mike Frysinger wrote:
> as does these:
> 	realpath --relative-to=/ /usr
> 	realpath --relative-to=/ /usr/
> which is to say, they show:
> 	..

sorry, typo here ... these actually output:
	../usr

i guess that should be just "usr".
-mike

[signature.asc (application/pgp-signature, inline)]

Message #14 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Mike Frysinger <vapier <at> gentoo.org>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Tue, 10 Jan 2012 22:30:48 +0000

On 01/10/2012 09:53 PM, Mike Frysinger wrote:
> On Tuesday 10 January 2012 15:15:57 Mike Frysinger wrote:
>> as does these:
>> 	realpath --relative-to=/ /usr
>> 	realpath --relative-to=/ /usr/
>> which is to say, they show:
>> 	..
> 
> sorry, typo here ... these actually output:
> 	../usr
> 
> i guess that should be just "usr".
> -mike

Agreed. python concurs too:

>>> os.path.relpath(start='/usr',path='/')
'..'
>>> os.path.relpath(start='/',path='/usr')
'usr'

Essentially in these edge cases the relative paths
printed are valid, but not canonicalised.

I'll fix it up.

cheers,
Pádraig.

Message #17 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Mike Frysinger <vapier <at> gentoo.org>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Tue, 10 Jan 2012 18:08:00 -0500

[Message part 1 (text/plain, inline)]

On Tuesday 10 January 2012 17:30:48 Pádraig Brady wrote:
> Essentially in these edge cases the relative paths
> printed are valid, but not canonicalised.

sure ... i think we all agree on that :)
-mike

[signature.asc (application/pgp-signature, inline)]

Message #22 received at 10472-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Mike Frysinger <vapier <at> gentoo.org>
Cc: 10472-done <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Wed, 25 Jan 2012 16:55:09 +0000

[Message part 1 (text/plain, inline)]

On 01/10/2012 10:30 PM, Pádraig Brady wrote:
> On 01/10/2012 09:53 PM, Mike Frysinger wrote:
>> On Tuesday 10 January 2012 15:15:57 Mike Frysinger wrote:
>>> as does these:
>>> 	realpath --relative-to=/ /usr
>>> 	realpath --relative-to=/ /usr/
>>> which is to say, they show:
>>> 	..
>>
>> sorry, typo here ... these actually output:
>> 	../usr
>>
>> i guess that should be just "usr".
>> -mike
> 
> Agreed. python concurs too:
> 
>>>> os.path.relpath(start='/usr',path='/')
> '..'
>>>> os.path.relpath(start='/',path='/usr')
> 'usr'
> 
> Essentially in these edge cases the relative paths
> printed are valid, but not canonicalised.
> 
> I'll fix it up.

Proposed fix attached.

cheers,
Pádraig.

[relpath-edge-cases.diff (text/plain, attachment)]

Message #25 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: 10472 <at> debbugs.gnu.org
Cc: P <at> draigBrady.com
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Wed, 25 Jan 2012 18:10:20 +0100

Pádraig Brady wrote:
...
> Subject: [PATCH] realpath: remove extraneous '/' for --relative-to edge cases
>
> * src/realpath.c (path_common_prefix): Be consistent and
> always include a leading '/' in the count returned.
> (relpath): Account for the change in path_common_prefix()
> and avoid outputting extra '/' chars in relative paths that
> span the root dir.
> * tests/misc/realpath: Add the two reported cases.
> Reported by Mike Frysinger
> ---
>  src/realpath.c      |   16 +++++++++++-----
>  tests/misc/realpath |   11 ++++++++---
>  2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/src/realpath.c b/src/realpath.c
> index b39f7bf..8a07ab6 100644
> --- a/src/realpath.c
> +++ b/src/realpath.c
> @@ -136,7 +136,7 @@ path_common_prefix (const char *path1, const char *path2)
>        if (*path1 != *path2)
>          break;
>        if (*path1 == '/')
> -        ret = i;
> +        ret = i + 1;
>        path1++;
>        path2++;
>        i++;
> @@ -171,11 +171,16 @@ relpath (const char *can_fname)
>        const char *relto_suffix = can_relative_to + common_index;
>        const char *fname_suffix = can_fname + common_index;
>
> +      /* skip over extraneous '/'.  */
> +      if (*relto_suffix == '/')
> +        relto_suffix++;
> +      if (*fname_suffix == '/')
> +        fname_suffix++;
> +
>        /* Replace remaining components of --relative-to with '..', to get
>           to a common directory.  Then output the remainder of fname.  */
>        if (*relto_suffix)
>          {
> -          ++relto_suffix;
>            printf ("%s", "..");
>            for (; *relto_suffix; ++relto_suffix)
>              {
> @@ -183,14 +188,15 @@ relpath (const char *can_fname)
>                  printf ("%s", "/..");
>              }
>
> -          printf ("%s", fname_suffix);
> +          if (*fname_suffix)
> +            printf ("/%s", fname_suffix);
>          }
>        else
>          {
>            if (*fname_suffix)
> -            printf ("%s", ++fname_suffix);
> +            printf ("%s", fname_suffix);
>            else
> -            printf ("%c", '.');
> +            putchar ('.');
>          }

That looks fine and passes existing and new tests (of course).
Thanks.

On an unrelated note, have you considered removing the remaining
printf uses in favor of fputc/fputs, since they're all trivial?

Message #28 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Wed, 25 Jan 2012 18:00:24 +0000

On 01/25/2012 05:10 PM, Jim Meyering wrote:
> On an unrelated note, have you considered removing the remaining
> printf uses in favor of fputc/fputs, since they're all trivial?

Good point. I'll undo the s/printf/putchar/ change in this patch,
and do a follow up, using the lower level functions.

cheers,
Pádraig.

Message #31 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Wed, 25 Jan 2012 18:42:45 +0000

On 01/25/2012 06:00 PM, Pádraig Brady wrote:
> On 01/25/2012 05:10 PM, Jim Meyering wrote:
>> On an unrelated note, have you considered removing the remaining
>> printf uses in favor of fputc/fputs, since they're all trivial?
> 
> Good point. I'll undo the s/printf/putchar/ change in this patch,
> and do a follow up, using the lower level functions.

The patch below, gives an 11% improvement.
Tested like:

seq 1000000 | sed 's|.*|/1/2/3/4/&|' > paths
time xargs src/realpath -sm < paths >/dev/null
time xargs src/realpath -sm --relative-to=/1/2/3 < paths >/dev/null

cheers,
Pádraig.

diff --git a/src/realpath.c b/src/realpath.c
index b03f375..2dc5e11 100644
--- a/src/realpath.c
+++ b/src/realpath.c
@@ -181,26 +181,27 @@ relpath (const char *can_fname)
          to a common directory.  Then output the remainder of fname.  */
       if (*relto_suffix)
         {
-          printf ("%s", "..");
+          fputs ("..", stdout);
           for (; *relto_suffix; ++relto_suffix)
             {
               if (*relto_suffix == '/')
-                printf ("%s", "/..");
+                fputs ("/..", stdout);
             }

           if (*fname_suffix)
-            printf ("/%s", fname_suffix);
+            {
+              putchar ('/');
+              fputs (fname_suffix, stdout);
+            }
         }
       else
         {
           if (*fname_suffix)
-            printf ("%s", fname_suffix);
+            fputs (fname_suffix, stdout);
           else
-            printf ("%c", '.');
+            putchar ('.');
         }

-      putchar (use_nuls ? '\0' : '\n');
-
       return true;
     }

@@ -228,7 +229,9 @@ process_path (const char *fname, int can_mode)
     }

   if (!relpath (can_fname))
-    printf ("%s%c", can_fname, (use_nuls ? '\0' : '\n'));
+    fputs (can_fname, stdout);
+
+  putchar (use_nuls ? '\0' : '\n');

   free (can_fname);

Message #34 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Wed, 25 Jan 2012 20:41:24 +0100

Pádraig Brady wrote:
> On 01/25/2012 06:00 PM, Pádraig Brady wrote:
>> On 01/25/2012 05:10 PM, Jim Meyering wrote:
>>> On an unrelated note, have you considered removing the remaining
>>> printf uses in favor of fputc/fputs, since they're all trivial?
>>
>> Good point. I'll undo the s/printf/putchar/ change in this patch,
>> and do a follow up, using the lower level functions.
>
> The patch below, gives an 11% improvement.
> Tested like:
>
> seq 1000000 | sed 's|.*|/1/2/3/4/&|' > paths
> time xargs src/realpath -sm < paths >/dev/null
> time xargs src/realpath -sm --relative-to=/1/2/3 < paths >/dev/null

Thanks!

> diff --git a/src/realpath.c b/src/realpath.c
...
> @@ -181,26 +181,27 @@ relpath (const char *can_fname)
...
> -      putchar (use_nuls ? '\0' : '\n');
> -
>        return true;
>      }
>
> @@ -228,7 +229,9 @@ process_path (const char *fname, int can_mode)
>      }
>
>    if (!relpath (can_fname))
> -    printf ("%s%c", can_fname, (use_nuls ? '\0' : '\n'));
> +    fputs (can_fname, stdout);
> +
> +  putchar (use_nuls ? '\0' : '\n');

Nice to see the "use_nuls" use factored out.
With that, it is feasible to move it's declaration into main.
ACK, either way.

Message #37 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Mike Frysinger <vapier <at> gentoo.org>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Mon, 30 Jan 2012 14:10:45 -0700

[Message part 1 (text/plain, inline)]

On 01/10/2012 01:15 PM, Mike Frysinger wrote:
> however, if the last argument is just the root path:
> 	realpath --relative-to=/usr /
> 	realpath --relative-to=/usr/ /
> we end up with a trailing slash:
> 	../
> 
> for consistency, i don't think that should be the case
> 
> (reported by Ulrich Müller via https://bugs.gentoo.org/398339)

Another bug, on a system where // is distinct from /:

$ realpath --relative-to=/ //machine / // /bin
machine
.
.
bin
$ realpath --relative-to=// //machine / // /bin
machine
.
.
bin

when it should really be:

$ realpath --relative-to=/ //machine / // /bin
//machine
.
//
bin
$ realpath --relative-to=// //machine / // /bin
machine
/
.
/bin

We need to make realpath robust to correct leading // handling; I don't
know if we should follow the lead of 'dirname' in only doing it on
machines where // is special, or if it is easier to make it honor POSIX
by special-casing // everywhere even on machines where / and // are
identical.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #40 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Eric Blake <eblake <at> redhat.com>
Cc: Mike Frysinger <vapier <at> gentoo.org>, 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Mon, 30 Jan 2012 22:01:57 +0000

On 01/30/2012 09:10 PM, Eric Blake wrote:
> On 01/10/2012 01:15 PM, Mike Frysinger wrote:
>> however, if the last argument is just the root path:
>> 	realpath --relative-to=/usr /
>> 	realpath --relative-to=/usr/ /
>> we end up with a trailing slash:
>> 	../
>>
>> for consistency, i don't think that should be the case
>>
>> (reported by Ulrich Müller via https://bugs.gentoo.org/398339)
> 
> Another bug, on a system where // is distinct from /:
> 
> $ realpath --relative-to=/ //machine / // /bin
> machine
> .
> .
> bin
> $ realpath --relative-to=// //machine / // /bin
> machine
> .
> .
> bin
> 
> when it should really be:
> 
> $ realpath --relative-to=/ //machine / // /bin
> //machine
> .
> //
> bin
> $ realpath --relative-to=// //machine / // /bin
> machine
> /
> .
> /bin
> 
> We need to make realpath robust to correct leading // handling; I don't
> know if we should follow the lead of 'dirname' in only doing it on
> machines where // is special, or if it is easier to make it honor POSIX
> by special-casing // everywhere even on machines where / and // are
> identical.

So on such a machine, I guess `readlink -m //machine/` outputs '//machine'.
To match up with that, I think it makes sense to only do this on systems
where a double slash is significant.

BTW, this is how I'm interpreting this example:

> $ realpath --relative-to=// //machine / // /bin
> machine
> /
> .
> /bin

I'm taking --relative-to=// to mean relative to "the network".
Hence relative output will be machines on the network,
while absolute are local paths.

gnulib says // matters for Apollo DomainOS (too old to port to),
Cygwin, and z/OS.

cheers,
Pádraig.

Message #43 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: Mike Frysinger <vapier <at> gentoo.org>, 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Mon, 30 Jan 2012 15:22:22 -0700

[Message part 1 (text/plain, inline)]

On 01/30/2012 03:01 PM, Pádraig Brady wrote:
>> $ realpath --relative-to=// //machine / // /bin
>> machine
>> /
>> .
>> /bin
>>
>> We need to make realpath robust to correct leading // handling; I don't
>> know if we should follow the lead of 'dirname' in only doing it on
>> machines where // is special, or if it is easier to make it honor POSIX
>> by special-casing // everywhere even on machines where / and // are
>> identical.
> 
> So on such a machine, I guess `readlink -m //machine/` outputs '//machine'.

Correct.

> To match up with that, I think it makes sense to only do this on systems
> where a double slash is significant.

I'm tending to agree - readlink, dirname, and realpath should all behave
consistently within a single compilation of coreutils.

> 
> BTW, this is how I'm interpreting this example:
> 
>> $ realpath --relative-to=// //machine / // /bin
>> machine
>> /
>> .
>> /bin
> 
> I'm taking --relative-to=// to mean relative to "the network".
> Hence relative output will be machines on the network,
> while absolute are local paths.

Correct.

Another bug still in the latest coreutils.git, but only on platforms
with distinct //:

$ src/realpath / // ///
/
//
//

Oops, that last line should be /.  This bug is shared by readlink:

$ src/readlink -m ///
//

> 
> gnulib says // matters for Apollo DomainOS (too old to port to),
> Cygwin, and z/OS.

And I tested on Cygwin (if it wasn't obvious :)

I can try to develop patches as part of porting coreutils 8.15 to cygwin
(cygwin already has a realpath(1), but it is severely limited in that it
only takes one file name and no command line options on what to do with
that name; I'm pretty sure that the coreutils realpath is upwards
compatible with the existing cygwin realpath:
http://cygwin.com/ml/cygwin-apps/2012-01/msg00080.html).

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #46 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Eric Blake <eblake <at> redhat.com>
Cc: Mike Frysinger <vapier <at> gentoo.org>, 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: `realpath --relative-to=<path> /` outputs inconsistent
	trailing slash
Date: Mon, 30 Jan 2012 23:53:08 +0000

On 01/30/2012 10:22 PM, Eric Blake wrote:
> On 01/30/2012 03:01 PM, Pádraig Brady wrote:
>>> $ realpath --relative-to=// //machine / // /bin
>>> machine
>>> /
>>> .
>>> /bin
>>>
>>> We need to make realpath robust to correct leading // handling; I don't
>>> know if we should follow the lead of 'dirname' in only doing it on
>>> machines where // is special, or if it is easier to make it honor POSIX
>>> by special-casing // everywhere even on machines where / and // are
>>> identical.
>>
>> So on such a machine, I guess `readlink -m //machine/` outputs '//machine'.
> 
> Correct.
> 
>> To match up with that, I think it makes sense to only do this on systems
>> where a double slash is significant.
> 
> I'm tending to agree - readlink, dirname, and realpath should all behave
> consistently within a single compilation of coreutils.
> 
>>
>> BTW, this is how I'm interpreting this example:
>>
>>> $ realpath --relative-to=// //machine / // /bin
>>> machine
>>> /
>>> .
>>> /bin
>>
>> I'm taking --relative-to=// to mean relative to "the network".
>> Hence relative output will be machines on the network,
>> while absolute are local paths.
> 
> Correct.
> 
> Another bug still in the latest coreutils.git, but only on platforms
> with distinct //:
> 
> $ src/realpath / // ///
> /
> //
> //
> 
> Oops, that last line should be /.  This bug is shared by readlink:
> 
> $ src/readlink -m ///
> //

The above will require a gnulib fix

>> gnulib says // matters for Apollo DomainOS (too old to port to),
>> Cygwin, and z/OS.
> 
> And I tested on Cygwin (if it wasn't obvious :)
> 
> I can try to develop patches as part of porting coreutils 8.15 to cygwin

I'll take you up on that, thanks!

> (cygwin already has a realpath(1), but it is severely limited in that it
> only takes one file name and no command line options on what to do with
> that name; I'm pretty sure that the coreutils realpath is upwards
> compatible with the existing cygwin realpath:
> http://cygwin.com/ml/cygwin-apps/2012-01/msg00080.html).

I had a look at the source, and it's just a simple wrapper around realpath(),
that only accepts a single path. So coreutils realpath should be a drop in replacement.
(wow it's hard to navigate cygwin stuff. It took me 3 mins to find the latest source).

cheers,
Pádraig.

Message #49 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: bug-gnulib <at> gnu.org
Cc: 10472 <at> debbugs.gnu.org
Subject: [PATCH] canonicalize: fix // handling
Date: Sat,  4 Feb 2012 09:56:32 -0700

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.

Signed-off-by: Eric Blake <eblake <at> redhat.com>
---
 ChangeLog          |    6 ++++++
 lib/canonicalize.c |   12 +++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4bbe447..a9aa40a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2012-02-04  Eric Blake  <eblake <at> redhat.com>
+
+	canonicalize: fix // handling
+	* lib/canonicalize.c (canonicalize_filename_mode): Don't convert
+	/// to //, since only // is special.
+
 2012-02-04  Bruno Haible  <bruno <at> clisp.org>

 	ioctl: Fix test failure on native Windows.
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index d3e5645..ed094b7 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -145,7 +145,7 @@ 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] == '/')
+      if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] != '/')
         *dest++ = '/';
     }

@@ -169,7 +169,7 @@ canonicalize_filename_mode (const char *name, canonicalize_mode_t can_mode)
           if (dest > rname + 1)
             while ((--dest)[-1] != '/');
           if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1
-              && *dest == '/')
+              && *dest == '/' && dest[1] != '/')
             dest++;
         }
       else
@@ -267,7 +267,8 @@ 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] == '/')
+                  if (DOUBLE_SLASH_IS_DISTINCT_ROOT
+                      && buf[1] == '/' && buf[2] != '/')
                     *dest++ = '/';
                 }
               else
@@ -277,7 +278,7 @@ canonicalize_filename_mode (const char *name, canonicalize_mode_t can_mode)
                   if (dest > rname + 1)
                     while ((--dest)[-1] != '/');
                   if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1
-                      && *dest == '/')
+                      && *dest == '/' && dest[1] != '/')
                     dest++;
                 }

@@ -295,7 +296,8 @@ canonicalize_filename_mode (const char *name, canonicalize_mode_t can_mode)
     }
   if (dest > rname + 1 && dest[-1] == '/')
     --dest;
-  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 && *dest == '/')
+  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1
+      && *dest == '/' && dest[1] != '/')
     dest++;
   *dest = '\0';
   if (rname_limit != dest + 1)
-- 
1.7.7.6

Message #52 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
Cc: bug-gnulib <at> gnu.org, 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: [PATCH] canonicalize: fix // handling
Date: Sat, 04 Feb 2012 10:59:21 -0700

[Message part 1 (text/plain, inline)]

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.
> 

> +++ b/lib/canonicalize.c
> @@ -145,7 +145,7 @@ 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] == '/')
> +      if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] != '/')
>          *dest++ = '/';

This never initializes rname[1] if name is "///",

> @@ -295,7 +296,8 @@ canonicalize_filename_mode (const char *name, canonicalize_mode_t can_mode)
>      }
>    if (dest > rname + 1 && dest[-1] == '/')
>      --dest;
> -  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 && *dest == '/')
> +  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1
> +      && *dest == '/' && dest[1] != '/')
>      dest++;

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';
     }

   for (start = end = name; *start; start = end)
diff --git i/lib/canonicalize.c w/lib/canonicalize.c
index ed094b7..2c73094 100644
--- i/lib/canonicalize.c
+++ w/lib/canonicalize.c
@@ -147,6 +147,7 @@ canonicalize_filename_mode (const char *name,
canonicalize_mode_t can_mode)
       dest = rname + 1;
       if (DOUBLE_SLASH_IS_DISTINCT_ROOT && name[1] == '/' && name[2] !=
'/')
         *dest++ = '/';
+      *dest = '\0';
     }

   for (start = name; *start; start = end)


-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #55 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
Cc: bug-gnulib <at> gnu.org, 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: [PATCH] canonicalize: fix // handling
Date: Sat, 04 Feb 2012 11:38:44 -0700

[Message part 1 (text/plain, inline)]

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



-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #58 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: 10472 <at> debbugs.gnu.org
Subject: [PATCH] realpath: fix problems with // handling
Date: Sat,  4 Feb 2012 12:05:56 -0700

On platforms like Cygwin where / and // are distinct, realpath was
incorrectly collapsing // into /.  http://debbugs.gnu.org/10472.

* src/realpath.c (path_common_prefix): When // is special, treat /
and // as having no common match.
(relpath): Allow for no match even without --relative-base.
---

This is the coreutils side of the patch; for this to work, we also
have to upgrade to the latest gnulib.

I'm not pushing this until we decide what to do about testing; I
guess we've already figured out how to make basename and dirname
tests conditional on whether they are on Linux or Cygwin (that is,
whether // and / are the same or different), so I should do something
similar to that for the realpath test.  But I can at least get a
code review on realpath.c while figuring out the testing situation.

 src/realpath.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/src/realpath.c b/src/realpath.c
index 2dc5e11..f06fbcc 100644
--- a/src/realpath.c
+++ b/src/realpath.c
@@ -131,6 +131,10 @@ path_common_prefix (const char *path1, const char *path2)
   int i = 0;
   int ret = 0;

+  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && *path1 == '/' && *path2 == '/'
+      && (path1[1] == '/') != (path2[1] == '/'))
+    return 0;
+
   while (*path1 && *path2)
     {
       if (*path1 != *path2)
@@ -168,6 +172,9 @@ relpath (const char *can_fname)

       /* Skip the prefix common to --relative-to and path.  */
       int common_index = path_common_prefix (can_relative_to, can_fname);
+      if (!common_index)
+        return false;
+
       const char *relto_suffix = can_relative_to + common_index;
       const char *fname_suffix = can_fname + common_index;

-- 
1.7.7.6

Message #61 received at 10472 <at> debbugs.gnu.org (full text, mbox):

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: Re: 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.

Message #64 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: bug-gnulib <at> gnu.org, 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: [PATCH] canonicalize: fix // handling
Date: Wed, 08 Feb 2012 09:19:07 -0700

[Message part 1 (text/plain, inline)]

On 02/08/2012 03:13 AM, Pádraig Brady wrote:

>> 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.
>>

> 
> Thanks for handling this Eric.

No problem.

> 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

I saw it go by, but never looked at it closely. I guess it's time to
revive that thread, although it may need rebasing now.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #67 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Eric Blake <eblake <at> redhat.com>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: [PATCH] realpath: fix problems with // handling
Date: Mon, 20 Feb 2012 15:21:28 +0000

On 02/04/2012 07:05 PM, Eric Blake wrote:
> On platforms like Cygwin where / and // are distinct, realpath was
> incorrectly collapsing // into /.  http://debbugs.gnu.org/10472.
> 
> * src/realpath.c (path_common_prefix): When // is special, treat /
> and // as having no common match.
> (relpath): Allow for no match even without --relative-base.
> ---
> 
> This is the coreutils side of the patch; for this to work, we also
> have to upgrade to the latest gnulib.
> 
> I'm not pushing this until we decide what to do about testing; I
> guess we've already figured out how to make basename and dirname
> tests conditional on whether they are on Linux or Cygwin (that is,
> whether // and / are the same or different), so I should do something
> similar to that for the realpath test.  But I can at least get a
> code review on realpath.c while figuring out the testing situation.
> 
>  src/realpath.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/src/realpath.c b/src/realpath.c
> index 2dc5e11..f06fbcc 100644
> --- a/src/realpath.c
> +++ b/src/realpath.c
> @@ -131,6 +131,10 @@ path_common_prefix (const char *path1, const char *path2)
>    int i = 0;
>    int ret = 0;
> 
> +  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && *path1 == '/' && *path2 == '/'
> +      && (path1[1] == '/') != (path2[1] == '/'))
> +    return 0;
> +
>    while (*path1 && *path2)
>      {
>        if (*path1 != *path2)
> @@ -168,6 +172,9 @@ relpath (const char *can_fname)
> 
>        /* Skip the prefix common to --relative-to and path.  */
>        int common_index = path_common_prefix (can_relative_to, can_fname);
> +      if (!common_index)
> +        return false;
> +
>        const char *relto_suffix = can_relative_to + common_index;
>        const char *fname_suffix = can_fname + common_index;
> 

is the DOUBLE_SLASH_IS_DISTINCT_ROOT check needed in realpath.c
I.E. if we get a leading // then that define is implicit?

Also doesn't path_prefix() need the same adjustment,
so as to verify --relative-base in the same way?

cheers,
Pádraig.

Message #70 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: [PATCH] realpath: fix problems with // handling
Date: Tue, 13 Mar 2012 21:15:16 -0600

[Message part 1 (text/plain, inline)]

On 02/20/2012 08:21 AM, Pádraig Brady wrote:
> On 02/04/2012 07:05 PM, Eric Blake wrote:
>> On platforms like Cygwin where / and // are distinct, realpath was
>> incorrectly collapsing // into /.  http://debbugs.gnu.org/10472.
>>

>> This is the coreutils side of the patch; for this to work, we also
>> have to upgrade to the latest gnulib.

The gnulib update has happened in the meantime.

>>
>> I'm not pushing this until we decide what to do about testing; I
>> guess we've already figured out how to make basename and dirname
>> tests conditional on whether they are on Linux or Cygwin (that is,
>> whether // and / are the same or different), so I should do something
>> similar to that for the realpath test.  But I can at least get a
>> code review on realpath.c while figuring out the testing situation.

I'm planning on resubmitting this series soon, once I polish my test,
with hopes of inclusion in this week's release.

>> +++ b/src/realpath.c
>> @@ -131,6 +131,10 @@ path_common_prefix (const char *path1, const char *path2)
>>    int i = 0;
>>    int ret = 0;
>>
>> +  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && *path1 == '/' && *path2 == '/'
>> +      && (path1[1] == '/') != (path2[1] == '/'))
>> +    return 0;
>> +
>>    while (*path1 && *path2)
>>      {
>>        if (*path1 != *path2)

> is the DOUBLE_SLASH_IS_DISTINCT_ROOT check needed in realpath.c
> I.E. if we get a leading // then that define is implicit?

Good point - path_common_prefix is only ever called with canonical
names, which means that it will only ever encounter // on systems where
it matters.

> 
> Also doesn't path_prefix() need the same adjustment,
> so as to verify --relative-base in the same way?

Yes, it looks like it.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #73 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
Cc: Pádraig Brady <P <at> draigBrady.com>, 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: [PATCH] realpath: fix problems with // handling
Date: Tue, 13 Mar 2012 22:07:41 -0600

[Message part 1 (text/plain, inline)]

On 03/13/2012 09:15 PM, Eric Blake wrote:
>> Also doesn't path_prefix() need the same adjustment,
>> so as to verify --relative-base in the same way?
> 
> Yes, it looks like it.

In fact, I found another bug, this time present also on Linux:

$ realpath --relative-base=/ --relative-to=/ /
/

when it should really output '.' (since '/' relative to itself is '.',
and ALL files are below '/' [except when '//' is special]).  Likewise:

$ realpath --relative-base=/usr/local --relative-to=/usr \
    /usr /usr/local/lib
/usr
/usr/local/lib

when it should really output '/usr' (absolute, since it is not a child
of /usr/local) and 'local/lib' (which is a file below /usr/local, and an
output name relative to /usr).

My test caught these, so now I have to revisit my realpath.c patch.
It's getting too late tonight, so I'll have to post the series early
tomorrow.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #76 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Eric Blake <eblake <at> redhat.com>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: [PATCH] realpath: fix problems with // handling
Date: Wed, 14 Mar 2012 09:12:57 +0000

On 03/14/2012 04:07 AM, Eric Blake wrote:
> On 03/13/2012 09:15 PM, Eric Blake wrote:
>>> Also doesn't path_prefix() need the same adjustment,
>>> so as to verify --relative-base in the same way?
>>
>> Yes, it looks like it.
> 
> In fact, I found another bug, this time present also on Linux:
> 
> $ realpath --relative-base=/ --relative-to=/ /
> /

This may be a local issue?
$ src/realpath --relative-base=/ --relative-to=/ /
.

> 
> when it should really output '.' (since '/' relative to itself is '.',
> and ALL files are below '/' [except when '//' is special]).  Likewise:
> 
> $ realpath --relative-base=/usr/local --relative-to=/usr \
>     /usr /usr/local/lib
> /usr
> /usr/local/lib
> 
> when it should really output '/usr' (absolute, since it is not a child
> of /usr/local) and 'local/lib' (which is a file below /usr/local, and an
> output name relative to /usr).

Well that was by design. I.E. --relative-base is a guard,
which if either --relative-to or the specified paths go higher,
an absolute name will be output.

cheers,
Pádraig.

Message #79 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: [PATCH] realpath: fix problems with // handling
Date: Wed, 14 Mar 2012 03:53:35 -0600

[Message part 1 (text/plain, inline)]

On 03/14/2012 03:12 AM, Pádraig Brady wrote:
> On 03/14/2012 04:07 AM, Eric Blake wrote:
>> On 03/13/2012 09:15 PM, Eric Blake wrote:
>>>> Also doesn't path_prefix() need the same adjustment,
>>>> so as to verify --relative-base in the same way?
>>>
>>> Yes, it looks like it.
>>
>> In fact, I found another bug, this time present also on Linux:
>>
>> $ realpath --relative-base=/ --relative-to=/ /
>> /
> 
> This may be a local issue?
> $ src/realpath --relative-base=/ --relative-to=/ /
> .

Looks like I was testing after some of my modifications to realpath.c;
I'm rebasing my tree to add the test before any of my realpath.c
changes, so that I can then be avoiding regressions.  But it still
doesn't explain:

$ src/realpath --relative-base=/ --relative-to=/ / /usr
.
/usr

where I would expect 'usr', not '/usr', since /usr is indeed a child of /.

>> $ realpath --relative-base=/usr/local --relative-to=/usr \
>>     /usr /usr/local/lib
>> /usr
>> /usr/local/lib
>>
>> when it should really output '/usr' (absolute, since it is not a child
>> of /usr/local) and 'local/lib' (which is a file below /usr/local, and an
>> output name relative to /usr).
> 
> Well that was by design. I.E. --relative-base is a guard,
> which if either --relative-to or the specified paths go higher,
> an absolute name will be output.

The documentation wasn't very clear on that point.  Either we need to
fix the documentation, or consider whether to make 'realpath' fail if
--relative-base is not a prefix of --relative-to, or even add another
option that allows the behavior I was expecting of making the two
orthogonal (that is, where it really is an independent filter - if the
path being canonicalized is a child of --relative-base, then output it
relative to --relative-to; otherwise output absolute).

Also, would it make sense to have --relative-base without --relative-to
imply a --relative-to of the same directory?  That is,

realpath --relative-base=/usr /usr

would be a useful shorthand for

realpath --relative-base=/usr --relative-to=/usr /usr

instead of its current error condition.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #82 received at 10472 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Eric Blake <eblake <at> redhat.com>
Cc: 10472 <at> debbugs.gnu.org
Subject: Re: bug#10472: [PATCH] realpath: fix problems with // handling
Date: Wed, 14 Mar 2012 10:49:43 +0000

On 03/14/2012 09:53 AM, Eric Blake wrote:
> On 03/14/2012 03:12 AM, Pádraig Brady wrote:
>> On 03/14/2012 04:07 AM, Eric Blake wrote:
>>> On 03/13/2012 09:15 PM, Eric Blake wrote:
>>>>> Also doesn't path_prefix() need the same adjustment,
>>>>> so as to verify --relative-base in the same way?
>>>>
>>>> Yes, it looks like it.
>>>
>>> In fact, I found another bug, this time present also on Linux:
>>>
>>> $ realpath --relative-base=/ --relative-to=/ /
>>> /
>>
>> This may be a local issue?
>> $ src/realpath --relative-base=/ --relative-to=/ /
>> .
> 
> Looks like I was testing after some of my modifications to realpath.c;
> I'm rebasing my tree to add the test before any of my realpath.c
> changes, so that I can then be avoiding regressions.  But it still
> doesn't explain:
> 
> $ src/realpath --relative-base=/ --relative-to=/ / /usr
> .
> /usr

Agreed. --relative-base=/ should essentially be a noop

> where I would expect 'usr', not '/usr', since /usr is indeed a child of /.
> 
>>> $ realpath --relative-base=/usr/local --relative-to=/usr \
>>>     /usr /usr/local/lib
>>> /usr
>>> /usr/local/lib
>>>
>>> when it should really output '/usr' (absolute, since it is not a child
>>> of /usr/local) and 'local/lib' (which is a file below /usr/local, and an
>>> output name relative to /usr).
>>
>> Well that was by design. I.E. --relative-base is a guard,
>> which if either --relative-to or the specified paths go higher,
>> an absolute name will be output.
> 
> The documentation wasn't very clear on that point.  Either we need to
> fix the documentation, or consider whether to make 'realpath' fail if
> --relative-base is not a prefix of --relative-to, or even add another
> option that allows the behavior I was expecting of making the two
> orthogonal (that is, where it really is an independent filter - if the
> path being canonicalized is a child of --relative-base, then output it
> relative to --relative-to; otherwise output absolute).
> 
> Also, would it make sense to have --relative-base without --relative-to
> imply a --relative-to of the same directory?  That is,
> 
> realpath --relative-base=/usr /usr

I considered that, and now I'm not sure why I didn't do it.
Having both relative-to and relative-base being the same,
is the common use case for --relative-base.

cheers,
Pádraig.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.