OperationalError: near "t": syntax error (Trac 0.11.6)

[anonymous]

#7610: OperationalError: near "t": syntax error

---

Reporter: frederic.olivie@… | Owner:

Type: defect | Status: new

Priority: normal | Milestone: not applicable

Component: general | Version: 0.11rc2

Severity: normal | Keywords:

---

How to Reproduce

While doing a GET operation on /pdashboard, Trac issued an internal error.

(please provide additional details here)

Request parameters:

{'imagename': None}

User Agent was: `Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.16) Gecko/20080702 Firefox/2.0.0.16`

System Information

20061115 (prerelease) (Debian 4.1.1-21)]`

Trac	0.11rc2
Python	2.4.4 (#1, Apr 16 2008, 17:56:48) `[GCC 4.1.2
setuptools	0.6c8
SQLite	3.3.8
pysqlite	2.3.2
Genshi	0.6dev-r884
mod_python	3.2.10
Subversion	1.4.2 (r22196)
jQuery:	1.2.3

Python Traceback

Traceback (most recent call last):
  File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/web/main.py", line 423, in
_dispatch_request
    dispatcher.dispatch(req)
  File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/web/main.py", line 197, in dispatch
    resp = chosen_handler.process_request(req)
  File "build/bdist.linux-x86_64/egg/tracmetrixplugin/web_ui.py", line
122, in process_request
  File "build/bdist.linux-x86_64/egg/tracmetrixplugin/web_ui.py", line
165, in _render_view
  File "build/bdist.linux-x86_64/egg/tracmetrixplugin/model.py", line 94,
in get_ticket_resolution_group_stats
  File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/db/util.py", line 51, in execute
    return self.cursor.execute(sql)
  File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/db/sqlite_backend.py", line 58, in
execute
    args or [])
  File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/db/sqlite_backend.py", line 50, in
_rollback_on_error
    return function(self, *args, **kwargs)
OperationalError: near "t": syntax error

[Bhuricha Deen Sethanandha]

I can't reproduce this error. Please use the trunk version and try again.

[Ryan J Ollos]

This is probably no longer valid with a stable release of Trac.

[paul.amor@…]

I can confirm that this issue definitely still exists when installed from the trunk, though the line numbers of the trace are slightly different the route is the same. If you need further info please let me know

[Ryan J Ollos]

Could you please test again with the latest version of the Trunk? If you still see the error, please send the debug trace and details of your installation. Thanks.

[Ryan J Ollos]

Replying to paul.amor@mi-pay.com:

I can confirm that this issue definitely still exists when installed from the trunk

In particular, which database are you using?

Ticket will be closed in one month if no response.

[jnelson@…]

