search

FSX / momoko

Wraps (asynchronous) Psycopg2 for Tornado.
http://momoko.61924.nl/
Other
364 stars 73 forks source link

fix for any uncatchable internal exceptions #134

Closed m-messiah closed 8 years ago

m-messiah commented 8 years ago

There is the problem, when momoko fails with exception, such as, for example, IndexError for parameters, which is not catchable by Tornado. In the result of this behavior - this connection stays busy and inoperable. So, this little patch would catch these exceptions and throw them to Tornado

haizaar commented 8 years ago

Thanks for reporting the issue. Can you please the working reproduction? Ideally, a failing unittest?

m-messiah commented 8 years ago

So, when this case is so strange and rare, and maybe you can say - this code is bad for using - problem still exists - little bug in one of threads of pool can produce uncatchable exception and crash all app.

m-messiah commented 8 years ago

Ok, then, RuCTFE 2015 is over and I can show you piece of code, which fails here. (except unittest, published earlier). Here, https://github.com/HackerDom/ructfe-2015/blob/master/services/mol/service/main.py#L458 , we have sql injection (in unittest I had written better way, when it may be needed), which works perfectly except one input:

when variable 'offset' is sended by another python code, where it looks like 

offset = "0; SELECT * FROM users WHERE username LIKE '%%='"

where %% used to prevent string interpolation.

So, when this input received - this thread of momoko.Pool throws uncatchable exception and doesn't close till application restart.

haizaar commented 8 years ago

I think I see what you are saying - cursor.execute can throw non psycopg2.Error based exception as well. The proper was to fix it is on the Pool level - if you are using Connection stand-alone, the error is propagated just fine to the caller.

Can you try my master branch haizaar/momoko@7dcc245? I've fixed it there.

m-messiah commented 8 years ago

My environment shows a lot of failed tests, so I can't recognize is problem still exists, or it is gone((

The main test, which failed for me is:

search = "WHERE name LIKE '%%searchable' LIMIT %s" % 10 sql = yield self.conn.execute("SELECT COUNT(*) FROM unittest_large_query %s;" % search)

Is it still fails, or your fix repaired it? I see, your test is about some different case, which was catchable for me earlier.

haizaar commented 8 years ago

My fix works for your case as well - they both raise IndexError. Mine version is dead stupid to outline that we pass malformed string deliberately.

I'm merging mine and closing this then.

Thanks for your time to find it out!