Thread: Bug in canonicalize_path()
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 */
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
>
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 didn't much like the patch as-is; we should think a little harder > about fixing the logic properly. > > regards, tom lane
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()
> 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
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 Index: src/port/path.c =================================================================== RCS file: /cvsroot/pgsql/src/port/path.c,v retrieving revision 1.54 diff -c -c -r1.54 path.c *** src/port/path.c 12 Aug 2005 03:07:45 -0000 1.54 --- src/port/path.c 12 Aug 2005 04:47:59 -0000 *************** *** 226,231 **** --- 226,232 ---- { char *p, *to_p; bool was_sep = false; + int pending_strips = 0; #ifdef WIN32 /* *************** *** 284,306 **** if (len > 2 && strcmp(path + len - 2, "/.") == 0) trim_directory(path); ! /* ! * Process only a single trailing "..", and only if ".." does ! * not preceed it. ! * So, we only deal with "/usr/local/..", not with "/usr/local/../..". ! * We don't handle the even more complex cases, like ! * "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; } } --- 285,311 ---- 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++; ! } ! /* for absolute paths, we just keep trimming */ ! else if (*path != '\0' && pending_strips > 0) ! { ! trim_directory(path); ! pending_strips--; } else break; } + + if (pending_strips > 0) + { + for (; pending_strips > 0; pending_strips--) + strcat(path, "../"); + trim_trailing_separator(path); + } }
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 Index: src/backend/postmaster/postmaster.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v retrieving revision 1.464 diff -c -c -r1.464 postmaster.c *** src/backend/postmaster/postmaster.c 12 Aug 2005 18:23:53 -0000 1.464 --- src/backend/postmaster/postmaster.c 12 Aug 2005 19:40:44 -0000 *************** *** 377,384 **** char *userDoption = NULL; int i; ! /* This will call exit() if strdup() fails. */ ! progname = get_progname(argv[0]); MyProcPid = PostmasterPid = getpid(); --- 377,387 ---- char *userDoption = NULL; int i; ! if ((progname = get_progname(argv[0])) == NULL) ! { ! printf(_("unable to allocate memory for program name \"%s\".\n"), progname); ! ExitPostmaster(0); ! } MyProcPid = PostmasterPid = getpid(); Index: src/port/Makefile =================================================================== RCS file: /cvsroot/pgsql/src/port/Makefile,v retrieving revision 1.25 diff -c -c -r1.25 Makefile *** src/port/Makefile 20 Mar 2005 03:53:39 -0000 1.25 --- src/port/Makefile 12 Aug 2005 19:40:51 -0000 *************** *** 31,36 **** --- 31,37 ---- LIBOBJS_SRV := $(patsubst dirmod.o,dirmod_srv.o, $(LIBOBJS_SRV)) LIBOBJS_SRV := $(patsubst exec.o,exec_srv.o, $(LIBOBJS_SRV)) LIBOBJS_SRV := $(patsubst getaddrinfo.o,getaddrinfo_srv.o, $(LIBOBJS_SRV)) + LIBOBJS_SRV := $(patsubst path.o,path_srv.o, $(LIBOBJS_SRV)) LIBOBJS_SRV := $(patsubst thread.o,thread_srv.o, $(LIBOBJS_SRV)) all: libpgport.a libpgport_srv.a *************** *** 66,72 **** getaddrinfo_srv.o: getaddrinfo.c $(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@ ! snprintf_srv.o: snprintf.c $(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@ # No thread flags for server version --- 67,73 ---- getaddrinfo_srv.o: getaddrinfo.c $(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@ ! path_srv.o: path.c $(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@ # No thread flags for server version Index: src/port/path.c =================================================================== RCS file: /cvsroot/pgsql/src/port/path.c,v retrieving revision 1.54 diff -c -c -r1.54 path.c *** src/port/path.c 12 Aug 2005 03:07:45 -0000 1.54 --- src/port/path.c 12 Aug 2005 19:40:53 -0000 *************** *** 13,19 **** *------------------------------------------------------------------------- */ ! #include "c.h" #include <ctype.h> #include <sys/stat.h> --- 13,23 ---- *------------------------------------------------------------------------- */ ! #ifndef FRONTEND ! #include "postgres.h" ! #else ! #include "postgres_fe.h" ! #endif #include <ctype.h> #include <sys/stat.h> *************** *** 226,231 **** --- 230,236 ---- { char *p, *to_p; bool was_sep = false; + int pending_strips = 0; #ifdef WIN32 /* *************** *** 284,302 **** if (len > 2 && strcmp(path + len - 2, "/.") == 0) trim_directory(path); ! /* ! * Process only a single trailing "..", and only if ".." does ! * not preceed it. ! * So, we only deal with "/usr/local/..", not with "/usr/local/../..". ! * We don't handle the even more complex cases, like ! * "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; --- 289,326 ---- 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++; ! } ! else if (pending_strips > 0) ! { ! /* If path is not "", we can keep trimming. Even if path is ! * "/", we can keep trimming because trim_directory never removes ! * the leading separator, and the parent directory of "/" is "/". ! */ ! if (*path != '\0') ! { ! trim_directory(path); ! pending_strips--; ! } ! else ! { ! /* ! * If we still have pending_strips, it means the supplied path ! * was exhausted and we still have more directories to move up. ! * This means that the resulting path is only parents, like ! * ".." or "../..". If so, callers can not handle trailing "..", ! * so we exit. ! */ ! #ifndef FRONTEND ! elog(ERROR, "relative paths (\"..\") not supported"); ! #else ! fprintf(stderr, _("relative paths (\"..\") not supported\n")); ! exit(1); ! #endif ! } } else break; *************** *** 305,312 **** /* ! * Extracts the actual name of the program as called - ! * stripped of .exe suffix if any */ const char * get_progname(const char *argv0) --- 329,338 ---- /* ! * Extracts the actual name of the program as called - ! * stripped of .exe suffix if any. ! * The server calling this must check for NULL return ! * and report the error. */ const char * get_progname(const char *argv0) *************** *** 329,336 **** progname = strdup(nodir_name); if (progname == NULL) { fprintf(stderr, "%s: out of memory\n", nodir_name); ! exit(1); /* This could exit the postmaster */ } progname[strlen(progname) - (sizeof(EXE) - 1)] = '\0'; nodir_name = progname; --- 355,370 ---- progname = strdup(nodir_name); if (progname == NULL) { + #ifndef FRONTEND + /* + * No elog() support in postmaster at this stage, + * so return NULL and print error at the call. + */ + return NULL; + #else fprintf(stderr, "%s: out of memory\n", nodir_name); ! exit(1); ! #endif } progname[strlen(progname) - (sizeof(EXE) - 1)] = '\0'; nodir_name = progname;
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
Tom Lane wrote: > 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. Actually, it is a cleanup. The original path.c::get_progname() used an exit() call in WIN32 because it didn't have frontend/backend-specific code. With specific versions, I could return a NULL from get_progname and print a relivant message in postmaster.c on failure. I doubt it would ever happen, but it was a cleanup. It isn't significant enough to justify making a server-specific version of path.c on its own, however. -- 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:
> 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.
BTW, you were right: this is a *lot* harder than it looks at first
glance. Here's what I ended up with:
/*
* Remove any trailing uses of "." and process ".." ourselves
*
* Note that "/../.." should reduce to just "/", while "../.." has to
* be kept as-is. In the latter case we put back mistakenly trimmed
* ".." components below. Also note that we want a Windows drive spec
* to be visible to trim_directory(), but it's not part of the logic
* that's looking at the name components; hence distinction between
* path and spath.
*/
spath = skip_drive(path);
pending_strips = 0;
for (;;)
{
int len = strlen(spath);
if (len >= 2 && strcmp(spath + len - 2, "/.") == 0)
trim_directory(path);
else if (strcmp(spath, ".") == 0)
{
/* Want to leave "." alone, but "./.." has to become ".." */
if (pending_strips > 0)
*spath = '\0';
break;
}
else if ((len >= 3 && strcmp(spath + len - 3, "/..") == 0) ||
strcmp(spath, "..") == 0)
{
trim_directory(path);
pending_strips++;
}
else if (pending_strips > 0 && *spath != '\0')
{
/* trim a regular directory name cancelled by ".." */
trim_directory(path);
pending_strips--;
/* foo/.. should become ".", not empty */
if (*spath == '\0')
strcpy(spath, ".");
}
else
break;
}
if (pending_strips > 0)
{
/*
* We could only get here if path is now totally empty (other than
* a possible drive specifier on Windows).
* We have to put back one or more ".."'s that we took off.
*/
while (--pending_strips > 0)
strcat(path, "../");
strcat(path, "..");
}
}
regards, tom lane