Make _is_writable_dir more flexible to obscure failure modes #4165

embray · 2015-02-25T17:48:49Z

This is pretty obscure, but is something I've observed in testing out some installation issues.

When setuptools installs a package downloaded from a package index using easy_install--in particular if that package is listed in the setup_requires list of build-time requirements for another package, it uses a function called run_setup to run the setup.py script for the downloaded package. It runs the setup.py in the context of a DirectorySandbox which prevents the setup.py from writing files outside of the temporary directory set up for building the package.

If, in this case, the setup_requires package happens to import matplotlib and it goes down the right code path that his function is called, it will result in a SandboxViolation because matplotlib tried to write a file outside the sandbox. Unfortunately this is not an OSError, so the exception is raised rather than simply returning False is it should (indeed, the directory is not writable, so no attempt should be made to write a matplotlibrc file).

I think that if matplotlib is going to attempt to write files to the filesystem during import time this function should be prepared for any kind of obscure error condition that could result :)

WeatherGod · 2015-02-25T17:56:29Z

Actually, while we are at it... I have run into rare issues where the home
partition would be filled up by some other user (unquota'ed partitions,
grrrr), and then import of matplotlib would fail because it was trying to
test-write some file (a matplotlibrc, maybe?), even though it didn't need
to do that. I never tracked down the exact source of the problem, but it is
a weakness somewhere in the imports. Yes, the font-cache already existed at
the time.

On Wed, Feb 25, 2015 at 12:48 PM, Erik Bray notifications@github.com
wrote:

This is pretty obscure, but is something I've observed in testing out some
installation issues.

When setuptools installs a package downloaded from a package index using
easy_install--in particular if that package is listed in the
setup_requires list of build-time requirements for another package, it
uses a function called run_setup
https://bitbucket.org/pypa/setuptools/src/e50d57c3e4ed359316dc2ddce195ecfebe0784a9/setuptools/sandbox.py?at=default#cl-224
to run the setup.py script for the downloaded package. It runs the
setup.py in the context of a DirectorySandbox
https://bitbucket.org/pypa/setuptools/src/e50d57c3e4ed359316dc2ddce195ecfebe0784a9/setuptools/sandbox.py?at=default#cl-361
which prevents the setup.py from writing files outside of the temporary
directory set up for building the package.

If, in this case, the setup_requires package happens to import matplotlib
and it goes down the right code path that his function is called, it will
result in a SandboxViolation because matplotlib tried to write a file
outside the sandbox. Unfortunately this is not an OSError, so the
exception is raised rather than simply returning False is it should
(indeed, the directory is not writable, so no attempt should be made to
write a matplotlibrc file).

I think that if matplotlib is going to attempt to write files to the
filesystem during import time this function should be prepared for any kind

of obscure error condition that could result :)

You can view, comment on, or merge this pull request online at:

#4165
Commit Summary

Make _is_writable_dir more flexible to obscure failure modes

File Changes

M lib/matplotlib/init.py
https://github.com/matplotlib/matplotlib/pull/4165/files#diff-0 (2)

Patch Links:

https://github.com/matplotlib/matplotlib/pull/4165.patch

https://github.com/matplotlib/matplotlib/pull/4165.diff

—
Reply to this email directly or view it on GitHub
#4165.

embray · 2015-02-25T18:00:18Z

Probably the same issue. I'm a little surprised that case wouldn't result in an OSError but it's not inconceivable either.

mdboom · 2015-02-25T18:00:29Z

Seems fine to me. @tacaswell

efiring · 2015-02-25T18:15:25Z

In the case where it is not an OSError, maybe it would be worth printing out the exception and then proceeding.

embray · 2015-02-25T18:18:47Z

In fact it does seem like this issue goes a bit deeper:

matplotlib/lib/matplotlib/__init__.py

Line 606 in 1fe5b9c

if _is_writable_dir(p):

This gets called every time matplotlib is imported, even if the config dir already exists and the matplotlibrc already exists. It seems to me it shouldn't be doing this _is_wrtitable_dir test until and unless it has determined it actually needs to write into that directory.

tacaswell · 2015-02-25T18:37:23Z

This is fine with me as is, but 👍 on printing exception in strange cases.

I am ambivalent on minor changes in the logic in __init__.py. I'm sure it can be improved, but it is probably also worth looking at a total overhaul/refactor as it is kind of terrifying that we have 1.4k lines of code in our init.

efiring · 2015-02-25T18:42:32Z

@tacaswell, Agreed--init.py is due for a close look. Related to that, it would be good to have a routine startup time benchmark test. For some applications startup time really matters. Part of it is out of our control (mainly numpy), but much of it isn't. A few years ago we were paying attention to this--@mdboom, I'm sure you remember.

embray · 2015-02-25T19:02:54Z

FWIW I was just testing this myself and found the time for import matplotlib.pyplot, for example, to be pretty negligible. I'm sure it could be improved, but it seems way better than import astropy :) On the other hand touching a temp file on every import seems unnecessary and I don't like system-level side-effects of my Python module imports (although in some cases it makes sense to so I'm not saying don't do it at all).

…nged imports a little bit.

MNT : Ensure the gc module is available during interpreter exit

efiring · 2015-02-28T22:53:43Z

@embray, would you add a comment to the code, so that a later reader will know why a universal except: is being used, and/or some output for the non OSError case? I'm not sure the latter is actually worth the LOC; perhaps you can judge whether it is sufficiently likely to help someone in the future to understand what is going on. If it's not, then I would be happy to merge this with just a single added comment line.

