sankarNarayanan / modwsgi

Automatically exported from code.google.com/p/modwsgi
0 stars 0 forks source link

Adapter_start_response does not check type of exc_info, so could pass an invalid object along. #220

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
We were doing this: 

 * Exception being raised inside application (we are using Trac)
 * Call to trac.web.main.dispatch_request() is wrapped in exception handler, which:
 * Calls start_response('500 Server Error', [('Content-type', 'text/html')], sys.exc_info)

Note the invalid final argument. This should be sys.exc_info().

This causes an invalid call to PyArg_ParseTuple, as shown in this backtrace:

#2  0x00002aaab53f2d65 in vgetargs1 (args=0x2aaaaaafc1b8, format=0x2aaab589b7f3 
"OOO", p_va=0x7fff33b93c40, 
    compat=0) at Python/getargs.c:229
#3  0x00002aaab53f2e3a in PyArg_ParseTuple (args=0x5035, format=0x5035 <Address 
0x5035 out of bounds>)
    at Python/getargs.c:54
#4  0x00002aaab589ad7e in Adapter_start_response (self=0x555555ee1f30, 
args=<value optimized out>)
    at mod_wsgi.c:2987
#5  0x00002aaab53df0ba in call_function (f=0x55555591c160) at 
Python/ceval.c:3563
#6  PyEval_EvalFrame (f=0x55555591c160) at Python/ceval.c:2163
#7  0x00002aaab53dffe5 in PyEval_EvalCodeEx (co=0x2aaaaab5b810, globals=<value 
optimized out>, 
    locals=<value optimized out>, args=0x555556d923c8, argcount=2, kws=0x0, kwcount=0, defs=0x0, defcount=0, 
    closure=0x0) at Python/ceval.c:2736
#8  0x00002aaab5396367 in function_call (func=0x555556e28f50, 
arg=0x555556d923b0, kw=0x0)
    at Objects/funcobject.c:548
#9  0x00002aaab53800f0 in PyObject_Call (func=0x5035, arg=0x5035, kw=0x6) at 
Objects/abstract.c:1795
#10 0x00002aaab53d9c3d in PyEval_CallObjectWithKeywords (func=0x555556e28f50, 
arg=0x555556d923b0, kw=0x0)
    at Python/ceval.c:3430
#11 0x00002aaab5892d62 in Adapter_run (self=0x555555ee1f30, 
object=0x555556e28f50) at mod_wsgi.c:3888
#12 0x00002aaab58938e9 in wsgi_execute_script (r=0x555556e986d0) at 
mod_wsgi.c:6783
#13 0x00002aaab58952ba in wsgi_hook_handler (r=0x555556e986d0) at 
mod_wsgi.c:9488
#14 0x000055555557da4a in ap_run_handler ()
#15 0x0000555555580ed8 in ap_invoke_handler ()
#16 0x000055555558b938 in ap_process_request ()
#17 0x0000555555588b70 in ?? ()
#18 0x0000555555584cd2 in ap_run_process_connection ()
#19 0x000055555558f789 in ?? ()
#20 0x000055555558fa1a in ?? ()
#21 0x000055555558fad0 in ?? ()
#22 0x00005555555907bb in ap_mpm_run ()
#23 0x000055555556ae48 in main ()

and we were seeing printed:

  SystemError: new style getargs format but argument is not a tuple 

