Thread: SQL_CURSOR_TYPE prepare execute issue

On 01/13/2015 07:57 PM, Faith, Jeremy wrote:
> Hi,
>
> With PosgreSQL 9.3.5 and pgsqlodbc 9.03.03
>
> I have found an issue with setting cursor type to anything other than the default(SQL_CURSOR_FORWARD_ONLY) and the
checkingdone on prepare/execute parameters. 
>
> In particular with regards to protection from SQL injection attacks.
>
> Say for example there is a table
>    create table t1(c1 integer);
>
> When following is run
>    SQLSMALLINT dec_digits;
>    SQLUINTEGER col_size;
>    SQLHSTMT stmt;
>    char buf[100];
>
>    //... not showing con setup
>    SQLAllocHandle(SQL_HANDLE_STMT,con->sqlcon,&stmt);
>
>    SQLSetStmtOption(stmt,SQL_CURSOR_TYPE,SQL_CURSOR_STATIC);
>    SQLPrepare(stmt,(SQLCHAR *)"select * from t1 where c1=?",SQL_NTS);
>
>    sprintf(buf,"%s","1 and zzz='dummy'"); //i.e. injection attempt
> //this could be much worse e.g. "1; drop table personnel;"
>    len=strlen(buf);
>    col_size=10;
>    dec_digits=-1;
>    SQLBindParameter(stmt,1,SQL_PARAM_INPUT,SQL_C_CHAR,SQL_INTEGER,col_size,
>      dec_digits,buf,0,&len);
>
>    SQLExecute(stmt);
>
> The SQLExecute will fail(as it should) but with the cursor set to SQL_CURSOR_STATIC then the error is:-
>    stmt native_err=7 [42703]ERROR: column "zzz" does not exist;
>
> Whereas if the cursor type is not set(and is thus the default SQL_CURSOR_FORWARD_ONLY) then the error is much
better(andsafer):- 
>    stmt native_err=7 [22P02]ERROR: invalid input syntax for integer: "1 and zzz='dummy' ";
>
> Also if in the odbc.ini file
>    UseServerSidePrepare  = 0
> for the connection then error is always: column "zzz" does not exist, regardless of the cursor type.

Hmm. The driver blindly assumes that if parameter is bound as
SQL_INTEGER or SQL_SMALLINT, it doesn't require quoting. But clearly
that's not true, if the passed parameter string is not a valid integer.

> I found this problem when testing with PHP odbc_prepare/odbc_execute
> functions as the odbc_prepare function sets the cursor type to
> SQL_CURSOR_STATIC(by default though it can be overridden by setting
> odbc.default_cursortype = SQL_CURSOR_FORWARD_ONLY in php.ini). PHP
> needs UseServerSidePrepare=1 as it also calls SQLDescribeParam()
> which fails if UseServerSidePrepare=0.
>
> The question is why does changing the cursor type change the way the
> parameters are bound? Can anything be done to fix this?

I just pushed a fix and a regression test to the git repository. Thanks
for the report!

- Heikki

Hi,

Thanks for the fix.

I think the test code should call
  SQLSetStmtOption(hstmt,SQL_CURSOR_TYPE,SQL_CURSOR_STATIC);
after SQLAllocHandle. As otherwise even with the 9.03.03 version of the ODBC driver the correct error occurs unless
UseServerSidePrepare=0in the odbc.ini file. That is I am not sure the changed code is being used by the test as it is. 

I have had a quick look over the change and it looks ok to me. Something of a clean up and simplification as well.
If I understand it correctly, the only things that don't get quoted are SQL_INTEGER and SQL_SMALLINT that pass the new
valid_int_literal()test. 
The only thing I can see that could pass that test and not be a valid integer would be a single minus char i.e. "-"
not sure if there is anyway that could be vulnerable though.

Any idea when a new release of the driver is likely?
I know a big change to use libpq has occurred recently so I guess it could be a while.

Regards,
Jeremy Faith

________________________________________
From: Heikki Linnakangas [hlinnakangas@vmware.com]
Sent: 13 January 2015 23:00
To: Faith, Jeremy; pgsql-odbc@postgresql.org
Subject: Re: [ODBC] SQL_CURSOR_TYPE prepare execute issue

