>From c45ea69fd3841c4cb223be57587e055b878e16e5 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Thu, 15 Jan 2015 13:25:17 +0200 Subject: [PATCH 1/2] More fixes to building query with SQL_INTEGER params. Jeremy Faith pointed out that the code still incorrectly accepted '-' as a valid integer. But a bigger issue with negative numbers is that even a valid negative integer can lead to bogus queries. Consider "SELECT 0-?", where the parameter is -123. It was expanded to "SELECT 0--123". To fix, don't accept '-' as a valid integer, forcing it to be quoted. And if the value is negative, put parens around it, so that the previous example becomes "SELECT 0-(-123)". Add test cases for these issues. --- convert.c | 127 +++++++++++++++++++++------------- test/expected/param-conversions.out | 9 +++ test/expected/param-conversions_1.out | 9 +++ test/src/param-conversions-test.c | 22 ++++-- 4 files changed, 113 insertions(+), 54 deletions(-) diff --git a/convert.c b/convert.c index cf8a16e..7cdf135 100644 --- a/convert.c +++ b/convert.c @@ -707,25 +707,34 @@ copy_and_convert_field_bindinfo(StatementClass *stmt, OID field_type, int atttyp * Is 'str' a valid integer literal, consisting only of ASCII characters * 0-9 ? * + * Also, *negative is set to TRUE if the value was negative. + * * We don't check for overflow here. This is just to decide if we need to * quote the value. */ static BOOL -valid_int_literal(const char *str, SQLLEN len) +valid_int_literal(const char *str, SQLLEN len, BOOL *negative) { SQLLEN i; i = 0; if (str[0] == '-') + { i++; + *negative = TRUE; + } + else + *negative = FALSE; + + /* Must begin with a digit. This rejects empty strings, too */ + if (!(str[i] >= '0' && str[i] <= '9')) + return FALSE; for (; str[i] && (len == SQL_NTS || i < len); i++) { - if (str[i] >= '0' && str[i] <= '9') - continue; - - return FALSE; + if (!(str[i] >= '0' && str[i] <= '9')) + return FALSE; } return TRUE; @@ -3885,7 +3894,10 @@ ResolveOneParam(QueryBuild *qb, QueryParse *qp, BOOL *isnull, BOOL *isbinary) int lobj_fd; SQLULEN offset = apdopts->param_offset_ptr ? *apdopts->param_offset_ptr : 0; size_t current_row = qb->current_row; - BOOL handling_large_object = FALSE, req_bind, add_quote = FALSE; + BOOL handling_large_object = FALSE, req_bind; + BOOL need_quotes = TRUE; + BOOL add_parens = FALSE; + BOOL negative; ParameterInfoClass *apara; ParameterImplClass *ipara; BOOL outputDiscard, @@ -4634,63 +4646,84 @@ mylog("cvt_null_date_string=%d pgtype=%d buf=%p\n", conn->connInfo.cvt_null_date break; case SQL_NUMERIC: break; + /* + * If it looks like a valid integer, we can pass it without quotes + * and let the server interpret it. Arguably, it would always be + * better to explicitly pass it as 'xxx'::integer or 'xxx'::smallint, + * but historically we haven't done that, so let's avoid changing the + * behaviour. + * + * If it's a negative number, we have to wrap it in parens. Otherwise + * a query like "SELECT 0-?" would turn into "SELECT 0--123". + */ + case SQL_INTEGER: + if (valid_int_literal(buf, used, &negative)) + { + need_quotes = FALSE; + add_parens = negative; + } + else + { + /* + * Doesn't look like a valid integer. The server will most + * likely throw an error, unless it's in some format we don't + * recognize but the server does. + */ + lastadd = "::int4"; + } + break; + case SQL_SMALLINT: + if (valid_int_literal(buf, used, &negative)) + { + need_quotes = FALSE; + add_parens = negative; + } + else + lastadd = "::smallint"; + break; default: /* a numeric type or SQL_BIT */ break; } + if (used == SQL_NTS) + used = strlen(buf); + /* * Ok, we now have the final string representation in 'buf', length 'used'. + * We're ready to output the final string, with quotes and other + * embellishments if necessary. * - * Do we need to escape it? + * In bind-mode, we don't need to do any quoting. */ - if (!req_bind) + if (req_bind) + CVT_APPEND_DATA(qb, buf, used); + else { - switch (param_sqltype) + if (add_parens) + CVT_APPEND_CHAR(qb, '('); + + if (need_quotes) { - case SQL_INTEGER: - case SQL_SMALLINT: - if (valid_int_literal(buf, used)) - break; - /* fall through */ - case SQL_CHAR: - case SQL_VARCHAR: - case SQL_LONGVARCHAR: - case SQL_BINARY: - case SQL_VARBINARY: - case SQL_LONGVARBINARY: -#ifdef UNICODE_SUPPORT - case SQL_WCHAR: - case SQL_WVARCHAR: - case SQL_WLONGVARCHAR: -#endif /* UNICODE_SUPPORT */ -mylog("buf=%p flag=%d\n", buf, qb->flags); - default: - if ((qb->flags & FLGB_LITERAL_EXTENSION) != 0) - CVT_APPEND_CHAR(qb, LITERAL_EXT); - CVT_APPEND_CHAR(qb, LITERAL_QUOTE); - add_quote = TRUE; - break; - } - } + if ((qb->flags & FLGB_LITERAL_EXTENSION) != 0) + CVT_APPEND_CHAR(qb, LITERAL_EXT); + CVT_APPEND_CHAR(qb, LITERAL_QUOTE); - if (used == SQL_NTS) - used = strlen(buf); - if (add_quote) - { - if (final_binary_convert) - CVT_APPEND_BINARY(qb, buf, used); + if (final_binary_convert) + CVT_APPEND_BINARY(qb, buf, used); + else + CVT_SPECIAL_CHARS(qb, buf, used); + + CVT_APPEND_CHAR(qb, LITERAL_QUOTE); + } else - CVT_SPECIAL_CHARS(qb, buf, used); - } - else - CVT_APPEND_DATA(qb, buf, used); + CVT_APPEND_DATA(qb, buf, used); - if (add_quote) - { - CVT_APPEND_CHAR(qb, LITERAL_QUOTE); + if (add_parens) + CVT_APPEND_CHAR(qb, ')'); if (lastadd) CVT_APPEND_STR(qb, lastadd); } + retval = SQL_SUCCESS; cleanup: if (allocbuf) diff --git a/test/expected/param-conversions.out b/test/expected/param-conversions.out index f282396..2359e39 100644 --- a/test/expected/param-conversions.out +++ b/test/expected/param-conversions.out @@ -27,4 +27,13 @@ Error while executing the query SQLExecDirect failed 22003=ERROR: value "99999999999999999999999" is out of range for type integer; Error while executing the query +Result set: +2 +SQLExecDirect failed +22P02=ERROR: invalid input syntax for integer: "-"; +Error while executing the query +Result set: +-1234 +Result set: +1234 disconnecting diff --git a/test/expected/param-conversions_1.out b/test/expected/param-conversions_1.out index c75aee9..f3d1194 100644 --- a/test/expected/param-conversions_1.out +++ b/test/expected/param-conversions_1.out @@ -26,4 +26,13 @@ SQLExecDirect failed Error while executing the query Result set: 0 +Result set: +2 +SQLExecDirect failed +22P02=ERROR: invalid input syntax for integer: "-"; +Error while executing the query +Result set: +-1234 +Result set: +1234 disconnecting diff --git a/test/src/param-conversions-test.c b/test/src/param-conversions-test.c index 47caee4..2603d03 100644 --- a/test/src/param-conversions-test.c +++ b/test/src/param-conversions-test.c @@ -7,13 +7,14 @@ #include "common.h" static void test_convert(const char *sql, SQLUSMALLINT c_type, - SQLSMALLINT sql_type, char *value); + SQLSMALLINT sql_type, SQLPOINTER value); static HSTMT hstmt = SQL_NULL_HSTMT; int main(int argc, char **argv) { SQLRETURN rc; + SQLINTEGER intparam; test_connect(); @@ -24,7 +25,7 @@ int main(int argc, char **argv) exit(1); } - /*** Test proper escaping of parameters ***/ + /*** Test proper escaping of integer parameters ***/ printf("\nTesting conversions...\n"); test_convert("SELECT 1 > ?", SQL_C_CHAR, SQL_INTEGER, "2"); @@ -39,6 +40,13 @@ int main(int argc, char **argv) test_convert("SELECT 1.3 > ?", SQL_C_CHAR, SQL_FLOAT, "3', 'injected, BAD!', '1"); test_convert("SELECT 1.4 > ?", SQL_C_CHAR, SQL_FLOAT, "4 \\'bad', '1"); test_convert("SELECT 1 > ?", SQL_C_CHAR, SQL_INTEGER, "99999999999999999999999"); + test_convert("SELECT 1-?", SQL_C_CHAR, SQL_INTEGER, "-1"); + test_convert("SELECT 1 > ?", SQL_C_CHAR, SQL_INTEGER, "-"); + + intparam = 1234; + test_convert("SELECT 0-?", SQL_C_SLONG, SQL_INTEGER, &intparam); + intparam = -1234; + test_convert("SELECT 0-?", SQL_C_SLONG, SQL_INTEGER, &intparam); /* Clean up */ test_disconnect(); @@ -52,13 +60,13 @@ int main(int argc, char **argv) */ static void test_convert(const char *sql, SQLUSMALLINT c_type, SQLSMALLINT sql_type, - char *value) + SQLPOINTER value) { - SQLRETURN rc; - SQLLEN cbParam = SQL_NTS; - int failed = 0; + SQLRETURN rc; + SQLLEN cbParam = SQL_NTS; + int failed = 0; - /* a query with an SQL_INTEGER param. */ + cbParam = SQL_NTS; /* ignored for non-character data */ rc = SQLBindParameter(hstmt, 1, SQL_PARAM_INPUT, c_type, /* value type */ sql_type, /* param type */ -- 2.1.4