WeatherGod · 2015-03-01T03:32:39Z

lib/matplotlib/__init__.py

@@ -225,7 +225,7 @@ def _is_writable_dir(p):
            t.write(b'1')
        finally:
            t.close()
-    except OSError:
+    except:


don't forget a "# noqa" flag here. I would also think it would be better to do catch Exception as a bare except catches everything.

What would it catch besides a subclass of Exception?

On the other hand there is still code out there that raises strange things (like strings), catching everything here seems like the correct thing to do 'Can we write to this dir yes or no?' I don't think we really care why we can't write there.

@embray, there is a subtle difference between a bare exception and catching Exception. A bare exception is basically equivalent to "except BaseException:", which isn't the same thing as "except Exception:". But, @tacaswell has a good point, we want to be able to catch any sort of failure, for whatever reason and just move on.

Ah, I didn't there was a separate BaseException under Exception, but all the more reason not to explicitly catch just Exception.

I don't see how could could "raise strings"--that would result in a TypeError.

I thought the restriction that you could only raise Exception objects was new-ish and we support back to 2.6 (but I couldn't figure out quickly when that got changed).

Looks like we are clear on that. Just tried on py2.6.

[centos6] [broot@rd22 gust_checks]$ /usr/bin/python Python 2.6.6 (r266:84292, Jan 22 2014, 09:42:36) [GCC 4.4.7 20120313 (Red Hat 4.4.7-4)] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> raise "SomeString" Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: exceptions must be old-style classes or derived from BaseException, not str >>>

On Mon, Mar 2, 2015 at 4:43 PM, Thomas A Caswell notifications@github.com
wrote:

In lib/matplotlib/init.py
#4165 (comment):

@@ -225,7 +225,7 @@ def _is_writable_dir(p):
t.write(b'1')
finally:
t.close()

except OSError:

except:

I thought the restriction that you could only raise Exception objects was
new-ish and we support back to 2.6 (but I couldn't figure out quickly when
that got changed).

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4165/files#r25641395.

embray · 2015-03-02T15:05:20Z

@efiring I might find it worth adding a message if I understood why this was being done at all. I guess it has something to do with writing a default matplotlibrc, but how frequently has anyone been worried about that?

Make _is_writable_dir more flexible to obscure failure modes

efiring · 2015-03-03T02:42:40Z

Travis is happy; let's move on.

Make _is_writable_dir more flexible to obscure failure modes

39470bd

tacaswell added the status: needs review label Feb 25, 2015

embray mentioned this pull request Feb 25, 2015

Ensure the gc module is available during interpreter exit #4166

Merged

embray added a commit to embray/astropy_helpers that referenced this pull request Feb 25, 2015

This adds a workaround for matplotlib/matplotlib#4165 and also rearra…

3d42bdc

…nged imports a little bit.

embray added a commit to embray/astropy_helpers that referenced this pull request Feb 26, 2015

This adds a workaround for matplotlib/matplotlib#4165 and also rearra…

642da87

…nged imports a little bit.

tacaswell added a commit that referenced this pull request Feb 27, 2015

Merge pull request #4165 from embray/patch-2

13a8f31

MNT : Ensure the gc module is available during interpreter exit

WeatherGod reviewed Mar 1, 2015
View reviewed changes

tacaswell added this to the next point release milestone Mar 2, 2015

efiring added a commit that referenced this pull request Mar 3, 2015

Merge pull request #4165 from embray/patch-1

7fd7a63

Make _is_writable_dir more flexible to obscure failure modes

efiring merged commit 7fd7a63 into matplotlib:master Mar 3, 2015

efiring removed the status: needs review label Mar 3, 2015

embray mentioned this pull request Mar 3, 2015

Test astropy-helpers v0.4.8 release against astropy astropy/astropy#3555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make _is_writable_dir more flexible to obscure failure modes #4165

Make _is_writable_dir more flexible to obscure failure modes #4165

embray commented Feb 25, 2015

WeatherGod commented Feb 25, 2015

of obscure error condition that could result :)

embray commented Feb 25, 2015

mdboom commented Feb 25, 2015

efiring commented Feb 25, 2015

embray commented Feb 25, 2015

tacaswell commented Feb 25, 2015

efiring commented Feb 25, 2015

embray commented Feb 25, 2015

efiring commented Feb 28, 2015

WeatherGod Mar 1, 2015

embray Mar 2, 2015

tacaswell Mar 2, 2015

WeatherGod Mar 2, 2015

embray Mar 2, 2015

tacaswell Mar 2, 2015

tacaswell Mar 2, 2015

WeatherGod Mar 2, 2015

embray commented Mar 2, 2015

efiring commented Mar 3, 2015

Make _is_writable_dir more flexible to obscure failure modes #4165

Make _is_writable_dir more flexible to obscure failure modes #4165

Conversation

embray commented Feb 25, 2015

WeatherGod commented Feb 25, 2015

of obscure error condition that could result :)

embray commented Feb 25, 2015

mdboom commented Feb 25, 2015

efiring commented Feb 25, 2015

embray commented Feb 25, 2015

tacaswell commented Feb 25, 2015

efiring commented Feb 25, 2015

embray commented Feb 25, 2015

efiring commented Feb 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

embray commented Mar 2, 2015

efiring commented Mar 3, 2015