On 01/13/2015 07:57 PM, Faith, Jeremy wrote:
> Hi,
>
> With PosgreSQL 9.3.5 and pgsqlodbc 9.03.03
>
> I have found an issue with setting cursor type to anything other than the default(SQL_CURSOR_FORWARD_ONLY) and the
checkingdone on prepare/execute parameters. 
>
> In particular with regards to protection from SQL injection attacks.
>
> Say for example there is a table
>    create table t1(c1 integer);
>
> When following is run
>    SQLSMALLINT dec_digits;
>    SQLUINTEGER col_size;
>    SQLHSTMT stmt;
>    char buf[100];
>
>    //... not showing con setup
>    SQLAllocHandle(SQL_HANDLE_STMT,con->sqlcon,&stmt);
>
>    SQLSetStmtOption(stmt,SQL_CURSOR_TYPE,SQL_CURSOR_STATIC);
>    SQLPrepare(stmt,(SQLCHAR *)"select * from t1 where c1=?",SQL_NTS);
>
>    sprintf(buf,"%s","1 and zzz='dummy'"); //i.e. injection attempt
> //this could be much worse e.g. "1; drop table personnel;"
>    len=strlen(buf);
>    col_size=10;
>    dec_digits=-1;
>    SQLBindParameter(stmt,1,SQL_PARAM_INPUT,SQL_C_CHAR,SQL_INTEGER,col_size,
>      dec_digits,buf,0,&len);
>
>    SQLExecute(stmt);
>
> The SQLExecute will fail(as it should) but with the cursor set to SQL_CURSOR_STATIC then the error is:-
>    stmt native_err=7 [42703]ERROR: column "zzz" does not exist;
>
> Whereas if the cursor type is not set(and is thus the default SQL_CURSOR_FORWARD_ONLY) then the error is much
better(andsafer):- 
>    stmt native_err=7 [22P02]ERROR: invalid input syntax for integer: "1 and zzz='dummy' ";
>
> Also if in the odbc.ini file
>    UseServerSidePrepare  = 0
> for the connection then error is always: column "zzz" does not exist, regardless of the cursor type.

Hmm. The driver blindly assumes that if parameter is bound as
SQL_INTEGER or SQL_SMALLINT, it doesn't require quoting. But clearly
that's not true, if the passed parameter string is not a valid integer.

> I found this problem when testing with PHP odbc_prepare/odbc_execute
> functions as the odbc_prepare function sets the cursor type to
> SQL_CURSOR_STATIC(by default though it can be overridden by setting
> odbc.default_cursortype = SQL_CURSOR_FORWARD_ONLY in php.ini). PHP
> needs UseServerSidePrepare=1 as it also calls SQLDescribeParam()
> which fails if UseServerSidePrepare=0.
>
> The question is why does changing the cursor type change the way the
> parameters are bound? Can anything be done to fix this?

I just pushed a fix and a regression test to the git repository. Thanks
for the report!

- Heikki

Tyco Safety Products/CEM Systems Ltd.

________________________________

This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you
arenot the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any
actionin respect of any information contained in it. If you have received this e-mail in error, please notify the
senderimmediately by e-mail and immediately destroy this e-mail and its attachments. 

On 01/14/2015 06:56 PM, Faith, Jeremy wrote:
> Hi,
>
> Thanks for the fix.
>
> I think the test code should call
>    SQLSetStmtOption(hstmt,SQL_CURSOR_TYPE,SQL_CURSOR_STATIC);
> after SQLAllocHandle. As otherwise even with the 9.03.03 version of the ODBC driver the correct error occurs unless
UseServerSidePrepare=0in the odbc.ini file. That is I am not sure the changed code is being used by the test as it is. 

It is common practice to run the test suite with different settings. At
least I do it, I'm not sure of others, though :-). I created a makefile
target "installcheck-all" specifically for that.

There are some subtle differences in the behaviour with
UserServerSidePrepare=0/1 in what datatype the server chooses for the
parameters, so I'd like to continue testing both and not force the
non-server-side codepath with SQLSetStmtOption.

> I have had a quick look over the change and it looks ok to me. Something of a clean up and simplification as well.
> If I understand it correctly, the only things that don't get quoted are SQL_INTEGER and SQL_SMALLINT that pass the
newvalid_int_literal() test. 
> The only thing I can see that could pass that test and not be a valid integer would be a single minus char i.e. "-"
> not sure if there is anyway that could be vulnerable though.

Ah, good catch. That is definitely a problem. Consider:

SELECT * FROM foo WHERE 1-? > 0

If you replace ? with -, it becomes "--", which comments out the rest of
the query. That's actually a problem with any negative number.

