From 16b158756a8e51279604bf9f8995b9392052b274 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi Date: Wed, 24 Apr 2019 11:50:41 +0900 Subject: [PATCH 1/5] Allow relative tablespace location paths Currently relative paths are not allowed as tablespace directory. That makes tests about tablespaces bothersome but doens'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 | 76 ++++++++--- src/backend/replication/basebackup.c | 7 ++ src/backend/utils/adt/misc.c | 7 ++ src/bin/pg_basebackup/pg_basebackup.c | 180 +++++++++++++++++++++++---- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 17 ++- src/test/regress/input/tablespace.source | 3 + src/test/regress/output/tablespace.source | 5 +- 8 files changed, 258 insertions(+), 45 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..66a70871e6 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -94,6 +94,42 @@ 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 cwd[MAXPGPATH]; + char abspath[MAXPGPATH]; + char absdatadir[MAXPGPATH]; + + getcwd(cwd, MAXPGPATH); + if (chdir(path) < 0) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid directory \"%s\": %m", path))); + + /* getcwd is defined as returning absolute path */ + getcwd(abspath, MAXPGPATH); + + /* DataDir needs to be canonicalized */ + if (chdir(DataDir)) + ereport(FATAL, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("could not chdir to the data directory \"%s\": %m", + DataDir))); + getcwd(absdatadir, MAXPGPATH); + + /* this must succeed */ + if (chdir(cwd)) + ereport(FATAL, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("could not chdir to the current working directory \"%s\": %m", + cwd))); + + return path_is_prefix_of_path(absdatadir, abspath); +} /* * Each database using a table space is isolated into its own name space @@ -267,35 +303,26 @@ CreateTableSpace(CreateTableSpaceStmt *stmt) (errcode(ERRCODE_INVALID_NAME), errmsg("tablespace location cannot contain single quotes"))); - /* - * Allowing relative paths seems risky - * - * 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//_.'. FYI, we never actually + * 'PG_XXX//_.'. 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,7 +597,9 @@ static void create_tablespace_directories(const char *location, const Oid tablespaceoid) { char *linkloc; + char *linktarget; char *location_with_version_dir; + bool free_linktarget = false; struct stat st; linkloc = psprintf("pg_tblspc/%u", tablespaceoid); @@ -639,13 +668,28 @@ 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..82ff4703d9 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -1238,6 +1238,13 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces, pathbuf))); linkpath[rllen] = '\0'; + /* + * Relative link target is always prefixed by "../". We need to + * remove it to obtain a relative tablespace directory. + */ + if (strncmp(linkpath, "../", 3) == 0) + memmove(linkpath, linkpath + 3, strlen(linkpath) + 1 - 3); + size += _tarWriteHeader(pathbuf + basepathlen + 1, linkpath, &statbuf, sizeonly); #else diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c index d330a88e3c..59f987e5ae 100644 --- a/src/backend/utils/adt/misc.c +++ b/src/backend/utils/adt/misc.c @@ -330,6 +330,13 @@ pg_tablespace_location(PG_FUNCTION_ARGS) sourcepath))); targetpath[rllen] = '\0'; + /* + * Relative target paths are always prefixed by "../". We need to remove + * it to obtain tablespace directory. + */ + if (strncmp(targetpath, "../", 3) == 0) + PG_RETURN_TEXT_P(cstring_to_text(targetpath + 3)); + PG_RETURN_TEXT_P(cstring_to_text(targetpath)); #else ereport(ERROR, diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index 1a735b8046..0cd7a89c56 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,34 @@ 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. + */ + 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 +1967,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 +2193,104 @@ BaseBackup(void) pg_log_info("base backup completed"); } +/* functions to avoid duplicate error handling */ +static void +simple_chdir(const char *path) +{ + if (chdir(path) < 0) + { + pg_log_error("chdir failed to \"%s\": %m", path); + exit(1); + } +} + +static void +simple_getcwd(char *buf, int len) +{ + if (getcwd(buf, len) < 0) + { + pg_log_error("getcwd failed: %m"); + exit(1); + } +} + +/* Sanity check of tablespace mappings */ +static void +check_tablespace_mappings(void) +{ + TablespaceListCell *cell; + char cwd[MAXPGPATH]; + char canonbase[MAXPGPATH]; + + simple_getcwd(cwd, MAXPGPATH); + + /* canonicalize basedir */ + simple_chdir(basedir); + simple_getcwd(canonbase, MAXPGPATH); + + for (cell = tablespace_dirs.head ; cell ; cell = cell->next) + { + char path[MAXPGPATH]; + + if (is_absolute_path(cell->new_dir)) + strlcpy(path, cell->new_dir, MAXPGPATH); + else + { + /* construct absolute path of relative new_dir */ + path[0] = 0; + if (!is_absolute_path(basedir)) + { + strlcpy(path, cwd, MAXPGPATH); + if (path[strlen(path) - 1] != '/') + strlcat(path, "/", MAXPGPATH); + } + + strlcat(path, basedir, MAXPGPATH); + if (path[strlen(path) - 1] != '/') + strlcat(path, "/", MAXPGPATH); + strlcat(path, cell->new_dir, MAXPGPATH); + } + + /* strip off trailing separators */ + while (path[strlen(path) - 1] == '/') + path[strlen(path) - 1] = 0; + + /* + * We allow new_dir is a nonexistent path. Even in that case, the + * parent directory must be exist. If chdir failed, go up one step + * then recheck then repeat. + */ + while (chdir(path) < 0) + { + /* only one step up is needed */ + if (errno == ENOENT) + { + /* Find the last separator */ + char *p = strrchr(path, '/'); + + /* We cannot go up further */ + if (p == NULL || p == path) + break; + + *p = 0; + continue; + } + + pg_log_error("invalid new_dir \"%s\" of mapping for \"%s\": %m", + cell->new_dir, cell->old_dir); + exit(1); + } + + simple_getcwd(path, MAXPGPATH); + if (strncmp(canonbase, path, strlen(canonbase)) == 0) + { + pg_log_error("new_dir \"%s\" of mapping for \"%s\" is inside target data directory \"%s\"", cell->new_dir, cell->old_dir, canonbase); + exit(1); + } + } + + simple_chdir(cwd); +} int main(int argc, char **argv) @@ -2513,6 +2641,12 @@ main(int argc, char **argv) if (format == 'p' || strcmp(basedir, "-") != 0) verify_dir_is_empty_or_create(basedir, &made_new_pgdata, &found_existing_pgdata); + /* + * Sanity check of tablespace mapping. This can be perform after creating + * basedir. + */ + check_tablespace_mappings(); + /* determine remote server's xlog segment size */ if (!RetrieveWalSegSize(conn)) exit(1); diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 33869fecc9..bd1a92df36 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,19 @@ $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 old directory not absolute succeeds'); +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=$tempdir/backup_foo/bar" ], + '-T with new absolute directory inside data dir fails'); $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', "-T/foo=/foo/bar" ], + '-T with new directory under nonexistent directory fails'); $node->command_fails( [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-Tfoo" ], '-T with invalid format fails'); diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source index 14ce0e7e04..cf596d607f 100644 --- a/src/test/regress/input/tablespace.source +++ b/src/test/regress/input/tablespace.source @@ -125,6 +125,9 @@ SELECT COUNT(*) FROM testschema.atable; -- checks heap -- Will fail with bad path CREATE TABLESPACE regress_badspace LOCATION '/no/such/location'; +-- Will fail with data directory +CREATE TABLESPACE regress_badspace LOCATION '.'; + -- No such tablespace CREATE TABLE bar (i int) TABLESPACE regress_nosuchspace; diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source index 8ebe08b9b2..c404d599f0 100644 --- a/src/test/regress/output/tablespace.source +++ b/src/test/regress/output/tablespace.source @@ -257,7 +257,10 @@ SELECT COUNT(*) FROM testschema.atable; -- checks heap -- Will fail with bad path CREATE TABLESPACE regress_badspace LOCATION '/no/such/location'; -ERROR: directory "/no/such/location" does not exist +ERROR: invalid directory "/no/such/location": No such file or directory +-- Will fail with data directory +CREATE TABLESPACE regress_badspace LOCATION '.'; +ERROR: tablespace location must not be inside the data directory -- No such tablespace CREATE TABLE bar (i int) TABLESPACE regress_nosuchspace; ERROR: tablespace "regress_nosuchspace" does not exist -- 2.16.3