From bc819b5d472972af61828035ef9a5fa5e0bf4b03 Mon Sep 17 00:00:00 2001 From: Sarah Hoffmann Date: Thu, 8 Sep 2022 18:01:47 +0200 Subject: [PATCH 1/3] make check_name() a global function There are a couple of other places outside the flex output where we should check that psql identifiers are correctly quotable. --- src/output-flex.cpp | 27 +++++++-------------------- src/pgsql.cpp | 13 +++++++++++++ src/pgsql.hpp | 13 +++++++++++++ 3 files changed, 33 insertions(+), 20 deletions(-) diff --git a/src/output-flex.cpp b/src/output-flex.cpp index b5f0b631c..4da84a716 100644 --- a/src/output-flex.cpp +++ b/src/output-flex.cpp @@ -983,25 +983,12 @@ int output_flex_t::app_as_geometrycollection() return 1; } -static void check_name(std::string const &name, char const *in) -{ - auto const pos = name.find_first_of("\"',.;$%&/()<>{}=?^*#"); - - if (pos == std::string::npos) { - return; - } - - throw std::runtime_error{ - "Special characters are not allowed in {} names: '{}'."_format(in, - name)}; -} - flex_table_t &output_flex_t::create_flex_table() { std::string const table_name = luaX_get_table_string(lua_state(), "name", -1, "The table"); - check_name(table_name, "table"); + check_identifier(table_name, "table"); auto const it = std::find_if(m_tables->cbegin(), m_tables->cend(), [&table_name](flex_table_t const &table) { @@ -1021,7 +1008,7 @@ flex_table_t &output_flex_t::create_flex_table() lua_getfield(lua_state(), -1, "schema"); if (lua_isstring(lua_state(), -1)) { std::string const schema = lua_tostring(lua_state(), -1); - check_name(schema, "schema"); + check_identifier(schema, "schema"); new_table.set_schema(schema); } lua_pop(lua_state(), 1); @@ -1052,7 +1039,7 @@ flex_table_t &output_flex_t::create_flex_table() lua_getfield(lua_state(), -1, "data_tablespace"); if (lua_isstring(lua_state(), -1)) { std::string const tablespace = lua_tostring(lua_state(), -1); - check_name(tablespace, "data_tablespace"); + check_identifier(tablespace, "data_tablespace"); new_table.set_data_tablespace(tablespace); } lua_pop(lua_state(), 1); @@ -1061,7 +1048,7 @@ flex_table_t &output_flex_t::create_flex_table() lua_getfield(lua_state(), -1, "index_tablespace"); if (lua_isstring(lua_state(), -1)) { std::string const tablespace = lua_tostring(lua_state(), -1); - check_name(tablespace, "index_tablespace"); + check_identifier(tablespace, "index_tablespace"); new_table.set_index_tablespace(tablespace); } lua_pop(lua_state(), 1); @@ -1098,7 +1085,7 @@ void output_flex_t::setup_id_columns(flex_table_t *table) if (lua_isstring(lua_state(), -1)) { std::string const column_name = lua_tolstring(lua_state(), -1, nullptr); - check_name(column_name, "column"); + check_identifier(column_name, "column"); auto &column = table->add_column(column_name, "id_type", ""); column.set_not_null(); } else if (!lua_isnil(lua_state(), -1)) { @@ -1111,7 +1098,7 @@ void output_flex_t::setup_id_columns(flex_table_t *table) std::string const name = luaX_get_table_string(lua_state(), "id_column", -2, "The ids field"); - check_name(name, "column"); + check_identifier(name, "column"); auto &column = table->add_column(name, "id_num", ""); column.set_not_null(); @@ -1143,7 +1130,7 @@ void output_flex_t::setup_flex_table_columns(flex_table_t *table) "Column entry", "text"); char const *const name = luaX_get_table_string(lua_state(), "column", -2, "Column entry"); - check_name(name, "column"); + check_identifier(name, "column"); char const *const sql_type = luaX_get_table_string( lua_state(), "sql_type", -3, "Column entry", ""); diff --git a/src/pgsql.cpp b/src/pgsql.cpp index 96b6e3868..a3b0b08a8 100644 --- a/src/pgsql.cpp +++ b/src/pgsql.cpp @@ -197,6 +197,19 @@ std::string qualified_name(std::string const &schema, std::string const &name) return result; } +void check_identifier(std::string const &name, char const *in) +{ + auto const pos = name.find_first_of("\"',.;$%&/()<>{}=?^*#"); + + if (pos == std::string::npos) { + return; + } + + throw std::runtime_error{ + "Special characters are not allowed in {} names: '{}'."_format(in, + name)}; +} + std::map get_postgresql_settings(pg_conn_t const &db_connection) { diff --git a/src/pgsql.hpp b/src/pgsql.hpp index 971386bfc..84b74cf7b 100644 --- a/src/pgsql.hpp +++ b/src/pgsql.hpp @@ -247,6 +247,19 @@ std::string tablespace_clause(std::string const &name); */ std::string qualified_name(std::string const &schema, std::string const &name); +/** + * Check that the string confirms to the identifier syntax we accept. + * Throws a runtime exception if an invalid character is found. + * + * Note that PostgreSQL accepts any character in a quoted identifier. + * This function checks for some characters that are problematic in the + * internal functions that create SQL statements. + * + * \param name Identifier to check. + * \param in Name of the identifier. Only used to create a human-readable error. + */ +void check_identifier(std::string const &name, char const *in); + struct postgis_version { int major; From 0210c7d69a96acb79bd5e157f7f8ef37d1f0f393 Mon Sep 17 00:00:00 2001 From: Sarah Hoffmann Date: Thu, 8 Sep 2022 21:18:57 +0200 Subject: [PATCH 2/3] check more user-supplied strings used as SQL identifier Adds checks for the prefix and schema commandline parameters and for column names and types from the pgsql style file. --- src/options.cpp | 4 ++++ src/table.cpp | 3 +++ 2 files changed, 7 insertions(+) diff --git a/src/options.cpp b/src/options.cpp index abde8c357..db5705ffe 100644 --- a/src/options.cpp +++ b/src/options.cpp @@ -10,6 +10,7 @@ #include "format.hpp" #include "logging.hpp" #include "options.hpp" +#include "pgsql.hpp" #include "reprojection.hpp" #include "util.hpp" #include "version.hpp" @@ -394,6 +395,7 @@ options_t::options_t(int argc, char *argv[]) : options_t() break; case 'p': prefix = optarg; + check_identifier(prefix, "prefix"); break; case 'd': database_options.db = optarg; @@ -557,9 +559,11 @@ options_t::options_t(int argc, char *argv[]) : options_t() break; case 215: middle_dbschema = optarg; + check_identifier(middle_dbschema, "middle-schema"); break; case 216: output_dbschema = optarg; + check_identifier(output_dbschema, "output-pgsql-schema"); break; case 217: if (std::strcmp(optarg, "false") == 0) { diff --git a/src/table.cpp b/src/table.cpp index 57ce32583..253b57570 100644 --- a/src/table.cpp +++ b/src/table.cpp @@ -107,11 +107,14 @@ void table_t::start(std::string const &conninfo, std::string const &table_space) //first with the regular columns for (auto const &column : m_columns) { + check_identifier(column.name, "column"); + check_identifier(column.type_name, "column type"); sql += R"("{}" {},)"_format(column.name, column.type_name); } //then with the hstore columns for (auto const &hcolumn : m_hstore_columns) { + check_identifier(hcolumn, "column"); sql += R"("{}" hstore,)"_format(hcolumn); } From 7f64c62d6e061759a89bcd985ab4c8c0e6561fca Mon Sep 17 00:00:00 2001 From: Sarah Hoffmann Date: Fri, 9 Sep 2022 08:58:38 +0200 Subject: [PATCH 3/3] improve error message for illegal characters --- src/options.cpp | 6 +++--- src/output-flex.cpp | 14 +++++++------- src/pgsql.cpp | 3 +-- src/table.cpp | 6 +++--- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/options.cpp b/src/options.cpp index db5705ffe..36dcd2431 100644 --- a/src/options.cpp +++ b/src/options.cpp @@ -395,7 +395,7 @@ options_t::options_t(int argc, char *argv[]) : options_t() break; case 'p': prefix = optarg; - check_identifier(prefix, "prefix"); + check_identifier(prefix, "--prefix parameter"); break; case 'd': database_options.db = optarg; @@ -559,11 +559,11 @@ options_t::options_t(int argc, char *argv[]) : options_t() break; case 215: middle_dbschema = optarg; - check_identifier(middle_dbschema, "middle-schema"); + check_identifier(middle_dbschema, "--middle-schema parameter"); break; case 216: output_dbschema = optarg; - check_identifier(output_dbschema, "output-pgsql-schema"); + check_identifier(output_dbschema, "--output-pgsql-schema parameter"); break; case 217: if (std::strcmp(optarg, "false") == 0) { diff --git a/src/output-flex.cpp b/src/output-flex.cpp index 4da84a716..9a8b2370a 100644 --- a/src/output-flex.cpp +++ b/src/output-flex.cpp @@ -988,7 +988,7 @@ flex_table_t &output_flex_t::create_flex_table() std::string const table_name = luaX_get_table_string(lua_state(), "name", -1, "The table"); - check_identifier(table_name, "table"); + check_identifier(table_name, "table names"); auto const it = std::find_if(m_tables->cbegin(), m_tables->cend(), [&table_name](flex_table_t const &table) { @@ -1008,7 +1008,7 @@ flex_table_t &output_flex_t::create_flex_table() lua_getfield(lua_state(), -1, "schema"); if (lua_isstring(lua_state(), -1)) { std::string const schema = lua_tostring(lua_state(), -1); - check_identifier(schema, "schema"); + check_identifier(schema, "schema field"); new_table.set_schema(schema); } lua_pop(lua_state(), 1); @@ -1039,7 +1039,7 @@ flex_table_t &output_flex_t::create_flex_table() lua_getfield(lua_state(), -1, "data_tablespace"); if (lua_isstring(lua_state(), -1)) { std::string const tablespace = lua_tostring(lua_state(), -1); - check_identifier(tablespace, "data_tablespace"); + check_identifier(tablespace, "data_tablespace field"); new_table.set_data_tablespace(tablespace); } lua_pop(lua_state(), 1); @@ -1048,7 +1048,7 @@ flex_table_t &output_flex_t::create_flex_table() lua_getfield(lua_state(), -1, "index_tablespace"); if (lua_isstring(lua_state(), -1)) { std::string const tablespace = lua_tostring(lua_state(), -1); - check_identifier(tablespace, "index_tablespace"); + check_identifier(tablespace, "index_tablespace field"); new_table.set_index_tablespace(tablespace); } lua_pop(lua_state(), 1); @@ -1085,7 +1085,7 @@ void output_flex_t::setup_id_columns(flex_table_t *table) if (lua_isstring(lua_state(), -1)) { std::string const column_name = lua_tolstring(lua_state(), -1, nullptr); - check_identifier(column_name, "column"); + check_identifier(column_name, "column names"); auto &column = table->add_column(column_name, "id_type", ""); column.set_not_null(); } else if (!lua_isnil(lua_state(), -1)) { @@ -1098,7 +1098,7 @@ void output_flex_t::setup_id_columns(flex_table_t *table) std::string const name = luaX_get_table_string(lua_state(), "id_column", -2, "The ids field"); - check_identifier(name, "column"); + check_identifier(name, "column names"); auto &column = table->add_column(name, "id_num", ""); column.set_not_null(); @@ -1130,7 +1130,7 @@ void output_flex_t::setup_flex_table_columns(flex_table_t *table) "Column entry", "text"); char const *const name = luaX_get_table_string(lua_state(), "column", -2, "Column entry"); - check_identifier(name, "column"); + check_identifier(name, "column names"); char const *const sql_type = luaX_get_table_string( lua_state(), "sql_type", -3, "Column entry", ""); diff --git a/src/pgsql.cpp b/src/pgsql.cpp index a3b0b08a8..7b36c64fc 100644 --- a/src/pgsql.cpp +++ b/src/pgsql.cpp @@ -206,8 +206,7 @@ void check_identifier(std::string const &name, char const *in) } throw std::runtime_error{ - "Special characters are not allowed in {} names: '{}'."_format(in, - name)}; + "Special characters are not allowed in {}: '{}'."_format(in, name)}; } std::map diff --git a/src/table.cpp b/src/table.cpp index 253b57570..940af176d 100644 --- a/src/table.cpp +++ b/src/table.cpp @@ -107,14 +107,14 @@ void table_t::start(std::string const &conninfo, std::string const &table_space) //first with the regular columns for (auto const &column : m_columns) { - check_identifier(column.name, "column"); - check_identifier(column.type_name, "column type"); + check_identifier(column.name, "column names"); + check_identifier(column.type_name, "column types"); sql += R"("{}" {},)"_format(column.name, column.type_name); } //then with the hstore columns for (auto const &hcolumn : m_hstore_columns) { - check_identifier(hcolumn, "column"); + check_identifier(hcolumn, "column names"); sql += R"("{}" hstore,)"_format(hcolumn); }