NameError: global name 'rec' is not defined in trac/web/_fcgi.py line 808

[dwhite@…]

There is a variable name typo in trac/web/_fcgi.py line 808 in Trac 0.11.3. This is triggered by attaching a file over a ~50KB to a Trac ticket. The user sees their brower hang on 'waiting for response' from the page. ktrace sees trac write a 500 ISE message to the fastcgi socket.

Environment:

Python 2.4.5 on FreeBSD 7.0-RELEASE-p2; trac running via fastcgi in lighttpd 1.4.21

Exception/traceback from the trac log:

Traceback (most recent call last):
  File "./trac/web/main.py", line 435, in _dispatch_request
  File "./trac/web/main.py", line 166, in dispatch
  File "./trac/attachment.py", line 358, in match_request
  File "./trac/web/api.py", line 194, in __getattr__
  File "./trac/web/api.py", line 475, in _parse_args
  File "/usr/local/lib/python2.4/cgi.py", line 530, in __init__
    self.read_multi(environ, keep_blank_values, strict_parsing)
  File "/usr/local/lib/python2.4/cgi.py", line 655, in read_multi
    environ, keep_blank_values, strict_parsing)
  File "/usr/local/lib/python2.4/cgi.py", line 532, in __init__
    self.read_single()
  File "/usr/local/lib/python2.4/cgi.py", line 665, in read_single
    self.read_lines()
  File "/usr/local/lib/python2.4/cgi.py", line 687, in read_lines
    self.read_lines_to_outerboundary()
  File "/usr/local/lib/python2.4/cgi.py", line 715, in read_lines_to_outerboundary
    line = self.fp.readline(1<<16)
  File "./trac/web/_fcgi.py", line 205, in readline
  File "./trac/web/_fcgi.py", line 159, in _waitForData
  File "./trac/web/_fcgi.py", line 696, in process_input
  File "./trac/web/_fcgi.py", line 808, in _do_unknown_type
NameError: global name 'rec' is not defined

The offending code in trac/web/_fcgi.py:

803    def _do_unknown_type(self, inrec):
804	        """Handle an unknown request type. Respond accordingly."""
805	        outrec = Record(FCGI_UNKNOWN_TYPE)
806	        outrec.contentData = struct.pack(FCGI_UnknownTypeBody, inrec.type)
807	        outrec.contentLength = FCGI_UnknownTypeBody_LEN
808	        self.writeRecord(rec)

Apparently this was cut and pasted from other code and the argument to self.writeRecord wasn't changed to 'inrec' to match the argument of the newly created function.

Don't ask me how nobody has ever exercised this code; the commit that introduced it is from 2005. I'm not sure why the file upload is getting tagged as unknown as other files upload correctly.

[Remy Blank]

Are you sure it's not outrec rather than inrec?

trac/web/_fcgi.py

@@ -805 +805 @@
         outrec = Record(FCGI_UNKNOWN_TYPE)
         outrec.contentData = struct.pack(FCGI_UnknownTypeBody, inrec.type)
         outrec.contentLength = FCGI_UnknownTypeBody_LEN
