Thread: Negative Integers Escaping

Hello everyone!

There is an unclear behaviour in negative integers escaping when they are being passed to specific SQL queries.
Here are some examples:

CREATE TABLE testdb (testval integer not null default 0);

>>> import psycopg2 as p
>>> p.__version__
'2.4 (dt dec pq3 ext)'
>>> c = p.connect(...)
>>> cr = c.cursor()
>>> cr.execute("insert into testdb(testval) values(9)")
>>> c.commit()
>>> cr.execute("select testval from testdb")
>>> cr.fetchall()
[(9,)]

>>> # Ok, we know about required parentheses here because we explicitly type the negative value
>>> cr.execute("update testdb set testval=testval-(-2)")
>>> c.commit()
>>> cr.execute("select testval from testdb")
>>> cr.fetchall()
[(11,)]

>>> # Here we'll get a correct expression but the wrong result caused by the comment sequence '--'
>>> cr.execute("update testdb set testval=testval-%s", (-2,))
>>> c.commit()
>>> cr.execute("select testval from testdb")
>>> cr.fetchall()
[(11,)]

>>> # So we got to explicitly ident or to frame the placeholder with parentheses
>>> cr.execute("update testdb set testval=testval - %s", (-2,))
>>> c.commit()
>>> cr.execute("select testval from testdb")
>>> cr.fetchall()
[(13,)]

>>> # The same behaviour with named placeholders
>>> cr.execute("update testdb set testval=testval-%(val)s", {'val':-2})
>>> c.commit()
>>> cr.execute("select testval from testdb")
>>> cr.fetchall()
[(13,)]

I found no strict rules about this case in DBAPI2 specification. So how negative integers escaping should behave?

El 27/05/2011 19:42, Maxim Avanov escribió:
> Hello everyone!
>
> There is an unclear behaviour in negative integers escaping when they
> are being passed to specific SQL queries.
> Here are some examples:
>
> CREATE TABLE testdb (testval integer not null default 0);
>
>  >>> import psycopg2 as p
>  >>> p.__version__
> '2.4 (dt dec pq3 ext)'
>  >>> c = p.connect(...)
>  >>> cr = c.cursor()
>  >>> cr.execute("insert into testdb(testval) values(9)")
>  >>> c.commit()
>  >>> cr.execute("select testval from testdb")
>  >>> cr.fetchall()
> [(9,)]
>
>  >>> # Ok, we know about required parentheses here because we explicitly
> type the negative value
>  >>> cr.execute("update testdb set testval=testval-(-2)")
>  >>> c.commit()
>  >>> cr.execute("select testval from testdb")
>  >>> cr.fetchall()
> [(11,)]
>
>  >>> # Here we'll get a correct expression but the wrong result caused
> by the comment sequence '--'
>  >>> cr.execute("update testdb set testval=testval-%s", (-2,))
>  >>> c.commit()
>  >>> cr.execute("select testval from testdb")
>  >>> cr.fetchall()
> [(11,)]
>
>  >>> # So we got to explicitly ident or to frame the placeholder with
> parentheses
>  >>> cr.execute("update testdb set testval=testval - %s", (-2,))
>  >>> c.commit()
>  >>> cr.execute("select testval from testdb")
>  >>> cr.fetchall()
> [(13,)]
>
>  >>> # The same behaviour with named placeholders
>  >>> cr.execute("update testdb set testval=testval-%(val)s", {'val':-2})
>  >>> c.commit()
>  >>> cr.execute("select testval from testdb")
>  >>> cr.fetchall()
> [(13,)]
>
> I found no strict rules about this case in DBAPI2 specification. So how
> negative integers escaping should behave?
>

When you do:
     cr.execute("update testdb set testval=testval-%s", (-2,))

Postgresql receive:
     update testdb set testval=testval--2

The double dash is treated as begin sql comment and only execute:

     update testdb set testval=testval

Is a good rule to always put spaces between operators

Regards

--
Oswaldo Hernández

Hi, Oswoldo. Thanks for reply.I agree. It's a good rule but it's neither in SQL nor in Postrges syntax rules. And psycopg should guarantee a valid escaping of parameters according to all possible and valid syntax rules.

On Fri, May 27, 2011 at 8:03 PM, Maxim Avanov <maxim.avanov@gmail.com> wrote:
> Hi, Oswoldo. Thanks for reply.
>> Is a good rule to always put spaces between operators
>
> I agree. It's a good rule but it's neither in SQL nor in Postrges syntax
> rules. And psycopg should guarantee a valid escaping of parameters according
> to all possible and valid syntax rules.

