Dead code in adminpack
I've noticed that convert_and_check_filename() is always passed false for the
"logAllowed" argument - someone probably forgot to remove the argument when it
was decided that log files are no longer accepted. If the argument was removed,
the function would become a bit simpler, see the patch.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
Attachments:
adminpack_dead_code.patchtext/x-diffDownload
diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c
index 29b46aea3e..f33cc6e532 100644
--- a/contrib/adminpack/adminpack.c
+++ b/contrib/adminpack/adminpack.c
@@ -69,10 +69,10 @@ typedef struct
* Convert a "text" filename argument to C string, and check it's allowable.
*
* Filename may be absolute or relative to the DataDir, but we only allow
- * absolute paths that match DataDir or Log_directory.
+ * absolute paths that match DataDir.
*/
static char *
-convert_and_check_filename(text *arg, bool logAllowed)
+convert_and_check_filename(text *arg)
{
char *filename = text_to_cstring(arg);
@@ -99,9 +99,7 @@ convert_and_check_filename(text *arg, bool logAllowed)
* Allow absolute paths if within DataDir or Log_directory, even
* though Log_directory might be outside DataDir.
*/
- if (!path_is_prefix_of_path(DataDir, filename) &&
- (!logAllowed || !is_absolute_path(Log_directory) ||
- !path_is_prefix_of_path(Log_directory, filename)))
+ if (!path_is_prefix_of_path(DataDir, filename))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("absolute path not allowed")));
@@ -185,7 +183,7 @@ pg_file_write_internal(text *file, text *data, bool replace)
char *filename;
int64 count = 0;
- filename = convert_and_check_filename(file, false);
+ filename = convert_and_check_filename(file);
if (!replace)
{
@@ -228,7 +226,7 @@ pg_file_sync(PG_FUNCTION_ARGS)
char *filename;
struct stat fst;
- filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0), false);
+ filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0));
if (stat(filename, &fst) < 0)
ereport(ERROR,
@@ -319,13 +317,13 @@ pg_file_rename_internal(text *file1, text *file2, text *file3)
*fn3;
int rc;
- fn1 = convert_and_check_filename(file1, false);
- fn2 = convert_and_check_filename(file2, false);
+ fn1 = convert_and_check_filename(file1);
+ fn2 = convert_and_check_filename(file2);
if (file3 == NULL)
fn3 = NULL;
else
- fn3 = convert_and_check_filename(file3, false);
+ fn3 = convert_and_check_filename(file3);
if (access(fn1, W_OK) < 0)
{
@@ -411,7 +409,7 @@ pg_file_unlink(PG_FUNCTION_ARGS)
requireSuperuser();
- filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0), false);
+ filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0));
if (access(filename, W_OK) < 0)
{
@@ -449,7 +447,7 @@ pg_file_unlink_v1_1(PG_FUNCTION_ARGS)
{
char *filename;
- filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0), false);
+ filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0));
if (access(filename, W_OK) < 0)
{
On Thu, Feb 13, 2020 at 12:14 PM Antonin Houska <ah@cybertec.at> wrote:
I've noticed that convert_and_check_filename() is always passed false for the
"logAllowed" argument - someone probably forgot to remove the argument when it
was decided that log files are no longer accepted. If the argument was removed,
the function would become a bit simpler, see the patch.
Indeed, and actually I don't see when this codepath was reachable.
Patch LGTM.
On Thu, Feb 13, 2020 at 12:15:39PM +0100, Antonin Houska wrote:
I've noticed that convert_and_check_filename() is always passed false for the
"logAllowed" argument - someone probably forgot to remove the argument when it
was decided that log files are no longer accepted. If the argument was removed,
the function would become a bit simpler, see the patch.
This routine was originally named absClusterPath(), but even at its
origin point (fe59e56) this argument has never been used. So no
objections to clean up that.
--
Michael
On Thu, Feb 13, 2020 at 12:41:46PM +0100, Julien Rouhaud wrote:
On Thu, Feb 13, 2020 at 12:14 PM Antonin Houska <ah@cybertec.at> wrote:
I've noticed that convert_and_check_filename() is always passed false for the
"logAllowed" argument - someone probably forgot to remove the argument when it
was decided that log files are no longer accepted. If the argument was removed,
the function would become a bit simpler, see the patch.Indeed, and actually I don't see when this codepath was reachable.
Patch LGTM.
Thanks, applied.
--
Michael