From 39d251a1e126f9a3fcea7e89aa34c5576a4e88e9 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Wed, 30 Jun 2021 14:30:20 +0000 Subject: [PATCH v4] 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 | 52 +++++++++++-------- contrib/postgres_fdw/postgres_fdw.c | 16 +++--- contrib/postgres_fdw/sql/postgres_fdw.sql | 16 ++++++ 4 files changed, 75 insertions(+), 28 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 31b5de91ad..966be24fdc 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -10478,3 +10478,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 value for numeric option "fdw_startup_cost": 100$%$#$# +-- Invalid fdw_tuple_cost option +CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw + OPTIONS(fdw_tuple_cost '100$%$#$#'); +ERROR: invalid value for numeric option "fdw_tuple_cost": 100$%$#$# +-- Invalid fetch_size option +CREATE FOREIGN TABLE inv_fsz (c1 int ) + SERVER loopback OPTIONS (fetch_size '100$%$#$#'); +ERROR: invalid value for integer option "fetch_size": 100$%$#$# +-- Invalid batch_size option +CREATE FOREIGN TABLE inv_bsz (c1 int ) + SERVER loopback OPTIONS (batch_size '100$%$#$#'); +ERROR: invalid value for integer option "batch_size": 100$%$#$# diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 672b55a808..a56818dcf7 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" /* @@ -119,14 +120,23 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) strcmp(def->defname, "fdw_tuple_cost") == 0) { /* these must have a non-negative numeric value */ - double val; - char *endp; + char *value; + double real_val; + bool is_parsed; - val = strtod(defGetString(def), &endp); - if (*endp || val < 0) + value = defGetString(def); + is_parsed = parse_real(value, &real_val, 0, NULL); + + if (!is_parsed) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid value for numeric option \"%s\": %s", + def->defname, value))); + + if (real_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 +144,26 @@ 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; + char *value; + int int_val; + bool is_parsed; + + value = defGetString(def); + is_parsed = parse_int(value, &int_val, 0, NULL); - fetch_size = strtol(defGetString(def), NULL, 10); - if (fetch_size <= 0) + if (!is_parsed) ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s requires a non-negative integer value", - def->defname))); - } - else if (strcmp(def->defname, "batch_size") == 0) - { - int batch_size; + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("invalid value for integer option \"%s\": %s", + def->defname, value))); - batch_size = strtol(defGetString(def), NULL, 10); - if (batch_size <= 0) + if (int_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 fafbab6b02..ad0fba317c 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -5032,7 +5032,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; } } @@ -5042,7 +5042,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; } } @@ -5809,14 +5809,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); } @@ -5839,7 +5841,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); } @@ -7347,7 +7349,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 286dd99573..3b6bfc5218 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -3337,3 +3337,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