Re: Regression test PANICs with master-standby setup on samemachine - Mailing list pgsql-hackers

At Wed, 24 Apr 2019 09:30:12 -0700, Andres Freund <andres@anarazel.de> wrote in
<20190424163012.7wzdl6j2v73cufip@alap3.anarazel.de>
> Hi,
> 
> On 2019-04-24 17:02:28 +0900, Kyotaro HORIGUCHI wrote:
> > +/*
> > + * Check if the path is in the data directory strictly.
> > + */
> > +static bool
> > +is_in_data_directory(const char *path)
> > +{
<hinde the ugly thing:p>
> > +                 errmsg("could not chdir to the current working directory \"%s\": %m",
> > +                     cwd)));
> > +
> > +    return path_is_prefix_of_path(absdatadir, abspath);
> > +}
> 
> This seems like a bad idea to me. Why don't we just use
> make_absolute_path() on the proposed tablespace path, and then check
> path_is_prefix_of() or such? Sure, that can be tricked using symlinks
> etc, but that's already the case.

Thanks for the suggestions, Tom, Andres. For clarity, as I
mentioned in the post, I didn't like the getcwd in 0001. The
reason for the previous patch are:

1. canonicalize_path doesn't process '/.' and '/..' in the middle
  of a path. That prevents correct checking of directory
  inclusiveness. Actually regression test suffers that.

2. Simply I missed make_absolute_path..

So, I rewote canonicalize_path to process '.' and '..'s not only
at the end of a path but all occurrances in a path. This makes
the strange loop of chdir-getwen useless. But the new
canonicalize_path is a bit complex.

The modified canonicalize_path works filesystem-access-free so it
doesn't follow symlinks. Thus it makes a misjudge when two paths
are in an inclusion relationship after resolving symlinks in the
paths. But I don't think we don't need to treat such a malicious
situation.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From b5d620940c70d59b70617e129eab72d54f010384 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 25 Apr 2019 15:08:54 +0900
Subject: [PATCH] Allow relative tablespace location paths

Currently relative paths are not allowed as tablespace directory. That
makes tests about tablespaces bothersome but doesn't prevent them
points into the data directory perfectly. This patch allows relative
direcotry based on the data directory as tablespace directory. Then
makes the check more strict.
---
 src/backend/access/transam/xlog.c            |   8 ++
 src/backend/commands/tablespace.c            |  55 ++++++--
 src/backend/replication/basebackup.c         |  11 +-
 src/backend/utils/adt/misc.c                 |  10 +-
 src/bin/pg_basebackup/pg_basebackup.c        | 113 ++++++++++++---
 src/bin/pg_basebackup/t/010_pg_basebackup.pl |  18 ++-
 src/port/path.c                              | 204 +++++++++++++++++----------
 7 files changed, 300 insertions(+), 119 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c00b63c751..abc2fd951f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10419,6 +10419,14 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
             }
             linkpath[rllen] = '\0';
 
+            /*
+             * In the relative path cases, the link target is always prefixed
+             * by "../" to convert from data directory-based. So we just do
+             * the reverse here.
+             */
+            if (strncmp(s, "../", 3) == 0)
+                s += 3;
+
             /*
              * Add the escape character '\\' before newline in a string to
              * ensure that we can distinguish between the newline in the
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 3784ea4b4f..98d9e65629 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -94,6 +94,25 @@ static void create_tablespace_directories(const char *location,
                               const Oid tablespaceoid);
 static bool destroy_tablespace_directories(Oid tablespaceoid, bool redo);
 
+/*
+ * Check if the path is in the data directory strictly.
+ */
+static bool
+is_in_data_directory(const char *path)
+{
+    char   *tmp_path = make_absolute_path(path);
+    char    tmp_datadir[MAXPGPATH];
+    bool    in_datadir;
+
+    Assert (tmp_path != NULL);
+    strlcpy(tmp_datadir, DataDir, MAXPGPATH);
+    canonicalize_path(tmp_datadir);
+
+    in_datadir = path_is_prefix_of_path(tmp_datadir, tmp_path);
+    free(tmp_path);
+
+    return in_datadir;
+}
 
 /*
  * Each database using a table space is isolated into its own name space
@@ -272,30 +291,26 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
      *
      * this also helps us ensure that location is not empty or whitespace
      */
