New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
buffer overflow fix #182
buffer overflow fix #182
Conversation
Nice work, @skinowski and thanks for contributing! I wonder if you think it's possible to write an automated test that can detect the issues that you're fixing? |
Thank you. The build failed it looks like, is it something to do with my fork? I'll see how I can put together a test case for this. |
Oh, please ignore the build failure. Yes, it's because we use special "secure environment variables" for Travis CI and those aren't supported in PRs from forks. If I push your branch to our repo, it might build. Let me try... |
@skinowski: Indeed your change passed the Travis CI build when I pushed the branch to the main repo: https://travis-ci.org/pymssql/pymssql/builds/18383974 Now if you put together a few tests, I think we can merge this. |
I'm having difficulty reproducing this issue in my unit test. I've tried putting up a mock TCP server and played around with sending TCP RST or ignoring incoming SYN. But tests are still passing. Originally, when I reproduced the problem, it occurred after running series of stress/regression tests using 30-40 connections connecting to a two SQL server IPs. During the tests, I was manipulating iptables on CentOS to drop traffic randomly. It failed the gcc FORTIFY_CODE=2 tests and caused a core dump. Easier way of testing this could be to expose err_handler() and call it with cooked data. What do you think? |
Yeah for the purposes of an automated test, it is too much trouble to set up mock TCP servers and manipulate iptables (though I'm very glad and impressed that you did that to test pymssql and uncover this issue!) Calling |
How about something like this?
I need to add some more code to be able to pass NULL strings. Maybe another wrapper for msg_handler() as well. I did get the core dump using this without connecting to any database. :-) What do you think? |
just noticed something else, shouldn't gil be released before calling |
Oh interesting, you wrote a test in Cython -- I wonder if we can have py.test run Cython tests? |
It's basically a wrapper function where I would call it from a unitest python file in tests directory using various arguments simulating db, os errors, etc. |
Yeah I was just looking at it again and I started to think that's what it was. Thanks for clarifying. This looks promising. |
Yeah it does seem like the gil should be released before calling |
All right, so the unittest file is like this;
|
It works. :-) Unpatched pymssql gets this;
|
Pushed a fix for nogil on dbopen. |
Nice! Thank you! I'll try to take a look soon. |
This is looking pretty good, though I noticed some test failures with Python 3: https://travis-ci.org/pymssql/pymssql/jobs/18916580 It's expecting one or more of the params to be bytes instead of str. |
Python 3!!! :-) I'll take a look at this soon...
|
It would be also cool to make |
This may be helpful: http://docs.cython.org/src/tutorial/strings.html#encoding-text-to-bytes |
Fix buffer overflow stuff for Python 3
Merged. Thank you so much for your contribution!!! |
Fix for:
buffer overrun in error handler #181