classification
Title: addpackage in site.py fails hard on badly formed .pth files
Type: behavior Stage: resolved
Components: Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: r.david.murray Nosy List: Arfrever, dwr2, eric.araujo, georg.brandl, jwheare, r.david.murray, tarek, vstinner
Priority: normal Keywords: patch

Created on 2009-02-14 12:36 by jwheare, last changed 2010-12-26 22:30 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
site-traceback.diff georg.brandl, 2010-08-01 15:18 review
site_pth_exceptions.diff r.david.murray, 2010-12-25 03:54
site_pth_exceptions.diff r.david.murray, 2010-12-26 19:02
Messages (9)
msg82032 - (view) Author: James Wheare (jwheare) Date: 2009-02-14 12:36
As described here: http://james.wheare.org/notes/2009/02/import-site-
failed-use-v-for-traceback.php

The addpackage function will result in a TypeError being raised from os.path.exists(dir) -> from os.stat(path) if the contents of an 
inspected .pth file contain binary data.

This can happen in OS X network environments where ._ AppleDouble files 
are created to store resource forks and file metadata.

As this function is run whenever the interpreter is initialised, Python 
should be robust enough to ignore invalid data in these files, either by 
catching the TypeError in os.path.exists, or by detecting them at a 
higher level, but should be careful of false positives.

Another alternative is to raise a different exception and use it to 
display more helpful debugging info for those not familiar with pdb.
msg82034 - (view) Author: James Wheare (jwheare) Date: 2009-02-14 12:40
To clarify, the exception doesn't interrupt the interpreter, but the 
only indication of a problem is the following message:

'import site' failed; use -v for traceback

And you're then unable to import modules from site-packages.

Also, here's a clickable link to the blog post with more details:

http://tinyurl.com/dnlepc

(rather aggressive line length enforcement going on here...)
msg112344 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-08-01 15:17
Adding a patch that catches exceptions in a single addpackage() call and prints them.  I'd like to have more input though if this is a Good Thing(tm).
msg124628 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-25 03:54
Yes, I think it is a good idea for site.py to issue error messages and continue on when it is processing files that don't come from the python distribution itself (such as pth files).  However, I think just printing the error message is not going to provide enough info.  For example the error in this issue would produce something like:

   TypeError: embedded NUL character

Which wouldn't be much of a clue as to what went wrong.  Likewise the case of a pth file containing a like this:

    import foo)bar

This would result in an error message like this:

    SyntaxError: invalid syntax

This seems fine at first glance, but what if the pth file is:

    import foo

and foo.py is:

     foo)bar

We get the *same* error message, but the pth file itself is syntactically correct.

So, I think it is better to print the traceback, even if it results in extra info (lines from the addpackage routine itself).  Attached is a patch that takes this approach.  I locate this code in addpackage itself so that I can get the line number from the pth file for the error message, further localizing it.  And I wrap all of the code that I think could throw errors due to bad pth files, but only that code.

Note that both of these patches also address issue 10642, so I'm going to make this a superseder for that issue.

If you like this approach, Georg, then we just need unit tests :)
msg124634 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-12-25 11:43
Your patch is indeed better than mine, but I think the try-except in addsitedir() is not needed anymore?
msg124638 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-25 14:53
Yes, I forgot to delete that bit when I realized it could all be done in one place.
msg124672 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-26 19:02
Here is a revised patch with tests.
msg124674 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-12-26 19:14
LGTM.
msg124681 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-26 22:30
Committed to py3k in r87497, 3.1 in r87499, and 2.7 in r87500.
History
Date User Action Args
2010-12-26 22:30:03r.david.murraysetstatus: open -> closed
nosy: georg.brandl, dwr2, vstinner, tarek, eric.araujo, Arfrever, r.david.murray, jwheare
messages: + msg124681

resolution: fixed
stage: patch review -> resolved
2010-12-26 19:14:27georg.brandlsetnosy: georg.brandl, dwr2, vstinner, tarek, eric.araujo, Arfrever, r.david.murray, jwheare
messages: + msg124674
2010-12-26 19:02:07r.david.murraysetfiles: + site_pth_exceptions.diff
nosy: georg.brandl, dwr2, vstinner, tarek, eric.araujo, Arfrever, r.david.murray, jwheare
messages: + msg124672
2010-12-25 14:53:10r.david.murraysetnosy: georg.brandl, dwr2, vstinner, tarek, eric.araujo, Arfrever, r.david.murray, jwheare
messages: + msg124638
2010-12-25 11:43:08georg.brandlsetnosy: georg.brandl, dwr2, vstinner, tarek, eric.araujo, Arfrever, r.david.murray, jwheare
messages: + msg124634
2010-12-25 04:00:56r.david.murraysetnosy: + dwr2, vstinner, Arfrever, eric.araujo, tarek
2010-12-25 04:00:21r.david.murraylinkissue10642 superseder
2010-12-25 03:54:25r.david.murraysetfiles: + site_pth_exceptions.diff

messages: + msg124628
nosy: georg.brandl, r.david.murray, jwheare
stage: patch review
2010-08-01 15:18:00georg.brandlsetfiles: + site-traceback.diff

nosy: + georg.brandl, r.david.murray
messages: + msg112344

assignee: r.david.murray
keywords: + patch
2009-02-14 12:41:00jwhearesetmessages: + msg82034
2009-02-14 12:36:02jwhearecreate