-    if (!is_absolute_path(location))
+    /* Reject tablespaces in the data directory. */
+    if (is_in_data_directory(location))
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-                 errmsg("tablespace location must be an absolute path")));
+                 errmsg("tablespace location must not be inside the data directory")));
 
     /*
      * Check that location isn't too long. Remember that we're going to append
-     * 'PG_XXX/<dboid>/<relid>_<fork>.<nnn>'.  FYI, we never actually
+     * 'PG_XXX/<dboid>/<relid>_<fork>.<nnn>'.  In the relative path case, we
+     * attach "../" at the beginning of the path. FYI, we never actually
      * reference the whole path here, but MakePGDirectory() uses the first two
      * parts.
      */
-    if (strlen(location) + 1 + strlen(TABLESPACE_VERSION_DIRECTORY) + 1 +
+    if (3 + strlen(location) + 1 + strlen(TABLESPACE_VERSION_DIRECTORY) + 1 +
         OIDCHARS + 1 + OIDCHARS + 1 + FORKNAMECHARS + 1 + OIDCHARS > MAXPGPATH)
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                  errmsg("tablespace location \"%s\" is too long",
                         location)));
 
-    /* Warn if the tablespace is in the data directory. */
-    if (path_is_prefix_of_path(DataDir, location))
-        ereport(WARNING,
-                (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-                 errmsg("tablespace location should not be inside the data directory")));
-
     /*
      * Disallow creation of tablespaces named "pg_xxx"; we reserve this
      * namespace for system purposes.
@@ -570,8 +585,10 @@ static void
 create_tablespace_directories(const char *location, const Oid tablespaceoid)
 {
     char       *linkloc;
+    char       *linktarget;
     char       *location_with_version_dir;
-    struct stat st;
+    bool        free_linktarget = false;
+     struct stat st;
 
     linkloc = psprintf("pg_tblspc/%u", tablespaceoid);
     location_with_version_dir = psprintf("%s/%s", location,
@@ -639,13 +656,27 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
 
     /*
      * Create the symlink under PGDATA
+     * Relative tablespace location is based on PGDATA directory. The symlink
+     * is created in PGDATA/pg_tblspc. So adjust relative paths by attaching
+     * "../" at the head.
      */
-    if (symlink(location, linkloc) < 0)
+    if (is_absolute_path(location))
+        linktarget = unconstify(char *, location);
+    else
+    {
+        linktarget = psprintf("../%s", location);
+        free_linktarget = true;
+    }
+
+    if (symlink(linktarget, linkloc) < 0)
         ereport(ERROR,
                 (errcode_for_file_access(),
                  errmsg("could not create symbolic link \"%s\": %m",
                         linkloc)));
 
+    if (free_linktarget)
+        pfree(linktarget);
+
     pfree(linkloc);
     pfree(location_with_version_dir);
 }
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 36dcb28754..98e34e21c1 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -1224,6 +1224,7 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
 #if defined(HAVE_READLINK) || defined(WIN32)
             char        linkpath[MAXPGPATH];
             int            rllen;
+            int            linkoffset = 0;
 
             rllen = readlink(pathbuf, linkpath, sizeof(linkpath));
             if (rllen < 0)
@@ -1238,7 +1239,15 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
                                 pathbuf)));
             linkpath[rllen] = '\0';
 