I also have this with a 0.11.7 install of Trac, using the TracMetrixPlugin installer offered from the Download link on the Trac page (​http://trac-hacks.org/wiki/TracMetrixPlugin).

Have since upgraded Trac to 0.12 with no change on the error, although now it's unclear from the site that this plugin is 0.12 compatible.

[jnelson@…]

(Please disregard obviously dumb comment about 0.12 compatibility)

As an update, have just finished installing 0.12 version of this plugin. On doing the trac-admin upgrade call, the only spew I got was "Database is up to date, no upgrade necessary." Did not get "TracMetrixPlugin needs an upgrade" as I was told to expect. Still getting same error message as stated in this bug.

[jnelson@…]

Traceback and system information:

Python Traceback
Most recent call last:
File "build/bdist.linux-i686/egg/trac/web/main.py", line 513, in _dispatch_request
File "build/bdist.linux-i686/egg/trac/web/main.py", line 235, in dispatch
File "build/bdist.linux-i686/egg/tracmetrixplugin/web_ui.py", line 181, in process_request
File "build/bdist.linux-i686/egg/tracmetrixplugin/web_ui.py", line 228, in _render_view
File "build/bdist.linux-i686/egg/tracmetrixplugin/model.py", line 95, in get_ticket_resolution_group_stats
File "build/bdist.linux-i686/egg/trac/db/util.py", line 66, in execute
File "build/bdist.linux-i686/egg/trac/db/sqlite_backend.py", line 78, in execute
File "build/bdist.linux-i686/egg/trac/db/sqlite_backend.py", line 56, in execute
File "build/bdist.linux-i686/egg/trac/db/sqlite_backend.py", line 48, in _rollback_on_error
 

System Information:
User Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.63 Safari/534.3

 Trac	0.12
 Genshi	0.6
 pysqlite	2.4.0
 Python	2.5.2 (r252:60911, Jan 20 2010, 21:48:48) [GCC 4.2.4 (Ubuntu 4.2.4-1ubuntu3)]
 pytz	2007k
 RPC	1.0.6
 setuptools	0.6c8
 SQLite	3.4.2
 Subversion	1.4.6 (r28521)
 jQuery	1.4.2

[jnelson@…]

I figured it out. Your code can't handle a tick mark in a bug resolution. I had a resolution value "Won't fix", and that was breaking it.

[Ryan J Ollos]

Issue confirmed when replacing default Trac resolution wontfix with won't fix. Thanks for tracking this down ... it was a tricky issue, no doubt.

[Ryan J Ollos]

(In [9166]) Replace possible single-quotes in resolution strings with double single-quotes, as required for SQLite queries (see ​http://www.sqlite.org/faq.html#q14). Refs #3684.

[Ryan J Ollos]

(In [9167]) Merged [9166] into 0.12 branch. Fixes #3684.

[osimons]

Just noticed this change by accident, but had to speak up as this is bad code practice...:

Trac and the underlying connector does all the needed escaping for you if used correctly - and 'correctly' is not by using string interpolation - depending in making right strings for all platforms will quite likely leave holes for SQL injection attacks.

The method cursor.execute is defined as cursor.execute(self, sql, args=None), and you identify any arg in sql as a plain %s regardless of type - type is looked up by the underlying mechanisms that translates correctly between underlying db connectors (usually the same, but may be different). It will take care of mapping, escaping and all that is needed. Review any SELECT in the main Trac code base to see more examples of usage with arguments.

So, it should be like this (simplified):

cursor.execute("SELECT * FROM ticket WHERE resolution=%s", [type.name])

BTW, type is a Python built-in name, and re-defining it as loop key is bad practice and may well get you in trouble - the newly defined type will exist for the duration of the method and if anything else further down depends on the original type they will be disappointed and barf errors...

I don't use the plugin, and haven't really seen more code than the changeset linked to above. However, I'm reopening the ticket as it really needs fixing - sorry to butt in... :-)

[Ryan J Ollos]

Osimons, thank you for the feedback! I'm glad this is all handled more elegantly in Trac than the ugly fix that I put in place, for lack of knowing any better. I'll commit a fix shortly and appreciate if you can review that as well.

[osimons]

See ​trac:wiki:TracDev/DatabaseApi for much more details about DB layer.

[Ryan J Ollos]

I spent some time reading ​t:wiki:TracDev/DatabaseApi#RulesforDBAPIUsage and studying the Trac source code, and have refactored the get_ticket_resolution_group_stats method accordingly. There is a lot more work to do on this plugin however, as just the two mis-uses that Osimons mentioned are extensive.

Osimons, if you have a chance to take a quick look at the changes, I'd appreciate any feedback.

I'm a bit puzzled by this use case, from get_ticket_group_stats in roadmap.py (Trac 0.11.7):

str_ids = [str(x) for x in sorted(ticket_ids)]
cursor.execute("SELECT status, count(status) FROM ticket "
               "WHERE id IN (%s) GROUP BY status" %
               ",".join(str_ids))

This seems to go against the rule of don't use string formatting for anything other than dynamically specifying the names of tables and columns. However, I don't see any example in the Trac code where a list of integers is passed as an argument in a call to execute, so I can't see another way to accomplish this.

[Ryan J Ollos]

(In [9463]) Refactoring of get_ticket_resolution_group_stats following suggestions by Osimons. Refs #3684.

[Ryan J Ollos]

Note to self: merge these changes into the 0.12 branch after feedback.

[osimons]

For the string interpolation example, what you need to do is make the string look like... IN (%s, %s, %s, %s) - with one %s for each value. Then all the actual values goes into the args list for replacement by cursor.execute(). Something like this:

cursor.execute("SELECT status, count(status) FROM ticket "
               "WHERE id IN (%s) GROUP BY status" %
               ",".join(['%s']*len(ticket_ids)), sorted(ticket_ids))

The roadmap.py example you found does not do it quite correctly, but that is likely also a concious decision as any input for this particular execute is just guaranteed to be existing ticket ids (safe numbers auto-generated by Trac) and therefore no user input that ever needs escaping. The moment you add resolution= (like in your changeset), that is a value that is input by users and must be escaped.

The good practice is always to escape properly though, so that you or other maintainer later do not need to research the origin of each possible input. Escape and forget :-)

[Ryan J Ollos]

(In [9466]) Format SQL string so that it is escaped properly. Refs #3684.

[Ryan J Ollos]

Replying to osimons:

The good practice is always to escape properly though, so that you or other maintainer later do not need to research the origin of each possible input. Escape and forget :-)

It's starting to make a lot of sense now. Thanks for your advice and detailed explanation! Hopefully the code is looking better.

Looks like there are a couple more SQL query strings and uses of type as a variable name, so I'll take care of those tomorrow and then close this ticket.

[Ryan J Ollos]

Replying to osimons:

One more quick question. Does your use of sorted have any significance such as speeding up the query? I couldn't immediately see a reason for it, but I'm also very new to working with databases, so its all new to me.

[osimons]

Replying to rjollos:

One more quick question. Does your use of sorted have any significance such as speeding up the query? I couldn't immediately see a reason for it, but I'm also very new to working with databases, so its all new to me.

I just left it in seeing the code you found had it. I doubt you'd notice any difference, so the net cost of sorting first is likely negative. However, if you turn on SQL debug logging then the output in order will be more "developer friendly" when reading :-)

[Ryan J Ollos]

(In [9563]) Refactored SQL queries so that strings were properly formatted for escaping by the underlying API. Fixes #3684.