Re: Bug in canonicalize_path() - Mailing list pgsql-patches

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;