There's plenty of space for creating pathological commands. Do you
want another one?

"select * from blah limit%s"

I think in general sticking characters in front of placeholders you
don't know how will get filled is not a robust way to write your sql
string.

I'm -1 about complicating the escaping of simple values just to
accommodate artificial problems: fixing this one IMO wouldn't justify
the potential problems of backward incompatibilities that may arise.

-- Daniele

On May 27, 2011, at 7:45 PM, Daniele Varrazzo wrote:

> On Fri, May 27, 2011 at 8:03 PM, Maxim Avanov <maxim.avanov@gmail.com> wrote:
>> Hi, Oswoldo. Thanks for reply.
>>> Is a good rule to always put spaces between operators
>>
>> I agree. It's a good rule but it's neither in SQL nor in Postrges syntax
>> rules. And psycopg should guarantee a valid escaping of parameters according
>> to all possible and valid syntax rules.
>
> There's plenty of space for creating pathological commands. Do you
> want another one?
>
> "select * from blah limit%s"
>
> I think in general sticking characters in front of placeholders you
> don't know how will get filled is not a robust way to write your sql
> string.
>
> I'm -1 about complicating the escaping of simple values just to
> accommodate artificial problems: fixing this one IMO wouldn't justify
> the potential problems of backward incompatibilities that may arise.

This only holds if one considers SQL parameters are components to concatenate into the string as psycopg does it today.
Witha real out-of-band parameter system, the above example will not PREPARE/parse. I am strong proponent for the real
parametersystem (which every other postgresql interface (even pg8000) already supports). 

agentm=# PREPARE test(int) AS SELECT$1;
ERROR:  syntax error at or near "SELECT$1" at character 22
STATEMENT:  PREPARE test(int) AS SELECT$1;
ERROR:  syntax error at or near "SELECT$1"
LINE 1: PREPARE test(int) AS SELECT$1;


Cheers,
M

On 28/05/11 01:45, Daniele Varrazzo wrote:
> On Fri, May 27, 2011 at 8:03 PM, Maxim Avanov <maxim.avanov@gmail.com> wrote:
>> > Hi, Oswoldo. Thanks for reply.
>>> >> Is a good rule to always put spaces between operators
>> >
>> > I agree. It's a good rule but it's neither in SQL nor in Postrges syntax
>> > rules. And psycopg should guarantee a valid escaping of parameters according
>> > to all possible and valid syntax rules.
> There's plenty of space for creating pathological commands. Do you
> want another one?
>
> "select * from blah limit%s"
>
> I think in general sticking characters in front of placeholders you
> don't know how will get filled is not a robust way to write your sql
> string.
>
> I'm -1 about complicating the escaping of simple values just to
> accommodate artificial problems: fixing this one IMO wouldn't justify
> the potential problems of backward incompatibilities that may arise.

Sorry, but I don't agree. SQL rules explicitly say that LIMITX is
invalid for any X because LIMIT should be separated from its argument by
white space; so you're writing incorrect SQL from the start.

A mathematical expression doesn't need, at least in SQL, any whitespace
so, writing colname-%s is *correct* and the programmer is correct when
expects the DB adapter to quote the arguments to make sure they don't
introduce any new errors in SQL.

federico

--
Federico Di Gregorio                         federico.digregorio@dndg.it
Studio Associato Di Nunzio e Di Gregorio                  http://dndg.it
             Quis custodiet ipsos custodes? -- Juvenal, Satires, VI, 347

On Sun, May 29, 2011 at 9:04 PM, Federico Di Gregorio
<federico.digregorio@dndg.it> wrote:

> A mathematical expression doesn't need, at least in SQL, any whitespace
> so, writing colname-%s is *correct* and the programmer is correct when
> expects the DB adapter to quote the arguments to make sure they don't
> introduce any new errors in SQL.

Groan... https://github.com/dvarrazzo/psycopg/commit/281427f450d6e9755d4c3cbc9fb159d45ca10ee6

tl:dr: added space in front of negative sign for all the numeric
types. With this patch the unit test doesn't report any failure, so I
hope no regression.

-- Daniele

