From 5ec9e489e1068a67256ca777a1e49d146c2e2c6c Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Wed, 19 May 2021 10:49:24 +0530 Subject: [PATCH v2] 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 | 28 +++++++------------ contrib/postgres_fdw/postgres_fdw.c | 16 ++++++----- contrib/postgres_fdw/sql/postgres_fdw.sql | 16 +++++++++++ 4 files changed, 54 insertions(+), 25 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 7df30010f2..df4fdb5bb5 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: fdw_startup_cost requires a non-negative numeric value +-- Invalid fdw_tuple_cost option +CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw + OPTIONS(fdw_tuple_cost '100$%$#$#'); +ERROR: fdw_tuple_cost requires a non-negative numeric value +-- Invalid fetch_size option +CREATE FOREIGN TABLE inv_fsz (c1 int ) + SERVER loopback OPTIONS (fetch_size '100$%$#$#'); +ERROR: fetch_size requires a non-negative integer value +-- Invalid batch_size option +CREATE FOREIGN TABLE inv_bsz (c1 int ) + SERVER loopback OPTIONS (batch_size '100$%$#$#'); +ERROR: batch_size requires a non-negative integer value diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 672b55a808..3dfcf2fb78 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,10 +121,10 @@ 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 || val < 0) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s requires a non-negative numeric value", @@ -134,23 +135,14 @@ 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) - 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; - - batch_size = strtol(defGetString(def), NULL, 10); - if (batch_size <= 0) + is_parsed = parse_int(defGetString(def), &val, 0, NULL); + if (!is_parsed || val <= 0) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("%s requires a non-negative integer value", 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