From 714702a6abfe1987eab98020c799b845917af156 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi Date: Wed, 24 Apr 2019 11:50:41 +0900 Subject: [PATCH 1/4] 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 | 56 +++++++++++++++++++++-- src/test/regress/input/tablespace.source | 3 ++ src/test/regress/output/tablespace.source | 5 +- 7 files changed, 142 insertions(+), 20 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..9fb73bfd8a 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -1399,9 +1399,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 +1536,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 +1987,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); } } 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