Bug in canonicalize_path()

What if we let the trailing "." or ".." as it is?

On Windows, GetFullPathName() can canonicalize a given path.

$ uname -a
MINGW32_NT-5.0 DEV 1.0.10(0.46/3/2) 2003-10-11 10:14 i686 unknown

$ cat path.c

#include <windows.h>

int
main(void)
{
CHAR Buf[1024];

GetFullPathName("C:\\Temp", 1024, Buf, NULL);
printf("%s\n", Buf);
GetFullPathName("C:\\Temp\\", 1024, Buf, NULL);
printf("%s\n", Buf);
GetFullPathName("C:\\Temp\\..\\", 1024, Buf, NULL);
printf("%s\n", Buf);
GetFullPathName("C:\\Temp\\..\\..", 1024, Buf, NULL);
printf("%s\n", Buf);

return 0;
}

$ gcc path.c

$ ./a.exe
C:\Temp
C:\Temp\ # trailing "\" is not removed.
C:\ #
C:\ # C:\Temp\..\..

"Bruce Momjian" <pgman@candle.pha.pa.us> wrote
news:200508110356.j7B3uqR14136@candle.pha.pa.us...

I found that in port/path.c::canonicalize_path, that if the path was
supplied as "/usr/local/bin/../.." we would return /usr/local/bin. The
problem is then when we saw a trailing ".." we stripped it off and the
previous directory, but we never checked if the previous directory was
itself "..".

Patch applied to suppress trimming of ".." if ".." is above it. I tried
coding something that would handle "../.." but is started to look too
messy and not worth the effort.

I don't see a need to backpatch this, but it could produce errors with
weird supplied paths. Comments?

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, Pennsylvania

19073

----------------------------------------------------------------------------
----

? pg_config_paths.h
Index: path.c
===================================================================
RCS file: /cvsroot/pgsql/src/port/path.c,v
retrieving revision 1.51
diff -c -r1.51 path.c
*** path.c 26 Jan 2005 19:24:03 -0000 1.51
--- path.c 11 Aug 2005 03:52:06 -0000
***************
*** 284,290 ****