-            size += _tarWriteHeader(pathbuf + basepathlen + 1, linkpath,
+            /*
+             * Relative link target is always prefixed by "../". Remove it to
+             * obtain a tablespace directory relative to the data directory.
+             */
+            if (strncmp(linkpath, "../", 3) == 0)
+                linkoffset = 3;
+
+            size += _tarWriteHeader(pathbuf + basepathlen + 1,
+                                    linkpath + linkoffset,
                                     &statbuf, sizeonly);
 #else
 
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index d330a88e3c..205c8ac903 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -293,6 +293,7 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
     char        sourcepath[MAXPGPATH];
     char        targetpath[MAXPGPATH];
     int            rllen;
+    int            pathoffset = 0;
 
     /*
      * It's useful to apply this function to pg_class.reltablespace, wherein
@@ -330,7 +331,14 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
                         sourcepath)));
     targetpath[rllen] = '\0';
 
-    PG_RETURN_TEXT_P(cstring_to_text(targetpath));
+    /*
+     * Relative target paths are always prefixed by "../". Remove it to obtain
+     * tablespace directory relative to the data directory.
+     */
+    if (strncmp(targetpath, "../", 3) == 0)
+        pathoffset = 3;
+
+    PG_RETURN_TEXT_P(cstring_to_text(targetpath + pathoffset));
 #else
     ereport(ERROR,
             (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 1a735b8046..2a9a10e3f4 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -269,26 +269,6 @@ tablespace_list_append(const char *arg)
         exit(1);
     }
 
-    /*
-     * This check isn't absolutely necessary.  But all tablespaces are created
-     * with absolute directories, so specifying a non-absolute path here would
-     * just never match, possibly confusing users.  It's also good to be
-     * consistent with the new_dir check.
-     */
-    if (!is_absolute_path(cell->old_dir))
-    {
-        pg_log_error("old directory is not an absolute path in tablespace mapping: %s",
-                     cell->old_dir);
-        exit(1);
-    }
-
-    if (!is_absolute_path(cell->new_dir))
-    {
-        pg_log_error("new directory is not an absolute path in tablespace mapping: %s",
-                     cell->new_dir);
-        exit(1);
-    }
-
     /*
      * Comparisons done with these values should involve similarly
      * canonicalized path values.  This is particularly sensitive on Windows
@@ -1399,9 +1379,20 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
     if (basetablespace)
         strlcpy(current_path, basedir, sizeof(current_path));
     else
-        strlcpy(current_path,
-                get_tablespace_mapping(PQgetvalue(res, rownum, 1)),
-                sizeof(current_path));
+    {
+        const char *path = get_tablespace_mapping(PQgetvalue(res, rownum, 1));
+
+        if (is_absolute_path(path))
+            strlcpy(current_path, path, sizeof(current_path));
+        else
+        {
+            /* Relative tablespace locations need to be prefixed by basedir. */
+            strlcpy(current_path, basedir, sizeof(current_path));
+            if (current_path[strlen(current_path) - 1] != '/')
+                strlcat(current_path, "/", sizeof(current_path));
+            strlcat(current_path, path, sizeof(current_path));
+        }
+    }
 
     /*
      * Get the COPY data
@@ -1525,15 +1516,35 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
                      * are, you can call it an undocumented feature that you
                      * can map them too.)
                      */
+                    bool free_path = false;
+
                     filename[strlen(filename) - 1] = '\0';    /* Remove trailing slash */
 
                     mapped_tblspc_path = get_tablespace_mapping(©buf[157]);
+                    if (!is_absolute_path(mapped_tblspc_path))
+                    {
+                        /*
+                         * Convert relative tablespace location based on data
+                         * directory into path link target based on pg_tblspc
+                         * directory. This must be in sync with
+                         * create_tablespace_directories().
+                         */
+                        char *p = malloc(3 + strlen(mapped_tblspc_path) + 1);
+
+                        strcpy(p, "../");
+                        strcat(p, mapped_tblspc_path);
+                        mapped_tblspc_path = p;
+                        free_path = true;
+                    }
                     if (symlink(mapped_tblspc_path, filename) != 0)
                     {
                         pg_log_error("could not create symbolic link from \"%s\" to \"%s\": %m",
                                      filename, mapped_tblspc_path);
                         exit(1);
                     }
+
+                    if (free_path)
+                        free(unconstify(char *, mapped_tblspc_path));
                 }
                 else
                 {
@@ -1957,8 +1968,28 @@ BaseBackup(void)
         if (format == 'p' && !PQgetisnull(res, i, 1))
         {
             char       *path = unconstify(char *, get_tablespace_mapping(PQgetvalue(res, i, 1)));
+            bool        freeit = false;
+
+            if (!is_absolute_path(path))
+            {
+                /*
+                 * Relative tablespace locations need to be converted
+                 * attaching basedir.
+                 */
+                char *p = malloc(strlen(basedir) + 1 + strlen(path) + 1);
+
+                strcpy(p, basedir);
+                if (p[strlen(p) - 1] != '/')
+                    strcat(p, "/");
+                strcat(p, path);
+                path = p;
+                freeit = true;
+            }
 
             verify_dir_is_empty_or_create(path, &made_tablespace_dirs, &found_tablespace_dirs);
+
+            if (freeit)
+                free(path);
         }
     }
 
@@ -2163,6 +2194,39 @@ BaseBackup(void)
         pg_log_info("base backup completed");
 }
 
