Edgewall Software
Modify

Opened 12 years ago

Closed 11 years ago

#8114 closed defect (fixed)

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

Reported by: dwhite@… Owned by: Remy Blank
Priority: normal Milestone: 0.11.6
Component: web frontend Version: 0.11.3
Severity: normal Keywords: needinfo
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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.

Attachments (0)

Change History (15)

comment:1 by Remy Blank, 12 years ago

Milestone: 0.11.4
Owner: set to Remy Blank

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

  • trac/web/_fcgi.py

    diff --git a/trac/web/_fcgi.py b/trac/web/_fcgi.py
    a b  
    805805        outrec = Record(FCGI_UNKNOWN_TYPE)
    806806        outrec.contentData = struct.pack(FCGI_UnknownTypeBody, inrec.type)
    807807        outrec.contentLength = FCGI_UnknownTypeBody_LEN
    808         self.writeRecord(rec)
     808        self.writeRecord(outrec)
    809809
    810810class MultiplexedConnection(Connection):
    811811    """

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

comment:2 by anonymous, 12 years ago

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.

comment:3 by Remy Blank, 11 years ago

Keywords: needinfo added
Milestone: 0.11.60.11.5

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

    diff --git a/trac/web/api.py b/trac/web/api.py
    a b  
    482482        # FieldStorage where QUERY_STRING is no longer ignored for POST
    483483        # requests. We'll keep the pre 2.6 behaviour for now...
    484484        if self.method == 'POST':
    485             qs_on_post = self.environ.pop('QUERY_STRING')
     485            qs_on_post = self.environ.pop('QUERY_STRING', '')
    486486        fs = cgi.FieldStorage(fp, environ=self.environ, keep_blank_values=True)
    487487        if self.method == 'POST':
    488488            self.environ['QUERY_STRING'] = qs_on_post

comment:4 by Christian Boos, 11 years ago

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

    a b  
    482482        # FieldStorage where QUERY_STRING is no longer ignored for POST
    483483        # requests. We'll keep the pre 2.6 behaviour for now...
    484484        if self.method == 'POST':
    485             qs_on_post = self.environ.pop('QUERY_STRING')
     485            qs_on_post = self.environ.pop('QUERY_STRING', None)
    486486        fs = cgi.FieldStorage(fp, environ=self.environ, keep_blank_values=True)
    487487        if self.method == 'POST':
    488              self.environ['QUERY_STRING'] = qs_on_post
     488             if qs_on_post is not None:
     489                 self.environ['QUERY_STRING'] = qs_on_post

comment:5 by Remy Blank, 11 years ago

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?

in reply to:  5 ; comment:6 by Christian Boos, 11 years ago

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'.

in reply to:  6 comment:7 by Remy Blank, 11 years ago

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).

comment:8 by Remy Blank, 11 years ago

Resolution: fixed
Status: newclosed

Applied patch from comment:3 in [8375].

comment:9 by robertneville73@…, 11 years ago

Resolution: fixed
Status: closedreopened

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.

comment:10 by Remy Blank, 11 years ago

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, …)?

in reply to:  10 ; comment:11 by anonymous, 11 years ago

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.

in reply to:  11 comment:12 by Remy Blank, 11 years ago

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?

comment:13 by Remy Blank, 11 years ago

Resolution: fixed
Status: reopenedclosed

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

in reply to:  4 comment:14 by lapo@…, 11 years ago

Resolution: fixed
Status: closedreopened

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'

comment:15 by Remy Blank, 11 years ago

Resolution: fixed
Status: reopenedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.