| msg77672 - (view) |
Author: Allan Crooks (amc1) |
Date: 2008-12-12 14:48 |
| If cgitb.html tries to get the value of an attribute from an object, and the getattr call causes an exception to be raised (other than an AttributeError), then cgitb.html fails to work: If you run the attached file in Python 2.5.2 or 2.6, you get the following output: ---- A: the letter a B: Something went wrong - attempting to generate HTML stack trace. Error generating HTML stack trace! Traceback (most recent call last): File "attrtest.py", line 21, in html_text = cgitb.html(sys.exc_info()) File "C:\python26\lib\cgitb.py", line 133, in html vars = scanvars(reader, frame, locals) File "C:\python26\lib\cgitb.py", line 84, in scanvars value = getattr(parent, token, __UNDEF__) File "attrtest.py", line 8, in __getattr__ return str(slf) # Intentional NameError NameError: global name 'slf' is not defined ---- The problem is in the scanvars function - it offers no protection against any unexpected exceptions that occur (that escape the getattr call) - which can be a problem if it is the same problematic code that caused cgitb.html to be called in the first place. I think scanvars should catch any exceptions that come from the getattr call and either mention that the attribute value could not be determined, or simply omit the mention of the attribute at all. If the line in the attached file is commented out, then the next line is caught appropriately and formatted correctly (the offending code occurs in the same block, but doesn't cause the same problem because it raises an AttributeError). |
|
|
| msg78005 - (view) |
Author: Allan Crooks (amc1) |
Date: 2008-12-18 01:43 |
| In terms of patching scanvars, I came up with the following solution: ORIGINAL: if parent is not __UNDEF__: value = getattr(parent, token, __UNDEF__) vars.append((prefix + token, prefix, value)) SOLUTION: if parent is not __UNDEF__: try: value = getattr(parent, token, __UNDEF__) except Exception: value = __UNDEF__ vars.append((prefix + token, prefix, value)) I think this makes the most sense - it requires a small change, and it won't have any strange side effect. One slightly undesirable aspect is that for an attribute value which could not be determined (due to this problem), it will say that it was "undefined", which isn't entirely accurate. My initial patch changed value to be a string to be "could not determine value", but if the line of code looked like this: print 'B:', weird.b.upper() Then for something which shouldn't work, it would determine that the value of weird.b.upper is the string method - which isn't what we're after. So I would recommend my original patch (as described above) as the best solution. |
|
|
| msg78457 - (view) |
Author: Allan Crooks (amc1) |
Date: 2008-12-29 15:43 |
| In the interests of getting this fixed (and not letting it die), should I submit a proper patch? I suppose I would have to do one for each version of Python that is affected (which is all of them, really). |
|
|
| msg78503 - (view) |
Author: Gabriel Genellina (ggenellina) |
Date: 2008-12-30 04:21 |
| I believe a patch against the trunk would be enough, but should include a test case. |
|
|
| msg177060 - (view) |
Author: Arthur Petitpierre (arthur.petitpierre) |
Date: 2012-12-06 21:22 |
| I attached a patch containing both the fix suggested by Allan and a test case. Tested against trunk and python2.7. |
|
|
| msg381025 - (view) |
Author: Irit Katriel (iritkatriel) *  |
Date: 2020-11-15 18:52 |
| The issue still occurs in 3.10. Python 3 version of the script: import cgitb class WeirdObject(object): def __getattr__(self, attr): if attr == 'a': return 'the letter a' elif attr == 'b': return str(slf) # Intentional NameError raise AttributeError(attr) try: weird = WeirdObject() print('A:', weird.a) print('B:', weird.b) print('C:', weird.c) except Exception as e: import sys print('\nSomething went wrong - attempting to generate HTML stack trace.') try: html_text = cgitb.html(sys.exc_info()) except: print('Error generating HTML stack trace!') raise else: print('Here is stack trace in HTML:\n', html_text) |
|
|
| msg381026 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2020-11-15 18:53 |
| Arthur, are you interested in converting your patch to a github pull request? |
|
|
| msg384170 - (view) |
Author: karl (karlcow) * |
Date: 2021-01-01 14:20 |
| Converted into GitHub PR https://github.com/python/cpython/pull/24038 |
|
|
| msg384183 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2021-01-01 15:17 |
| On closer scrutiny I'm not sure this patch should be merged. The getattr call here has a default value, so it should not raise AttributeError. It should also not raise any other exception because a valid implementation of __getattr__ should raise only AttributeError: https://docs.python.org/3/reference/datamodel.html#object.__getattr__ |
|
|
| msg384221 - (view) |
Author: karl (karlcow) * |
Date: 2021-01-02 13:21 |
| > The getattr call here has a default value, so it should not raise AttributeError. It should also not raise any other exception because a valid implementation of __getattr__ should raise only AttributeError: but probably the intent of the patch here is to surface a meaningful error with regards to the script, more than an error on the cgitb python lib itself as this is a traceback tool. A bit like an HTML validator which continue to process things to give some kind of meaning. Diving into previous issues about scanvars https://bugs.python.org/issue966992 Similar https://bugs.python.org/issue1047397 The last comment is basically this issue here. There is a patch which is lot better for this bug and which addresses the issues here. https://github.com/python/cpython/pull/15094 It has not been merged yet. Not sure why or if there is anything missing. Probably this bug could be closed as a duplicate of https://bugs.python.org/issue966992 And someone needs to push to push the latest bits for https://github.com/python/cpython/pull/15094 What do you think @iritkatriel ? I will close my PR. |
|
|
| msg384237 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2021-01-02 19:28 |
| I agree that this can be closed as duplicate. Thanks, Karl, for the research. |
|
|