+/* Sanity check of tablespace mappings */
+static void
+check_tablespace_mappings(void)
+{
+    TablespaceListCell *cell;
+    char *absbasedir = make_absolute_path(basedir);
+
+    for (cell = tablespace_dirs.head ; cell ; cell = cell->next)
+    {
+        char pathbuf[MAXPGPATH];
+        char *path = cell->new_dir;
+
+        /*
+         * absbasedir doesn't have trailing '/'. new_dir is already
+         * canonicalized.
+         */
+        if (!is_absolute_path(cell->new_dir))
+        {
+            /* but concatenated path needs re-canonicalization */
+            snprintf(pathbuf, MAXPGPATH, "%s/%s", absbasedir, cell->new_dir);
+            canonicalize_path(pathbuf);
+            path = pathbuf;
+        }
+
+        if (path_is_prefix_of_path(absbasedir, path))
+        {
+            pg_log_error("new_dir \"%s\" of mapping for \"%s\" is inside target data directory \"%s\"", cell->new_dir,
cell->old_dir,absbasedir);
 
+            exit(1);
+        }
+    }
+
+    free(absbasedir);
+}
 
 int
 main(int argc, char **argv)
@@ -2478,6 +2542,9 @@ main(int argc, char **argv)
         }
     }
 
+    /* Sanity check of tablespace mapping */
+    check_tablespace_mappings();
+
 #ifndef HAVE_LIBZ
     if (compresslevel != 0)
     {
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 33869fecc9..239569a4c3 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -6,7 +6,7 @@ use File::Basename qw(basename dirname);
 use File::Path qw(rmtree);
 use PostgresNode;
 use TestLib;
-use Test::More tests => 106;
+use Test::More tests => 108;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -186,12 +186,20 @@ $node->command_fails(
         "-T/foo=/bar=/baz"
     ],
     '-T with multiple = fails');