Recommend that near this code:

    if (exc_info && exc_info != Py_None) {
    if (self->status_line && !self->headers) {
            PyObject *type = NULL;
            PyObject *value = NULL;
            PyObject *traceback = NULL;

            if (!PyArg_ParseTuple(exc_info, "OOO", &type,
                                  &value, &traceback)) {
                Py_XDECREF(latin_item);
        return NULL;
            }

the type of exc_info is checked so a TypeError could be thrown early. The 
SystemError (with hindsight) does say that something is not a tuple, but it 
wasn't that clear to me as the name of the error suggests a C-level problem.

I'd have found it really useful if that PyArg_ParseTuple had a function 
name/comment in, such as "OOO:start_response exc_info parse" (although I don't 
know if spaces are allowed...), as it took a while to track this down. I ended 
up rebuilding Python and mod_wsgi with debugging, patching to get a core dump 
at the right place, etc. :)

Original issue reported on code.google.com by nick.pi...@logica.com on 7 Jan 2011 at 8:45

GoogleCodeExporter commented 8 years ago
In general, error handling that tries to account for every weird way people can 
get something wrong can just get too complicated. All the same, I'll leave the 
issue here and look at it at some point and see if any improvements can be 
made. One thing that may be done is that Python does have the ability to add 
validators for 'O' arguments so you can say that is expected that they be 
certain types.

Original comment by Graham.Dumpleton@gmail.com on 8 Jan 2011 at 12:07

GoogleCodeExporter commented 8 years ago
Agree that in the end the fault was entirely our own; we were just mislead by 
the type and wording of the exception "SystemError: new style getargs format 
[...]". Because it's talking about "new style getargs" this hinted at some 
C-level fault and led us astray in the debugging :-)

I did think it might have been our start_response(...) call, but was confused 
why we'd get this error (expecting that it was coming from the _first_ parse of 
the arguments, not a later second-parse of the final argument.)

If it can be checked that the final O is really a tuple, it might save someone 
else in the future a lot of time.

Thanks for considering it!

Original comment by nick.pi...@logica.com on 8 Jan 2011 at 11:54

GoogleCodeExporter commented 8 years ago
Looking at code now, yes it was an omission that basic type check for tuple 
wasn't being done.

This is fixed now in revision 1754 of trunk for mod_wsgi 4.0.

The code now does:

    if (!PyArg_ParseTuple(args, "OO!|O!:start_response",
        &item, &PyList_Type, &headers, &PyTuple_Type, &exc_info)) {
        return NULL;
    }

That is, check that type of argument is tuple when first passed in to 
start_response().

The error you will get now is:

File "/Library/WebServer/Sites/hello-1/htdocs/hello-py3k.wsgi", line 7, in 
application
     start_response(status, response_headers, [])
TypeError: must be tuple, not list

As usual, Python isn't going to tell which argument of function, but is better 
than before.

Original comment by Graham.Dumpleton@gmail.com on 18 Jan 2011 at 6:39

GoogleCodeExporter commented 8 years ago
This is having a side effect on, at least, Python 2.7.1 on Mac OS 10.7.1 
(Python as supplied by Apple) when using, amongst other, the Pylons framework.

What is happening there is that sometimes "None" is passed as exc_info, which 
no longer is allowed with this change to parsing. So it might be better to 
switch back to using the plain "O" format instead of "O!", and then verify that 
either no argument was passed, None was passed or a tuple, and only raise an 
exception if it is none of those...

Original comment by tlock...@gmail.com on 10 Sep 2011 at 9:08

GoogleCodeExporter commented 8 years ago
Yes, am aware of this. I rambled about the issue at:

https://plus.google.com/114657481176404420131/posts/NxMETz1mAUj

As usual, not too much of a response and so had just left it to see when 
someone would actually notice it was a problem so could see which frameworks 
were not necessarily following the letter of the specification. :-)

I have reopened the issue.

Original comment by Graham.Dumpleton@gmail.com on 10 Sep 2011 at 9:18

GoogleCodeExporter commented 8 years ago
The mod_wsgi 4.X repository trunk is now accepting None again for exc_info when 
calling start_response().

Original comment by Graham.Dumpleton@gmail.com on 24 Sep 2011 at 5:49

GoogleCodeExporter commented 8 years ago

Original comment by Graham.Dumpleton@gmail.com on 19 Mar 2012 at 10:24

GoogleCodeExporter commented 8 years ago
Closing out old issue. Believe all is okay in 4.X.

Original comment by Graham.Dumpleton@gmail.com on 12 Nov 2014 at 10:49