It would be tempting to just always quote the value, but that again
would lead to subtle changes in the datatype that the server chooses.
There is a difference between:

SELECT '555' > '6';

and

SELECT '555' > 6;

We'd have to force the datatype with something like:

SELECT '555' > '6'::int4;

Perhaps that's the safest thing to do. It might cause subtle changes in
behaviour too, if e.g. you pass a number larger than what fits in an
int4. The server will upgrade it to an int8 or numeric constant if it's
just a numeric literal, but if you force it to int4, you'll get an
error. Now, you can certainly argue that that's a good thing, as you
shouldn't have specified SQL_INTEGER for an over-sized value in the
first place. So perhaps we should just bite the bullet and do that.

Another argument is that '6'::int4 looks more ugly than just 6. Not a
big deal, but still.

Another option is to use parens around a negative number. So it would
become:

SELECT '555' > (-6)

or

SELECT * FROM foo WHERE 1-(-6) > 0

That would be closest to the current behaviour. Does anyone see a
problem with that?

- Heikki

On 01/14/2015 06:56 PM, Faith, Jeremy wrote:
> Any idea when a new release of the driver is likely?
> I know a big change to use libpq has occurred recently so I guess it could be a while.

No idea.. We really should make a release pretty soon, IMHO. There's
been a bunch of fixes that would be nice to get released. There's a new
version of OpenSSL out there too, although AFAICS none of the fixed
vulnerabilities are too serious (DoS bugs, and the POODLE issue which I
believe doesn't affect use with PostgreSQL).

It would be nice to get more people to test the libpq changes before
that, but to be honest, we're not going to get any more testing than
what's already been done until we make a release and people start using it.

- Heikki

Hi,

As long as you are confident that the test suite is checking this code path that is fine, I just wanted to be sure that
washappening. 

I hadn't thought of
  select x-?
You are right that is a problem.
What does the server prepare code path do?
I did a quick check, sever side prepare, with the log turned on and it shows the following
  LOG:  execute <unnamed>: select 1-$1
  DETAIL:  parameters: $1 = '-1'
  LOG:  execute <unnamed>: select 1-$1
  DETAIL:  parameters: $1 = '1'
But if the bound string is '-1' then I get
  ERROR:  invalid input syntax for integer: "'-1'"   i.e. double quote,single quote,-,1,single quote,double quote
  STATEMENT:  select 'wibble',1-$1
Also get invalid syntax error if it is just a minus sign.

ok, server side prepare log when string is just 6 shows
  LOG:  execute <unnamed>: select '555'>$1
  DETAIL:  parameters: $1 = '6'
and this return one row with a value of 0
so this is doing select '555'>'6';

So to be consistent with what the server side prepare does, I think it should quote the value.
You should check this as I may have been getting confused.
I realise this may have some implications for existing programs but given that sometimes it will be server side prepare
andsometimes client side it is likely to cause more issues if the results are different. 

PS. Sorry about top posting. Outlook Web App provides no other option.

Regards,
Jeremy
________________________________________
From: Heikki Linnakangas [hlinnakangas@vmware.com]
Sent: 15 January 2015 10:52
To: Faith, Jeremy; pgsql-odbc@postgresql.org
Subject: Re: [ODBC] SQL_CURSOR_TYPE prepare execute issue

On 01/14/2015 06:56 PM, Faith, Jeremy wrote:
> Hi,
>
> Thanks for the fix.
>
> I think the test code should call
>    SQLSetStmtOption(hstmt,SQL_CURSOR_TYPE,SQL_CURSOR_STATIC);
> after SQLAllocHandle. As otherwise even with the 9.03.03 version of the ODBC driver the correct error occurs unless
UseServerSidePrepare=0in the odbc.ini file. That is I am not sure the changed code is being used by the test as it is. 

It is common practice to run the test suite with different settings. At
least I do it, I'm not sure of others, though :-). I created a makefile
target "installcheck-all" specifically for that.

There are some subtle differences in the behaviour with
UserServerSidePrepare=0/1 in what datatype the server chooses for the
parameters, so I'd like to continue testing both and not force the
non-server-side codepath with SQLSetStmtOption.

> I have had a quick look over the change and it looks ok to me. Something of a clean up and simplification as well.
> If I understand it correctly, the only things that don't get quoted are SQL_INTEGER and SQL_SMALLINT that pass the
newvalid_int_literal() test. 
> The only thing I can see that could pass that test and not be a valid integer would be a single minus char i.e. "-"
> not sure if there is anyway that could be vulnerable though.

Ah, good catch. That is definitely a problem. Consider:

SELECT * FROM foo WHERE 1-? > 0

If you replace ? with -, it becomes "--", which comments out the rest of
the query. That's actually a problem with any negative number.

It would be tempting to just always quote the value, but that again
would lead to subtle changes in the datatype that the server chooses.
There is a difference between:

SELECT '555' > '6';

and

SELECT '555' > 6;

We'd have to force the datatype with something like:

SELECT '555' > '6'::int4;

Perhaps that's the safest thing to do. It might cause subtle changes in
behaviour too, if e.g. you pass a number larger than what fits in an
int4. The server will upgrade it to an int8 or numeric constant if it's
just a numeric literal, but if you force it to int4, you'll get an
error. Now, you can certainly argue that that's a good thing, as you
shouldn't have specified SQL_INTEGER for an over-sized value in the
first place. So perhaps we should just bite the bullet and do that.

Another argument is that '6'::int4 looks more ugly than just 6. Not a
big deal, but still.

Another option is to use parens around a negative number. So it would
become:

SELECT '555' > (-6)

or

SELECT * FROM foo WHERE 1-(-6) > 0

That would be closest to the current behaviour. Does anyone see a
problem with that?

- Heikki

Tyco Safety Products/CEM Systems Ltd.

________________________________

This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you
arenot the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any
actionin respect of any information contained in it. If you have received this e-mail in error, please notify the
senderimmediately by e-mail and immediately destroy this e-mail and its attachments. 

On 01/15/2015 02:22 PM, Faith, Jeremy wrote:
> As long as you are confident that the test suite is checking this code path that is fine, I just wanted to be sure
thatwas happening. 

Sure, thanks!

> I hadn't thought of
>    select x-?
> You are right that is a problem.
> What does the server prepare code path do?
> I did a quick check, sever side prepare, with the log turned on and it shows the following
>    LOG:  execute <unnamed>: select 1-$1
>    DETAIL:  parameters: $1 = '-1'
>    LOG:  execute <unnamed>: select 1-$1
>    DETAIL:  parameters: $1 = '1'
> But if the bound string is '-1' then I get
>    ERROR:  invalid input syntax for integer: "'-1'"   i.e. double quote,single quote,-,1,single quote,double quote
>    STATEMENT:  select 'wibble',1-$1

Huh. I cannot reproduce that. Can you modify the
test/src/param-conversions-test.c test case to demonstrate it?

> Also get invalid syntax error if it is just a minus sign.
>
> ok, server side prepare log when string is just 6 shows
>    LOG:  execute <unnamed>: select '555'>$1
>    DETAIL:  parameters: $1 = '6'
> and this return one row with a value of 0
> so this is doing select '555'>'6';
>
> So to be consistent with what the server side prepare does, I think it should quote the value.
> You should check this as I may have been getting confused.
> I realise this may have some implications for existing programs but given that sometimes it will be server side
prepareand sometimes client side it is likely to cause more issues if the results are different. 

Ugh, I didn't realize that happens with UseServerSidePrepare=1. But
looking at the code, it's quite obvious that it does; we never send the
datatypes with server-side parameters, so the server always treats them
as "unknown", that is, the same as a plain string literal. That usually
works fine, as the server deduces the right datatype from the context,
but for ambiguous cases like above, it goes wrong.

I think it's pretty clear that the UseServerSidePrepare=1 behaviour is
wrong in this case. In particular, if you bind a parameter as
SQL_INTEGER, and run query "SELECT '555' > ?", the argument should be
treated as an integer and the answer should be "true".

I propose the attached two patches. The first one fixes the "-" case and
negative integers, with UseServerSidePrepare=0. The second one changes
the UseServerSidePrepare=1 behaviour so that a datatype is sent along
with each parameter. String-like datatypes, SQL_CHAR, SQL_WCHAR,
SQL_VARCHAR etc. are still sent as "unknown". It makes sense for string
types IMHO.

Thoughts?
- Heikki
h

Heikki Linnakangas wrote:

> >I have had a quick look over the change and it looks ok to me. Something of a clean up and simplification as well.
> >If I understand it correctly, the only things that don't get quoted are SQL_INTEGER and SQL_SMALLINT that pass the
newvalid_int_literal() test. 
> >The only thing I can see that could pass that test and not be a valid integer would be a single minus char i.e. "-"
> >not sure if there is anyway that could be vulnerable though.
>
> Ah, good catch. That is definitely a problem. Consider:
>
> SELECT * FROM foo WHERE 1-? > 0
>
> If you replace ? with -, it becomes "--", which comments out the rest of the
> query. That's actually a problem with any negative number.
>
> It would be tempting to just always quote the value, but that again would
> lead to subtle changes in the datatype that the server chooses.

Maybe you can "quote" it with whitespace, so that it becomes

SELECT * FROM foo WHERE 1- -1  > 0

which is no longer a comment and has no other side effect.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 01/15/2015 06:02 PM, Alvaro Herrera wrote:
> Heikki Linnakangas wrote:
>
>>> I have had a quick look over the change and it looks ok to me. Something of a clean up and simplification as well.
>>> If I understand it correctly, the only things that don't get quoted are SQL_INTEGER and SQL_SMALLINT that pass the
newvalid_int_literal() test. 
>>> The only thing I can see that could pass that test and not be a valid integer would be a single minus char i.e. "-"
>>> not sure if there is anyway that could be vulnerable though.
>>
>> Ah, good catch. That is definitely a problem. Consider:
>>
>> SELECT * FROM foo WHERE 1-? > 0
>>
>> If you replace ? with -, it becomes "--", which comments out the rest of the
>> query. That's actually a problem with any negative number.
>>
>> It would be tempting to just always quote the value, but that again would
>> lead to subtle changes in the datatype that the server chooses.
>
> Maybe you can "quote" it with whitespace, so that it becomes
>
> SELECT * FROM foo WHERE 1- -1  > 0
>
> which is no longer a comment and has no other side effect.

Hmm. Strictly speaking, -1 is interpreted as -(1) by the server. Usually
it doesn't make any difference, but see:

postgres=# select -32768::smallint;
ERROR:  smallint out of range
postgres=# select (-32768)::smallint;
   int2
--------
  -32768
(1 row)

It also affects the automatically chosen column name:

postgres=# select -1::int4;
  ?column?
----------
        -1
(1 row)

postgres=# select (-1)::int4;
  int4
------
    -1
(1 row)

On the whole, using parens seems better.

- Heikki

On Thu, Jan 15, 2015 at 7:56 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
> On 01/14/2015 06:56 PM, Faith, Jeremy wrote:
> It would be nice to get more people to test the libpq changes before that,
> but to be honest, we're not going to get any more testing than what's
> already been done until we make a release and people start using it.
FWIW, we do perform tests on latest HEAD in some branches dedicated to
dev. That's not a zero-bug insurance, still..
--
Michael

Don't worry about the '-1' test I was just trying to determine, in the server side prepare case, if it was adding
quotesi.e. the string I was supplying already had single quotes and was not accepted. 

I've had a quick look at the new patches.

patch 0001
To be extra extra careful maybe valid_int_literal() should check 'len' before accessing the first byte and second bytes
of'str' in case len is 0 or 1. This may not be necessary it depends on where 'str' comes from. 
Otherwise the patch looks fine but maybe you should add a couple of test cases for SQL_SMALLINT.


patch 0002
In convert.c you have added the comment and line
   /* XXX: should we use param_pgtype here instead? */
   *pgType = sqltype_to_bind_pgtype(conn, param_sqltype);
this really requires someone with more in depth knowledge than I have.
Again maybe a couple of test cases for SQL_SMALLINT.

I have done a few very quick tests myself and have not found anything wrong so far.

Looks like I opened up a can of worms here, oops, but the end result should be more secure and more consistent with
regardsto the server side and client side prepare behaviour, which is good. Also your helping people out by forcing
integercomparison more often. 
I will try to do some more testing next week.

Regards,
Jeremy Faith



________________________________________

From: Heikki Linnakangas [hlinnakangas@vmware.com]
Sent: 15 January 2015 15:58
To: Faith, Jeremy; pgsql-odbc@postgresql.org
Subject: Re: [ODBC] SQL_CURSOR_TYPE prepare execute issue

On 01/15/2015 02:22 PM, Faith, Jeremy wrote:
> As long as you are confident that the test suite is checking this code path that is fine, I just wanted to be sure
thatwas happening. 

Sure, thanks!

> I hadn't thought of
>    select x-?
> You are right that is a problem.
> What does the server prepare code path do?
> I did a quick check, sever side prepare, with the log turned on and it shows the following
>    LOG:  execute <unnamed>: select 1-$1
>    DETAIL:  parameters: $1 = '-1'
>    LOG:  execute <unnamed>: select 1-$1
>    DETAIL:  parameters: $1 = '1'
> But if the bound string is '-1' then I get
>    ERROR:  invalid input syntax for integer: "'-1'"   i.e. double quote,single quote,-,1,single quote,double quote
>    STATEMENT:  select 'wibble',1-$1

Huh. I cannot reproduce that. Can you modify the
test/src/param-conversions-test.c test case to demonstrate it?

> Also get invalid syntax error if it is just a minus sign.
>
> ok, server side prepare log when string is just 6 shows
>    LOG:  execute <unnamed>: select '555'>$1
>    DETAIL:  parameters: $1 = '6'
> and this return one row with a value of 0
> so this is doing select '555'>'6';
>
> So to be consistent with what the server side prepare does, I think it should quote the value.
> You should check this as I may have been getting confused.
> I realise this may have some implications for existing programs but given that sometimes it will be server side
prepareand sometimes client side it is likely to cause more issues if the results are different. 

Ugh, I didn't realize that happens with UseServerSidePrepare=1. But
looking at the code, it's quite obvious that it does; we never send the
datatypes with server-side parameters, so the server always treats them
as "unknown", that is, the same as a plain string literal. That usually
works fine, as the server deduces the right datatype from the context,
but for ambiguous cases like above, it goes wrong.

I think it's pretty clear that the UseServerSidePrepare=1 behaviour is
wrong in this case. In particular, if you bind a parameter as
SQL_INTEGER, and run query "SELECT '555' > ?", the argument should be
treated as an integer and the answer should be "true".

I propose the attached two patches. The first one fixes the "-" case and
negative integers, with UseServerSidePrepare=0. The second one changes
the UseServerSidePrepare=1 behaviour so that a datatype is sent along
with each parameter. String-like datatypes, SQL_CHAR, SQL_WCHAR,
SQL_VARCHAR etc. are still sent as "unknown". It makes sense for string
types IMHO.

Thoughts?
- Heikki
h
Tyco Safety Products/CEM Systems Ltd.

________________________________

This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you
arenot the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any
actionin respect of any information contained in it. If you have received this e-mail in error, please notify the
senderimmediately by e-mail and immediately destroy this e-mail and its attachments. 

On 01/16/2015 02:03 AM, Michael Paquier wrote:
> On Thu, Jan 15, 2015 at 7:56 PM, Heikki Linnakangas
> <hlinnakangas@vmware.com> wrote:
>> On 01/14/2015 06:56 PM, Faith, Jeremy wrote:
>> It would be nice to get more people to test the libpq changes before that,
>> but to be honest, we're not going to get any more testing than what's
>> already been done until we make a release and people start using it.
> FWIW, we do perform tests on latest HEAD in some branches dedicated to
> dev. That's not a zero-bug insurance, still..

Right - and that's great - but my point was that we're not going to get
much more testing than what's already been done, by waiting.

- Heikki

On 01/16/2015 09:21 PM, Faith, Jeremy wrote:
> Don't worry about the '-1' test I was just trying to determine, in the server side prepare case, if it was adding
quotesi.e. the string I was supplying already had single quotes and was not accepted. 
>
> I've had a quick look at the new patches.
>
> patch 0001
> To be extra extra careful maybe valid_int_literal() should check 'len' before accessing the first byte and second
bytesof 'str' in case len is 0 or 1. This may not be necessary it depends on where 'str' comes from. 

Ah, you're right. Fixed.

> Otherwise the patch looks fine but maybe you should add a couple of test cases for SQL_SMALLINT.

Added.

> patch 0002
> In convert.c you have added the comment and line
>     /* XXX: should we use param_pgtype here instead? */
>     *pgType = sqltype_to_bind_pgtype(conn, param_sqltype);
> this really requires someone with more in depth knowledge than I have.

I don't fully understand that either. It's not clear to me when
param_pgtype is set, and where the value comes from. I decided to ignore
it for now, which makes on which datatype is chosen simpler, and I think
is closer to the old behaviour.

> Again maybe a couple of test cases for SQL_SMALLINT.

Added.

> I have done a few very quick tests myself and have not found anything wrong so far.
>
> Looks like I opened up a can of worms here, oops, but the end result should be more secure and more consistent with
regardsto the server side and client side prepare behaviour, which is good. Also your helping people out by forcing
integercomparison more often. 
> I will try to do some more testing next week.

Thanks, I've pushed these changes now.

- Heikki