+$node->command_ok(
+    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-Tfoo=../bar/baz/foo" ],
+    '-T with relative old directory not rejected');
+rmtree("$tempdir/backup_foo");
 $node->command_fails(
-    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-Tfoo=/bar" ],
-    '-T with old directory not absolute fails');
+    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T/foo=../backup_foo/bar/baz" ],
+    '-T with new relative directory inside data dir fails');
+$node->command_ok(
+    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T/foo=/foo/bar" ],
+    '-T with new directory under nonexistent directory succeeds');
+rmtree("$tempdir/backup_foo");
 $node->command_fails(
-    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T/foo=bar" ],
-    '-T with new directory not absolute fails');
+    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T/foo=../backup_foo/bar/baz" ],
+    '-T with new relative directory inside data dir fails');
 $node->command_fails(
     [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-Tfoo" ],
     '-T with invalid format fails');
diff --git a/src/port/path.c b/src/port/path.c
index 4b214e89e4..8a03ea065b 100644
--- a/src/port/path.c
+++ b/src/port/path.c
@@ -247,19 +247,17 @@ join_path_components(char *ret_path,
  *        o  remove trailing quote on Win32
  *        o  remove trailing slash
  *        o  remove duplicate adjacent separators
- *        o  remove trailing '.'
- *        o  process trailing '..' ourselves
+ *        o  process '.' and '..' ourselves
  */
 void
 canonicalize_path(char *path)
 {
-    char       *p,
-               *to_p;
-    char       *spath;
-    bool        was_sep = false;
-    int            pending_strips;
+    char    *src;
+    char    *dst;
+    int     totallen PG_USED_FOR_ASSERTS_ONLY;
 
 #ifdef WIN32
+    char    *p;
 
     /*
      * The Windows command processor will accept suitably quoted paths with
@@ -277,91 +275,143 @@ canonicalize_path(char *path)
      */
     if (p > path && *(p - 1) == '"')
         *(p - 1) = '/';
+
+    /* Skip drive component or UNC header */
+    path = skip_drive(path);
+#endif
+
+#ifdef USE_ASSERT_CHECKING
+    totallen = strlen(path);
 #endif
 
     /*
-     * Removing the trailing slash on a path means we never get ugly double
-     * trailing slashes. Also, Win32 can't stat() a directory with a trailing
-     * slash. Don't remove a leading slash, though.
-     */
-    trim_trailing_separator(path);
-
-    /*
-     * Remove duplicate adjacent separators
-     */
-    p = path;
-#ifdef WIN32
-    /* Don't remove leading double-slash on Win32 */
-    if (*p)
-        p++;
-#endif
-    to_p = p;
-    for (; *p; p++, to_p++)
-    {
-        /* Handle many adjacent slashes, like "/a///b" */
-        while (*p == '/' && was_sep)
-            p++;
-        if (to_p != p)
-            *to_p = *p;
-        was_sep = (*p == '/');
-    }
-    *to_p = '\0';
-
-    /*
-     * Remove any trailing uses of "." and process ".." ourselves
+     * Process any uses of "." and ".." 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.
+     * kept as-is.
      */
-    spath = skip_drive(path);
-    pending_strips = 0;
-    for (;;)
-    {
-        int            len = strlen(spath);
+    src = dst = path;
 
-        if (len >= 2 && strcmp(spath + len - 2, "/.") == 0)
-            trim_directory(path);
-        else if (strcmp(spath, ".") == 0)
+    while (*src)
+    {
+        char *p;
+        int elen;
+
+        Assert(dst >= path && src >= path &&
+               dst - path <= totallen && src - path <= totallen);
+
+        /* copy separator, removing duplicate adjacent separators */
+        if (IS_DIR_SEP(*src))
         {
-            /* Want to leave "." alone, but "./.." has to become ".." */
-            if (pending_strips > 0)
-                *spath = '\0';
-            break;
+            if (dst > path && IS_DIR_SEP(dst[-1]))
+            {
+                /* skip duplicates */
+                src++;
+                continue;
+            }
+            *dst++ = *src++;
+            continue;
         }
-        else if ((len >= 3 && strcmp(spath + len - 3, "/..") == 0) ||
-                 strcmp(spath, "..") == 0)
+
+        /* locate the next element */
+        for (p = src ; *p && !IS_DIR_SEP(*p) ; p++);
+        elen = p - src;
+
+        Assert (elen != 0);
+
+        /* process "." */
+        if (elen == 1 && src[0] == '.')
         {
-            trim_directory(path);
-            pending_strips++;
+            /* just skip the element and succeeding separators */
+            for (src += elen ; IS_DIR_SEP(*src) ; src++);
+
+            /* If the path was exactly ".", don't skip it */
+            if (*src || src != path + 1)
+                continue;
+
+            src = path;
         }
-        else if (pending_strips > 0 && *spath != '\0')
+        /* process ".." not at the beginning of a relative path */
+        else if (elen == 2 && src[0] == '.' && src[1] == '.' && dst != path)
         {
-            /* trim a regular directory name canceled by ".." */
-            trim_directory(path);
-            pending_strips--;
-            /* foo/.. should become ".", not empty */
-            if (*spath == '\0')
-                strcpy(spath, ".");
+            /*
+             * Found ".." and there's something up-step, step back.
+             * But only when we have the upper element to back up to.
+             * Also don't annihilate two successive ".."s.
+             */
+            if (dst == path + 1)
+            {
+                /*
+                 * dst is absolute root. skip the ".." and trailing separator
+                 * if any
+                 */
+                src += elen;
+                if (IS_DIR_SEP(*src))
+                    src++;
+                continue;
+            }
+            else
+            {
+                /* Find the previous element. dst[-1] == '/' here.  */
+                char *pelm;
+
+                for (pelm = dst - 1 ;
+                     pelm > path && !IS_DIR_SEP(pelm[-1]) ;
+                     pelm--);
+
+                if (dst - pelm == 2 && *pelm == '.')
+                {
+                    /*
+                     * Convert "./.." into ".." then process this ".."
+                     * again.
+                     */
+                    dst = pelm;
+                    continue;
+                }
+                else if (dst - pelm > 3 || strncmp(pelm, "..", 2) != 0)
+                {
+                    /* The previous element is not a relative movement */
+                    if (pelm == path)
+                    {
+                        /*
+                         * The relative path is being translated as
+                         * nothing. Insert "./" instead.
+                         */
+                        memcpy(path, "./", 2);
+                        dst = path + 2;
+                        src += elen;
+                        if (IS_DIR_SEP(*src))
+                            src++;
+                    }
+                    else
+                    {
+                        /* annihilate involving the previous element */
+                        dst = pelm;
+                        src += elen;
+                        if (IS_DIR_SEP(*src))
+                            src++;
+                    }
+                    continue;
+                }
+            }
         }
-        else
-            break;
+        else if ((dst == path + 2 && *path == '.') ||
+                 (dst > path + 2 && IS_DIR_SEP(dst[-3]) && dst[-2] == '.'))
+        {
+            /* the previous element is removable ".", eliminate it */
+            dst -= 2;
+        }
+
+        /* copy it, then move */
+        memcpy(dst, src, elen);
+        dst += elen;
+        src += elen;
     }
 
-    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, "..");
-    }
+    /* trim trailing separators */
+    while (dst > path + 1 && dst[-1] == '/')
+        dst--;
+    *dst = 0;
 }
 
 /*
-- 
2.16.3