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
