From 4e475d1d55966b8057b9808a3439b2ca16fa787e Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Wed, 19 May 2021 21:29:16 +0530 Subject: [PATCH v3] Tighten up batch_size, fetch_size options against non-numeric values It looks like the values such as '100$%$#$#', '9,223,372,' are accepted and treated as valid integers for postgres_fdw options batch_size and fetch_size. Whereas this is not the case with fdw_startup_cost and fdw_tuple_cost options for which an error is thrown. This is because endptr is not used while converting strings to integers using strtol. Use parse_int function instead of strtol as it serves the purpose by returning false in case if it is unable to convert the string to integer. Note that this function also rounds off the values such as '100.456' to 100 and '100.567' or '100.678' to 101. While on this, use parse_real for fdw_startup_cost and fdw_tuple_cost options. Since parse_int and parse_real are being used for reloptions and GUCs, it is more appropriate to use in postgres_fdw rather than using strtol and strtod directly. --- .../postgres_fdw/expected/postgres_fdw.out | 19 ++++++++ contrib/postgres_fdw/option.c | 44 +++++++++++-------- contrib/postgres_fdw/postgres_fdw.c | 16 ++++--- contrib/postgres_fdw/sql/postgres_fdw.sql | 16 +++++++ 4 files changed, 69 insertions(+), 26 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 7df30010f2..ed2b590361 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -10231,3 +10231,22 @@ DROP TABLE result_tbl; DROP TABLE join_tbl; ALTER SERVER loopback OPTIONS (DROP async_capable); ALTER SERVER loopback2 OPTIONS (DROP async_capable); +-- =================================================================== +-- test invalid server and foreign table options +-- =================================================================== +-- Invalid fdw_startup_cost option +CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw + OPTIONS(fdw_startup_cost '100$%$#$#'); +ERROR: invalid numeric value for option "fdw_startup_cost" +-- Invalid fdw_tuple_cost option +CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw + OPTIONS(fdw_tuple_cost '100$%$#$#'); +ERROR: invalid numeric value for option "fdw_tuple_cost" +-- Invalid fetch_size option +CREATE FOREIGN TABLE inv_fsz (c1 int ) + SERVER loopback OPTIONS (fetch_size '100$%$#$#'); +ERROR: invalid integer value for option "fetch_size" +-- Invalid batch_size option +CREATE FOREIGN TABLE inv_bsz (c1 int ) + SERVER loopback OPTIONS (batch_size '100$%$#$#'); +ERROR: invalid integer value for option "batch_size" diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 672b55a808..d39113ca94 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -20,6 +20,7 @@ #include "commands/extension.h" #include "postgres_fdw.h" #include "utils/builtins.h" +#include "utils/guc.h" #include "utils/varlena.h" /* @@ -120,13 +121,20 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) { /* these must have a non-negative numeric value */ double val; - char *endp; + bool is_parsed; - val = strtod(defGetString(def), &endp); - if (*endp || val < 0) + is_parsed = parse_real(defGetString(def), &val, 0, NULL); + + if (!is_parsed) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid numeric value for option \"%s\"", + def->defname))); + + if (val < 0) ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s requires a non-negative numeric value", + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("\"%s\" requires a non-negative numeric value", def->defname))); } else if (strcmp(def->defname, "extensions") == 0) @@ -134,26 +142,24 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) /* check list syntax, warn about uninstalled extensions */ (void) ExtractExtensionList(defGetString(def), true); } - else if (strcmp(def->defname, "fetch_size") == 0) + else if (strcmp(def->defname, "fetch_size") == 0 || + strcmp(def->defname, "batch_size") == 0) { - int fetch_size; + int val; + bool is_parsed; - fetch_size = strtol(defGetString(def), NULL, 10); - if (fetch_size <= 0) + is_parsed = parse_int(defGetString(def), &val, 0, NULL); + + if (!is_parsed) ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s requires a non-negative integer value", + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid integer value for option \"%s\"", def->defname))); - } - else if (strcmp(def->defname, "batch_size") == 0) - { - int batch_size; - batch_size = strtol(defGetString(def), NULL, 10); - if (batch_size <= 0) + if (val <= 0) ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s requires a non-negative integer value", + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("\"%s\" requires a non-negative integer value", def->defname))); } else if (strcmp(def->defname, "password_required") == 0) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index c48a421e88..479043c189 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -4974,7 +4974,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel, if (strcmp(def->defname, "fetch_size") == 0) { - fetch_size = strtol(defGetString(def), NULL, 10); + (void) parse_int(defGetString(def), &fetch_size, 0, NULL); break; } } @@ -4984,7 +4984,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel, if (strcmp(def->defname, "fetch_size") == 0) { - fetch_size = strtol(defGetString(def), NULL, 10); + (void) parse_int(defGetString(def), &fetch_size, 0, NULL); break; } } @@ -5751,14 +5751,16 @@ apply_server_options(PgFdwRelationInfo *fpinfo) if (strcmp(def->defname, "use_remote_estimate") == 0) fpinfo->use_remote_estimate = defGetBoolean(def); else if (strcmp(def->defname, "fdw_startup_cost") == 0) - fpinfo->fdw_startup_cost = strtod(defGetString(def), NULL); + (void) parse_real(defGetString(def), &fpinfo->fdw_startup_cost, 0, + NULL); else if (strcmp(def->defname, "fdw_tuple_cost") == 0) - fpinfo->fdw_tuple_cost = strtod(defGetString(def), NULL); + (void) parse_real(defGetString(def), &fpinfo->fdw_tuple_cost, 0, + NULL); else if (strcmp(def->defname, "extensions") == 0) fpinfo->shippable_extensions = ExtractExtensionList(defGetString(def), false); else if (strcmp(def->defname, "fetch_size") == 0) - fpinfo->fetch_size = strtol(defGetString(def), NULL, 10); + (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL); else if (strcmp(def->defname, "async_capable") == 0) fpinfo->async_capable = defGetBoolean(def); } @@ -5781,7 +5783,7 @@ apply_table_options(PgFdwRelationInfo *fpinfo) if (strcmp(def->defname, "use_remote_estimate") == 0) fpinfo->use_remote_estimate = defGetBoolean(def); else if (strcmp(def->defname, "fetch_size") == 0) - fpinfo->fetch_size = strtol(defGetString(def), NULL, 10); + (void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL); else if (strcmp(def->defname, "async_capable") == 0) fpinfo->async_capable = defGetBoolean(def); } @@ -7289,7 +7291,7 @@ get_batch_size_option(Relation rel) if (strcmp(def->defname, "batch_size") == 0) { - batch_size = strtol(defGetString(def), NULL, 10); + (void) parse_int(defGetString(def), &batch_size, 0, NULL); break; } } diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 78379bdea5..61c15669d5 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -3262,3 +3262,19 @@ DROP TABLE join_tbl; ALTER SERVER loopback OPTIONS (DROP async_capable); ALTER SERVER loopback2 OPTIONS (DROP async_capable); + +-- =================================================================== +-- test invalid server and foreign table options +-- =================================================================== +-- Invalid fdw_startup_cost option +CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw + OPTIONS(fdw_startup_cost '100$%$#$#'); +-- Invalid fdw_tuple_cost option +CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw + OPTIONS(fdw_tuple_cost '100$%$#$#'); +-- Invalid fetch_size option +CREATE FOREIGN TABLE inv_fsz (c1 int ) + SERVER loopback OPTIONS (fetch_size '100$%$#$#'); +-- Invalid batch_size option +CREATE FOREIGN TABLE inv_bsz (c1 int ) + SERVER loopback OPTIONS (batch_size '100$%$#$#'); -- 2.25.1