On Sun, May 29, 2011 at 11:04 PM, Federico Di Gregorio
<federico.digregorio@dndg.it> wrote:
> On 28/05/11 01:45, Daniele Varrazzo wrote:
>> On Fri, May 27, 2011 at 8:03 PM, Maxim Avanov <maxim.avanov@gmail.com> wrote:
>>> > Hi, Oswoldo. Thanks for reply.
>>>> >> Is a good rule to always put spaces between operators
>>> >
>>> > I agree. It's a good rule but it's neither in SQL nor in Postrges syntax
>>> > rules. And psycopg should guarantee a valid escaping of parameters according
>>> > to all possible and valid syntax rules.
>> There's plenty of space for creating pathological commands. Do you
>> want another one?
>>
>> "select * from blah limit%s"
>>
>> I think in general sticking characters in front of placeholders you
>> don't know how will get filled is not a robust way to write your sql
>> string.
>>
>> I'm -1 about complicating the escaping of simple values just to
>> accommodate artificial problems: fixing this one IMO wouldn't justify
>> the potential problems of backward incompatibilities that may arise.
>
> Sorry, but I don't agree. SQL rules explicitly say that LIMITX is
> invalid for any X because LIMIT should be separated from its argument by
> white space; so you're writing incorrect SQL from the start.
>
> A mathematical expression doesn't need, at least in SQL, any whitespace
> so, writing colname-%s is *correct* and the programmer is correct when
> expects the DB adapter to quote the arguments to make sure they don't
> introduce any new errors in SQL.

And the proper fix is quite well-known: $x placeholders
and Extended Query protocol.

Any other kind of query massaging seems inappropriate.

--
marko

On Tue, May 31, 2011 at 12:47 PM, Marko Kreen <markokr@gmail.com> wrote:

> And the proper fix is quite well-known: $x placeholders
> and Extended Query protocol.

Do you have ideas about how to avoid breaking applications where
people have used multiple queries in an execute? Or how to pass arrays
in a robust way when the suggested method (ARRAY[] construct) doesn't
work with PQexecParams? Or to make IN work?

I've already called for discussion a couple of months ago [1] about
supporting the EQ protocol: it will eventually be done, but the result
will hardly be a complete replacement for what psycopg currently does,
so don't see it becoming the default escape mechanism. (Of course,
while I'm positive about its implementation, nobody has stepped ahead
for implementing it, so I'm afraid it will have to wait for a slice of
my Copious Spare Time).

While it's good stuff the EQ exists for applications directly using
the libpq, It wouldn't have saved many troubles for psycopg: IMO this
one is really borderline to a pathological case and is not a security
issue.

If there was a psycopg3 to be written with no backward compatibility
to maintain, the EQ protocol would be a reasonable choice. Of course
it wouldn't offer the above features, no mogrify(), no cursor.query
and so on, but that would be tolerable for a new driver with a
different features set. But just putting EQ into psycopg2 simply means
dropping features and causing problems to a random bunch of users: I
don't see dropping such features just to use the EQ a victory overall.

-- Daniele

[1] http://archives.postgresql.org/psycopg/2011-02/msg00076.php

On 31/05/11 18:56, Daniele Varrazzo wrote:
> On Tue, May 31, 2011 at 12:47 PM, Marko Kreen <markokr@gmail.com> wrote:
[snip]
> I've already called for discussion a couple of months ago [1] about
> supporting the EQ protocol: it will eventually be done, but the result
> will hardly be a complete replacement for what psycopg currently does,
> so don't see it becoming the default escape mechanism. (Of course,
> while I'm positive about its implementation, nobody has stepped ahead
> for implementing it, so I'm afraid it will have to wait for a slice of
> my Copious Spare Time).

Lucky you! Mine ISN'T Copious. :D

> While it's good stuff the EQ exists for applications directly using
> the libpq, It wouldn't have saved many troubles for psycopg: IMO this
> one is really borderline to a pathological case and is not a security
> issue.

Also this one can generically be solved by putting parentheses around
every single argument. It is a +2 bytes per argument and the output of
cursor.query isn't pretty at all but if the need arise that will work
with minimal changes to the code (i.e., no new bugs).

Btw, I completely agree with Daniele's analisys of EQ and psycopg.
psycopg offers a lot of features and we shoudl find the right place for
EQ. Just dropping it in and have regressions on the existing code isn't
a good idea.

federico

--
Federico Di Gregorio                         federico.digregorio@dndg.it
Studio Associato Di Nunzio e Di Gregorio                  http://dndg.it
  Lord, defend me from my friends; I can account for my enemies.
                                                  -- Charles D'Hericault