if (len > 2 && strcmp(path + len - 2, "/.") == 0)
trim_directory(path);
! else if (len > 3 && strcmp(path + len - 3, "/..") == 0)
{
trim_directory(path);
trim_directory(path); /* remove directory above */
--- 284,293 ----

if (len > 2 && strcmp(path + len - 2, "/.") == 0)
trim_directory(path);
! /* We can only deal with "/usr/local/..", not "/usr/local/../.." */
! else if (len > 3 && strcmp(path + len - 3, "/..") == 0 &&
! (len != 5 || strcmp(path, "../..") != 0) &&
! (len < 6 || strcmp(path + len - 6, "/../..") != 0))
{
trim_directory(path);
trim_directory(path); /* remove directory above */

----------------------------------------------------------------------------
----

Yep, it is a bug on 8.0.X. Are you suggesting a backpatch? We can do
it. Any objections?

---------------------------------------------------------------------------

William ZHANG wrote:

What if we let the trailing "." or ".." as it is?

On Windows, GetFullPathName() can canonicalize a given path.

$ uname -a
MINGW32_NT-5.0 DEV 1.0.10(0.46/3/2) 2003-10-11 10:14 i686 unknown

$ cat path.c

#include <windows.h>

int
main(void)
{
CHAR Buf[1024];

GetFullPathName("C:\\Temp", 1024, Buf, NULL);
printf("%s\n", Buf);
GetFullPathName("C:\\Temp\\", 1024, Buf, NULL);
printf("%s\n", Buf);
GetFullPathName("C:\\Temp\\..\\", 1024, Buf, NULL);
printf("%s\n", Buf);
GetFullPathName("C:\\Temp\\..\\..", 1024, Buf, NULL);
printf("%s\n", Buf);

return 0;
}

$ gcc path.c

$ ./a.exe
C:\Temp
C:\Temp\ # trailing "\" is not removed.
C:\ #
C:\ # C:\Temp\..\..

"Bruce Momjian" <pgman@candle.pha.pa.us> wrote
news:200508110356.j7B3uqR14136@candle.pha.pa.us...

I found that in port/path.c::canonicalize_path, that if the path was
supplied as "/usr/local/bin/../.." we would return /usr/local/bin. The
problem is then when we saw a trailing ".." we stripped it off and the
previous directory, but we never checked if the previous directory was
itself "..".

Patch applied to suppress trimming of ".." if ".." is above it. I tried
coding something that would handle "../.." but is started to look too
messy and not worth the effort.

I don't see a need to backpatch this, but it could produce errors with
weird supplied paths. Comments?

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, Pennsylvania

19073

----------------------------------------------------------------------------
----

? pg_config_paths.h
Index: path.c
===================================================================
RCS file: /cvsroot/pgsql/src/port/path.c,v
retrieving revision 1.51
diff -c -r1.51 path.c
*** path.c 26 Jan 2005 19:24:03 -0000 1.51
--- path.c 11 Aug 2005 03:52:06 -0000
***************
*** 284,290 ****

if (len > 2 && strcmp(path + len - 2, "/.") == 0)
trim_directory(path);
! else if (len > 3 && strcmp(path + len - 3, "/..") == 0)
{
trim_directory(path);
trim_directory(path); /* remove directory above */
--- 284,293 ----

if (len > 2 && strcmp(path + len - 2, "/.") == 0)
trim_directory(path);
! /* We can only deal with "/usr/local/..", not "/usr/local/../.." */
! else if (len > 3 && strcmp(path + len - 3, "/..") == 0 &&
! (len != 5 || strcmp(path, "../..") != 0) &&
! (len < 6 || strcmp(path + len - 6, "/../..") != 0))
{
trim_directory(path);
trim_directory(path); /* remove directory above */

----------------------------------------------------------------------------
----

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Yep, it is a bug on 8.0.X. Are you suggesting a backpatch? We can do
it. Any objections?

I think he's suggesting that we depend on GetFullPathName(), which seems
a bit pointless considering we have to write the code anyway for other
platforms.

I didn't much like the patch as-is; we should think a little harder
about fixing the logic properly.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Yep, it is a bug on 8.0.X. Are you suggesting a backpatch? We can do
it. Any objections?

I think he's suggesting that we depend on GetFullPathName(), which seems
a bit pointless considering we have to write the code anyway for other
platforms.

Oh.

I didn't much like the patch as-is; we should think a little harder
about fixing the logic properly.

I thought about it. Here is where we get stuck:

usr/local/../../.. has to return ..

Yuck. If it an absolute path like /usr, you are fine.

In my first attempt, I counted the number of ".." groups, then went up
to remove each "..", and them remove a regular directory for each "..".
And then you have this case:

/usr/local/../bin/../..

Here you hit the first ".." as you are going up. It just seemed like a
lost cause.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

----- Original Message -----
From: "Tom Lane" <tgl@sss.pgh.pa.us>
To: "Bruce Momjian" <pgman@candle.pha.pa.us>
Cc: "William ZHANG" <uniware@zedware.org>; <pgsql-patches@postgresql.org>
Sent: Friday, August 12, 2005 10:31 AM
Subject: Re: [PATCHES] Bug in canonicalize_path()

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Yep, it is a bug on 8.0.X. Are you suggesting a backpatch? We can do
it. Any objections?

I think he's suggesting that we depend on GetFullPathName(), which seems
a bit pointless considering we have to write the code anyway for other
platforms.

If it must be fixed, I think we can learn sth. from GetFullPathName().
Since we can not rely on this function on other platforms, we should
write it from scratch.

I wrote:

I didn't much like the patch as-is; we should think a little harder
about fixing the logic properly.

Like, say, this. (Very poorly tested but I think it's right.)

regards, tom lane

*** src/port/path.c.orig	Thu Aug 11 21:39:22 2005
--- src/port/path.c	Thu Aug 11 22:46:05 2005
***************
*** 226,231 ****
--- 226,232 ----
  {
  	char	   *p, *to_p;
  	bool		was_sep = false;
+ 	int			pending_strips;

#ifdef WIN32
/*
***************
*** 277,296 ****

/*
* Remove any trailing uses of "." and process ".." ourselves
*/
for (;;)
{
int len = strlen(path);

  		if (len > 2 && strcmp(path + len - 2, "/.") == 0)
  			trim_directory(path);
! 		/* We can only deal with "/usr/local/..", not "/usr/local/../.." */
! 		else if (len > 3 && strcmp(path + len - 3, "/..") == 0 &&
! 				 (len != 5 || strcmp(path, "../..") != 0) &&
! 				 (len < 6 || strcmp(path + len - 6, "/../..") != 0))
  		{
  			trim_directory(path);
! 			trim_directory(path);	/* remove directory above */
  		}
  		else
  			break;
--- 278,302 ----

  	/*
  	 * Remove any trailing uses of "." and process ".." ourselves
+ 	 *
+ 	 * This is tricky because of the possibility of /foo/bar/baz/../..
  	 */
+ 	pending_strips = 0;
  	for (;;)
  	{
  		int			len = strlen(path);

if (len > 2 && strcmp(path + len - 2, "/.") == 0)
trim_directory(path);
! else if (len > 3 && strcmp(path + len - 3, "/..") == 0)
{
trim_directory(path);
! pending_strips++; /* must remove directory above */
! }
! else if (pending_strips > 0)
! {
! trim_directory(path);
! pending_strips--;
}
else
break;

Bruce Momjian <pgman@candle.pha.pa.us> writes:

And then you have this case:

/usr/local/../bin/../..

AFAICS, the patch I just proposed handles this.

If I recall the code properly, we do not have to make canonicalize_path
remove embedded "." or ".." --- that is, we do not have to simplify

/usr/local/../bin

But we do have to get rid of every trailing "." or "..", else there are
failure cases when we replace the trailing component with an ordinary
file name.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

And then you have this case:

/usr/local/../bin/../..

AFAICS, the patch I just proposed handles this.

If I recall the code properly, we do not have to make canonicalize_path
remove embedded "." or ".." --- that is, we do not have to simplify

/usr/local/../bin

But we do have to get rid of every trailing "." or "..", else there are
failure cases when we replace the trailing component with an ordinary
file name.

But what about "usr/local/../../.."? You loop is similar to what I
coded, but then when I realized I had to check for pending trims after I
run out of directories, and start appending them to create things like
../.., I gave up.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

But what about "usr/local/../../.."?

What about it? The case of /usr/local/../../.. is handled correctly,
and the case where it's an underspecified relative path doesn't seem
that interesting to me --- certainly that is not so important that we
should get the wrong answer on cases that *are* plausible.

Most of the uses of canonicalize_path are on paths that are required to
be absolute, anyway.

It wouldn't be too implausible to error out if pending_strips>0 after
exiting the loop.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

But what about "usr/local/../../.."?

What about it? The case of /usr/local/../../.. is handled correctly,
and the case where it's an underspecified relative path doesn't seem
that interesting to me --- certainly that is not so important that we
should get the wrong answer on cases that *are* plausible.

Most of the uses of canonicalize_path are on paths that are required to
be absolute, anyway.

It wouldn't be too implausible to error out if pending_strips>0 after
exiting the loop.

I figured it would be best to leave it alone if we can't process it, but
if you think it is more imporant to trim in cases like ../.., go ahead.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

I have test the following on Windows and Linux:

Windows:
C:\>cd Winnt

C:\WINNT>cd C:\Temp\..\..\

C:\>

Linux:
$ cd /usr/../../
$ pwd
/

We should handle this correctly.

1 Single dot in the path can be removed safety. (except the first one. e.g. ./usr/local)
2 Every double dot may need a removal of the last part of the path. (except the first one. e.g. ../local)
And if there are not enough part left, keep the last part as it is.

We can even make it easier by adding step 0: make sure path is an absolute path.

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>
To: "Tom Lane" <tgl@sss.pgh.pa.us>
Cc: "William ZHANG" <uniware@zedware.org>; <pgsql-patches@postgresql.org>
Sent: Friday, August 12, 2005 11:15 AM
Subject: Re: [PATCHES] Bug in canonicalize_path()

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I figured it would be best to leave it alone if we can't process it, but
if you think it is more imporant to trim in cases like ../.., go ahead.

My recollection of this patch:

2004-08-09 16:20 tgl

* src/: port/exec.c, port/path.c, bin/initdb/initdb.c:
Path-mangling logic was failing to account for paths containing
mentions of '.' or '..'. Extend canonicalize_path() to trim off
trailing occurrences of these things, and use it to fix up paths
where needed (which I think is only after places where we trim the
last path component, but maybe some others will turn up). Fixes
Josh's complaint that './initdb' does not work.

is that it's part of the API contract of canonicalize_path() that it
will not return something with trailing "." or "..". It's not just
neatnik-ism to strip those, it's necessary to allow higher-level
path-mangling routines to reason about how to do what they need.
The entire concept of "trim the last directory" fails if you have
to account for "." or "..".

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I figured it would be best to leave it alone if we can't process it, but
if you think it is more imporant to trim in cases like ../.., go ahead.

My recollection of this patch:

2004-08-09 16:20 tgl

* src/: port/exec.c, port/path.c, bin/initdb/initdb.c:
Path-mangling logic was failing to account for paths containing
mentions of '.' or '..'. Extend canonicalize_path() to trim off
trailing occurrences of these things, and use it to fix up paths
where needed (which I think is only after places where we trim the
last path component, but maybe some others will turn up). Fixes
Josh's complaint that './initdb' does not work.

is that it's part of the API contract of canonicalize_path() that it
will not return something with trailing "." or "..". It's not just
neatnik-ism to strip those, it's necessary to allow higher-level
path-mangling routines to reason about how to do what they need.
The entire concept of "trim the last directory" fails if you have
to account for "." or "..".

OK, new patch which I think handles all cases.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

... it's part of the API contract of canonicalize_path() that it
will not return something with trailing "." or "..".

OK, new patch which I think handles all cases.

+ 	if (pending_strips > 0)
+ 	{
+ 		for (; pending_strips > 0; pending_strips--)
+ 			strcat(path, "../");
+ 		trim_trailing_separator(path);
+ 	}

Uh, that hardly meets the API contract that I mentioned. I think
we really have to throw an error if the path tries to ".." above
the starting point. (Remember again that most of the uses of
this thing are dealing with absolute paths anyway, so this isn't
that big a deal.)

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

... it's part of the API contract of canonicalize_path() that it
will not return something with trailing "." or "..".

OK, new patch which I think handles all cases.

+ 	if (pending_strips > 0)
+ 	{
+ 		for (; pending_strips > 0; pending_strips--)
+ 			strcat(path, "../");
+ 		trim_trailing_separator(path);
+ 	}

Uh, that hardly meets the API contract that I mentioned. I think
we really have to throw an error if the path tries to ".." above
the starting point. (Remember again that most of the uses of
this thing are dealing with absolute paths anyway, so this isn't
that big a deal.)

OK, so how do you want to error out? exit()? There are no ereport
calls in that file. We can add them (using a *_srv.c file) or let it
return a boolean and check it at each call site.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Uh, that hardly meets the API contract that I mentioned. I think
we really have to throw an error if the path tries to ".." above
the starting point.

OK, so how do you want to error out? exit()? There are no ereport
calls in that file.

Yeah, we'll have to #ifdef FRONTEND it ... tedious ...

regards, tom lane

I wrote:

Uh, that hardly meets the API contract that I mentioned. I think
we really have to throw an error if the path tries to ".." above
the starting point.

After rereading all the callers of canonicalize_path, I've concluded
that none of them actually depend on not having a terminating ".."
as I thought. There is a risk factor, which is that a lot of places
blindly trim the last component of a path --- but AFAICS, this is only
done with paths that are known to represent the name of a program,
so the last component wouldn't be ".." anyway.

So your last version of the patch seems like the way to go. I'll
apply it along with changing path.c to support the parent-directory
test better.

regards, tom lane

Tom Lane wrote:

I wrote:

Uh, that hardly meets the API contract that I mentioned. I think
we really have to throw an error if the path tries to ".." above
the starting point.

After rereading all the callers of canonicalize_path, I've concluded
that none of them actually depend on not having a terminating ".."
as I thought. There is a risk factor, which is that a lot of places
blindly trim the last component of a path --- but AFAICS, this is only
done with paths that are known to represent the name of a program,
so the last component wouldn't be ".." anyway.

So your last version of the patch seems like the way to go. I'll
apply it along with changing path.c to support the parent-directory
test better.

OK, here is a version that errors out that I just applied and reversed
out when I saw your email. Feel free to use it or discard it.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

OK, here is a version that errors out that I just applied and reversed
out when I saw your email. Feel free to use it or discard it.

Sorry about that --- should have tried to mail you a bit sooner.

BTW, what's with the postmaster.c change? Seems mistaken.

regards, tom lane