From 83cb59dd4957112fda9bcff9d2b5846a6b4e7f32 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Thu, 1 Jul 2021 03:55:21 +0000 Subject: [PATCH v5] 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 ++++++ doc/src/sgml/postgres-fdw.sgml | 14 ++--- 5 files changed, 82 insertions(+), 35 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 31b5de91ad..d8173d4cc4 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 floating point 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 floating point 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..a7abbae5a1 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 floating point 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 floating point 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$%$#$#'); diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index d7d2baafc9..c9fce77599 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -266,11 +266,11 @@ OPTIONS (ADD password_required 'false'); fdw_startup_cost (floating point) - This option, which can be specified for a foreign server, is a numeric - value that is added to the estimated startup cost of any foreign-table - scan on that server. This represents the additional overhead of - establishing a connection, parsing and planning the query on the - remote side, etc. + This option, which can be specified for a foreign server, is a floating + point value that is added to the estimated startup cost of any + foreign-table scan on that server. This represents the additional + overhead of establishing a connection, parsing and planning the query on + the remote side, etc. The default value is 100. @@ -280,8 +280,8 @@ OPTIONS (ADD password_required 'false'); fdw_tuple_cost (floating point) - This option, which can be specified for a foreign server, is a numeric - value that is used as extra cost per-tuple for foreign-table + This option, which can be specified for a foreign server, is a floating + point value that is used as extra cost per-tuple for foreign-table scans on that server. This represents the additional overhead of data transfer between servers. You might increase or decrease this number to reflect higher or lower network delay to the remote server. -- 2.25.1