-        self.writeRecord(rec)
+        self.writeRecord(outrec)
 
 class MultiplexedConnection(Connection):
     """

I have no idea how fcgi works, but it looks very suspect that outrec would be created in that function and never used.

[anonymous]

That looks right, however it now results in this exception:

2009-03-09 10:48:45,765 Trac[main] DEBUG: Dispatching <Request "POST u'/attachment/ticket/1170/'">
2009-03-09 10:48:45,768 Trac[main] ERROR: Internal Server Error: 
Traceback (most recent call last):
  File "/usr/local/lib/python2.4/site-packages/Trac-0.11.3-py2.4.egg/trac/web/main.py", line 435, in _dispatch_request
    dispatcher.dispatch(req)
  File "/usr/local/lib/python2.4/site-packages/Trac-0.11.3-py2.4.egg/trac/web/main.py", line 166, in dispatch
    if handler.match_request(req):
  File "/usr/local/lib/python2.4/site-packages/Trac-0.11.3-py2.4.egg/trac/attachment.py", line 358, in match_request
    req.args['realm'] = realm
  File "/usr/local/lib/python2.4/site-packages/Trac-0.11.3-py2.4.egg/trac/web/api.py", line 194, in __getattr__
    value = self.callbacks[name](self)
  File "/usr/local/lib/python2.4/site-packages/Trac-0.11.3-py2.4.egg/trac/web/api.py", line 474, in _parse_args
    qs_on_post = self.environ.pop('QUERY_STRING')
KeyError: 'QUERY_STRING'

We've been able to isolate this to attempting to upload an attachment larger than 64KB, which triggers some special behavior in lighttpd.

[Remy Blank]

Patch from comment:1 applied in [8240].

About comment:2, I currently don't have a way to test FCGI, so this is just a guess, but does the following patch help?

trac/web/api.py

@@ -482 +482 @@
         # FieldStorage where QUERY_STRING is no longer ignored for POST
         # requests. We'll keep the pre 2.6 behaviour for now...
         if self.method == 'POST':
-            qs_on_post = self.environ.pop('QUERY_STRING')
+            qs_on_post = self.environ.pop('QUERY_STRING', '')
         fs = cgi.FieldStorage(fp, environ=self.environ, keep_blank_values=True)
         if self.method == 'POST':
             self.environ['QUERY_STRING'] = qs_on_post

[Christian Boos]

Given the lack of feedback, I was going to say "please apply previous fix and close", but I figured there could be eventually a problem with that fix, in the case the QUERY_STRING is not in the environment (like in the situation shown in the traceback) but where cgi.FieldStorage would manage to set the QUERY_STRING nevertheless (not sure if that can actually happen, as I've never run and debugged this code either ;-) ).

So I'd suggest a slightly different fix, which is perhaps safer:

trac/web/api.py

@@ -482 +482 @@
         # FieldStorage where QUERY_STRING is no longer ignored for POST
         # requests. We'll keep the pre 2.6 behaviour for now...
         if self.method == 'POST':
-            qs_on_post = self.environ.pop('QUERY_STRING')
+            qs_on_post = self.environ.pop('QUERY_STRING', None)
         fs = cgi.FieldStorage(fp, environ=self.environ, keep_blank_values=True)
         if self.method == 'POST':
-             self.environ['QUERY_STRING'] = qs_on_post
+             if qs_on_post is not None:
+                 self.environ['QUERY_STRING'] = qs_on_post

[Remy Blank]

I see what you mean. OTOH, isn't the whole purpose of this qs_on_post thing to prevent cgi.FieldStorage from overriding QUERY_STRING in the first place? That is, what happens if I pass a field named QUERY_STRING as part of POST data?

[Christian Boos]

Replying to rblank:

I see what you mean. OTOH, isn't the whole purpose of this qs_on_post thing to prevent cgi.FieldStorage from overriding QUERY_STRING in the first place?

No, it's there to prevent arguments to be doubled (#7876).

But thinking again about this, I guess the cgi.FieldStorage will never write anything into its environ argument (I just looked at cgi.py), so I think there's no effective difference between the two variants of the patch, we might as well go with the simpler one.

That is, what happens if I pass a field named QUERY_STRING as part of POST data?

Nothing special I guess, you'll get a request parameter named 'QUERY_STRING'.

[Remy Blank]

Replying to cboos:

No, it's there to prevent arguments to be doubled (#7876).

Oh, yes, right.

we might as well go with the simpler one.

Ok, will commit in a few minutes (right after the sync on multirepos).

[Remy Blank]

Applied patch from comment:3 in [8375].

[robertneville73@…]

patch [8375] does not resolve the issue…

Without understanding the ramifications of what I'm changing, my hack was to do the following:

        # Python 2.6 introduced a backwards incompatible change for
        # FieldStorage where QUERY_STRING is no longer ignored for POST
        # requests. We'll keep the pre 2.6 behaviour for now...
        if self.method == 'POST':
            try:
               qs_on_post = self.environ.pop('QUERY_STRING', '')
            except KeyError:
               None
        fs = cgi.FieldStorage(fp, environ=self.environ, keep_blank_values=True)
        if self.method == 'POST':
            self.environ['QUERY_STRING'] = qs_on_post

…And that seemed to solve it.

[Remy Blank]

I fail to see how self.environ.pop('QUERY_STRING', '') could ever raise a KeyError (see ​http://www.python.org/doc/2.3.5/lib/typesmapping.html). Except possibly if self.environ was not a dict. Could this be the case?

What web server and frontend are you using (tracd, mod_python, mod_wsgi, …)?

[anonymous]

Replying to rblank:

I fail to see how self.environ.pop('QUERY_STRING', '') could ever raise a KeyError (see ​http://www.python.org/doc/2.3.5/lib/typesmapping.html). Except possibly if self.environ was not a dict. Could this be the case?

What web server and frontend are you using (tracd, mod_python, mod_wsgi, …)?

I would normally agree, but that's what it's doing if I believe the error stack on my browser (and the fact the trouble went away (or at least appeared to…hard to know for sure since the problem wasn't consistent to begin with). This is working via mod_wsgi.

[Remy Blank]

Replying to anonymous:

I would normally agree, but that's what it's doing if I believe the error stack on my browser

Could you please post the stack trace you got?

[Remy Blank]

No reply, closing again. Please reopen if you can provide the requested traceback.

[lapo@…]

Replying to cboos:

So I'd suggest a slightly different fix, which is perhaps safer

I've got the same problem on every POST page with FreeBSD 6.2, Python 2.6.4, trac 0.11.5 configured in CGI; applying that patch to web.py solves the problem for me.

  File "/usr/local/lib/python2.6/site-packages/Genshi-0.5.1-py2.6-freebsd-6.4-STABLE-amd64.egg/genshi/template/eval.py", line 313, in lookup_attr
    val = getattr(obj, key)
  File "/usr/local/lib/python2.6/site-packages/Trac-0.11.5-py2.6.egg/trac/web/api.py", line 195, in __getattr__
    value = self.callbacks[name](self)
  File "/usr/local/lib/python2.6/site-packages/Trac-0.11.5-py2.6.egg/trac/web/api.py", line 485, in _parse_args
    qs_on_post = self.environ.pop('QUERY_STRING')
KeyError: 'QUERY_STRING'

[Remy Blank]

This issue has been fixed in 0.11.6, so you should upgrade.