Merge pull request #407 from drdavella/strict-context-handler

Disallow access to tree when file handle is closed

.travis.yml

@@ -24,6 +24,7 @@ env:
         - PYTHON_VERSION=3.6
         - ASTROPY_VERSION=development
         - NUMPY_VERSION=stable
+        - PIP_DEPENDENCIES='pytest-astropy pytest-faulthandler'
         - CONDA_DEPENDENCIES='semantic_version jsonschema pyyaml six lz4'
         - GWCS_GIT='git+git://github.com/spacetelescope/gwcs.git#egg=gwcs'
         - SETUP_CMD='test --remote-data'
@@ -63,7 +64,6 @@ install:
     - git clone git://github.com/astropy/ci-helpers.git
     - source ci-helpers/travis/setup_conda.sh
     - python -m pip install --no-deps $GWCS_GIT
-    - python -m pip install pytest-astropy
     - python setup.py install
 
 script:

CHANGES.rst

@@ -8,6 +8,9 @@
 1.4.0 (unreleased)
 ------------------
 
+- Explicitly disallow access to entire tree for ASDF file objects that have
+  been closed. [#407]
+
 - Install and load extensions using ``setuptools`` entry points. [#384]
 
 - Automatically initialize ``asdf-standard`` submodule in ``setup.py``. [#398]

appveyor.yml

@@ -11,6 +11,7 @@ environment:
       ASTROPY_VERSION: "development"
       GWCS_GIT: "git+git://github.com/spacetelescope/gwcs.git#egg=gwcs"
       CONDA_DEPENDENCIES: "semantic_version jsonschema pyyaml six lz4"
+      PIP_DEPENDENCIES: "pytest-astropy pytest-faulthandler"
       PYTHON_ARCH: "64"
 
   matrix:
@@ -39,7 +40,6 @@ install:
     - "SET PATH=%PYTHON%;%PYTHON%\\Scripts;%PATH%"
     - "activate test"
     - "%CMD_IN_ENV% python -m pip install --no-deps %GWCS_GIT%"
-    - "%CMD_IN_ENV% python -m pip install pytest-astropy"
     - "%CMD_IN_ENV% python setup.py develop --no-deps"
 
 # Not a .NET project

asdf/asdf.py

@@ -91,6 +91,7 @@ def __init__(self, tree=None, uri=None, extensions=None, version=None,
         self._file_format_version = None
 
         self._fd = None
+        self._closed = False
         self._external_asdf_by_uri = {}
         self._blocks = block.BlockManager(self, copy_arrays=copy_arrays)
         self._uri = None
@@ -121,15 +122,7 @@ def __enter__(self):
         return self
 
     def __exit__(self, type, value, traceback):
-        if self._fd:
-            # This is ok to always do because GenericFile knows
-            # whether it "owns" the file and should close it.
-            self._fd.__exit__(type, value, traceback)
-            self._fd = None
-        for external in self._external_asdf_by_uri.values():
-            external.__exit__(type, value, traceback)
-        self._external_asdf_by_uri.clear()
-        self._blocks.close()
+        self.close()
 
     def _process_extensions(self, extensions):
         if extensions is None or extensions == []:
@@ -154,11 +147,12 @@ def close(self):
         """
         Close the file handles associated with the `AsdfFile`.
         """
-        if self._fd:
+        if self._fd and not self._closed:
             # This is ok to always do because GenericFile knows
             # whether it "owns" the file and should close it.
             self._fd.close()
             self._fd = None
+            self._closed = True
         for external in self._external_asdf_by_uri.values():
             external.close()
         self._external_asdf_by_uri.clear()
@@ -274,6 +268,8 @@ def tree(self):
 
         When set, the tree will be validated against the ASDF schema.
         """
+        if self._closed:
+            raise OSError("Cannot access data from closed ASDF file")
         return self._tree
 
     @tree.setter

asdf/tests/conftest.py

@@ -0,0 +1,31 @@
+# Licensed under a 3-clause BSD style license - see LICENSE.rst
+# -*- coding: utf-8 -*-
+
+import pytest
+import numpy as np
+
+
+@pytest.fixture
+def small_tree():
+    x = np.arange(0, 10, dtype=np.float)
+    tree = {
+        'science_data': x,
+        'subset': x[3:-3],
+        'skipping': x[::2],
+        'not_shared': np.arange(10, 0, -1, dtype=np.uint8)
+    }
+    return tree
+
+
+@pytest.fixture
+def large_tree():
+    # These are designed to be big enough so they don't fit in a
+    # single block, but not so big that RAM/disk space for the tests
+    # is enormous.
+    x = np.random.rand(256, 256)
+    y = np.random.rand(16, 16, 16)
+    tree = {
+        'science_data': x,
+        'more': y
+    }
+    return tree

asdf/tests/test_asdftypes.py

@@ -84,9 +84,9 @@ def check(tag):
             buff, extensions=FractionCallable()) as ff:
         assert ff.tree['a'] == fractions.Fraction(2, 3)
 
-    buff = io.BytesIO()
-    ff.write_to(buff)
-    buff.close()
+        buff = io.BytesIO()
+        ff.write_to(buff)
+        buff.close()
 
 
 def test_version_mismatch():

asdf/tests/test_generic_io.py

@@ -19,33 +19,11 @@
 from .. import util
 
 from . import helpers
+# The only reason for importing these is to use them in the fixture below
+from .conftest import small_tree, large_tree
 
 
-def _get_small_tree():
-    x = np.arange(0, 10, dtype=np.float)
-    tree = {
-        'science_data': x,
-        'subset': x[3:-3],
-        'skipping': x[::2],
-        'not_shared': np.arange(10, 0, -1, dtype=np.uint8)
-        }
-    return tree
-
-
-def _get_large_tree():
-    # These are designed to be big enough so they don't fit in a
-    # single block, but not so big that RAM/disk space for the tests
-    # is enormous.
-    x = np.random.rand(256, 256)
-    y = np.random.rand(16, 16, 16)
-    tree = {
-        'science_data': x,
-        'more': y
-        }
-    return tree
-
-
-@pytest.fixture(params=[_get_small_tree, _get_large_tree])
+@pytest.fixture(params=[small_tree, large_tree])
 def tree(request):
     return request.param()
 
@@ -75,13 +53,13 @@ def test_mode_fail(tmpdir):
         generic_io.get_file(path, mode="r+")
 
 
-def test_open(tmpdir):
+def test_open(tmpdir, small_tree):
     from .. import open
 
     path = os.path.join(str(tmpdir), 'test.asdf')
 
     # Simply tests the high-level "open" function
-    ff = asdf.AsdfFile(_get_small_tree())
+    ff = asdf.AsdfFile(small_tree)
     ff.write_to(path)
     with open(path) as ff2:
         helpers.assert_tree_match(ff2.tree, ff.tree)
@@ -373,46 +351,39 @@ def get_read_fd():
         assert len(list(ff.blocks.external_blocks)) == 2
 
 
-def test_exploded_stream_write():
+def test_exploded_stream_write(small_tree):
     # Writing an exploded file to an output stream should fail, since
     # we can't write "files" alongside it.
 
-    tree = _get_small_tree()
-
-    ff = asdf.AsdfFile(tree)
+    ff = asdf.AsdfFile(small_tree)
 
     with pytest.raises(ValueError):
         ff.write_to(io.BytesIO(), all_array_storage='external')
 
 
-def test_exploded_stream_read(tmpdir):
+def test_exploded_stream_read(tmpdir, small_tree):
     # Reading from an exploded input file should fail, but only once
     # the data block is accessed.  This behavior is important so that
     # the tree can still be accessed even if the data is missing.
-    tree = _get_small_tree()
 
     path = os.path.join(str(tmpdir), 'test.asdf')
 
-    ff = asdf.AsdfFile(tree)
+    ff = asdf.AsdfFile(small_tree)
     ff.write_to(path, all_array_storage='external')
 
     with open(path, 'rb') as fd:
         # This should work, so we can get the tree content
         x = generic_io.InputStream(fd, 'r')
         with asdf.AsdfFile.open(x) as ff:
-            pass
-
-    # It's only on trying to get at the block data that the error
-    # occurs.
-    with pytest.raises(ValueError):
-        ff.tree['science_data'][:]
+            # It's only when trying to access external data that an error occurs
+            with pytest.raises(ValueError):
+                ff.tree['science_data'][:]
 
 
-def test_unicode_open(tmpdir):
+def test_unicode_open(tmpdir, small_tree):
     path = os.path.join(str(tmpdir), 'test.asdf')
 
-    tree = _get_small_tree()
-    ff = asdf.AsdfFile(tree)
+    ff = asdf.AsdfFile(small_tree)
 
     ff.write_to(path)
 

asdf/tests/test_low_level.py

@@ -25,17 +25,6 @@
 from ..exceptions import AsdfDeprecationWarning
 
 
-def _get_small_tree():
-    x = np.arange(0, 10, dtype=np.float)
-    tree = {
-        'science_data': x,
-        'subset': x[3:-3],
-        'skipping': x[::2],
-        'not_shared': np.arange(10, 0, -1, dtype=np.uint8)
-        }
-    return tree
-
-
 def test_no_yaml_end_marker(tmpdir):
     content = b"""#ASDF 1.0.0
 %YAML 1.1
@@ -139,10 +128,10 @@ def test_no_asdf_blocks(tmpdir):
             assert len(ff.blocks) == 0
 
 
-def test_invalid_source():
+def test_invalid_source(small_tree):
     buff = io.BytesIO()
 
-    ff = asdf.AsdfFile(_get_small_tree())
+    ff = asdf.AsdfFile(small_tree)
     ff.write_to(buff)
 
     buff.seek(0)
@@ -232,13 +221,12 @@ def test_block_header_too_small():
 
 
 if six.PY2:
-    def test_file_already_closed(tmpdir):
+    def test_file_already_closed(tmpdir, small_tree):
         # Test that referencing specific blocks in another asdf file
         # works.
-        tree = _get_small_tree()
 
         path = os.path.join(str(tmpdir), 'test.asdf')
-        ff = asdf.AsdfFile(tree)
+        ff = asdf.AsdfFile(small_tree)
         ff.write_to(path)
 
         with open(path, 'rb') as fd:
@@ -767,12 +755,10 @@ def test_checksum_update(tmpdir):
             b'T\xaf~[\x90\x8a\x88^\xc2B\x96D,N\xadL'
 
 
-def test_atomic_write(tmpdir):
+def test_atomic_write(tmpdir, small_tree):
     tmpfile = os.path.join(str(tmpdir), 'test.asdf')
 
-    tree = _get_small_tree()
-
-    ff = asdf.AsdfFile(tree)
+    ff = asdf.AsdfFile(small_tree)
     ff.write_to(tmpfile)
 
     with asdf.AsdfFile.open(tmpfile) as ff:
@@ -831,10 +817,10 @@ def test_copy(tmpdir):
     assert_array_equal(ff2.tree['my_array'], ff2.tree['my_array'])
 
 
-def test_deferred_block_loading():
+def test_deferred_block_loading(small_tree):
     buff = io.BytesIO()
 
-    ff = asdf.AsdfFile(_get_small_tree())
+    ff = asdf.AsdfFile(small_tree)
     ff.write_to(buff, include_block_index=False)
 
     buff.seek(0)
@@ -1177,13 +1163,13 @@ def test_fd_not_seekable():
     assert b.data.tobytes() == data.tobytes()
 
 
-def test_top_level_tree():
-    tree = {'tree': _get_small_tree()}
+def test_top_level_tree(small_tree):
+    tree = {'tree': small_tree}
     ff = asdf.AsdfFile(tree)
     assert_tree_match(ff.tree['tree'], ff['tree'])
 
     ff2 = asdf.AsdfFile()
-    ff2['tree'] = _get_small_tree()
+    ff2['tree'] = small_tree
     assert_tree_match(ff2.tree['tree'], ff2['tree'])
 
 
@@ -1195,3 +1181,36 @@ def test_tag_to_schema_resolver_deprecation():
     with pytest.warns(AsdfDeprecationWarning):
         extension_list = extension.default_extensions.extension_list
         extension_list.tag_to_schema_resolver('foo')
+
+
+def test_access_tree_outside_handler(tmpdir):
+    tempname = str(tmpdir.join('test.asdf'))
+
+    tree = {'random': np.random.random(10)}
+
+    ff = asdf.AsdfFile(tree)
+    ff.write_to(str(tempname))
+
+    with asdf.AsdfFile.open(tempname) as newf:
+        pass
+
+    # Accessing array data outside of handler should fail
+    with pytest.raises(OSError):
+        newf.tree['random'][0]
+
+
+def test_context_handler_resolve_and_inline(tmpdir):
+    # This reproduces the issue reported in
+    # https://github.com/spacetelescope/asdf/issues/406
+    tempname = str(tmpdir.join('test.asdf'))
+
+    tree = {'random': np.random.random(10)}
+
+    ff = asdf.AsdfFile(tree)
+    ff.write_to(str(tempname))
+
+    with asdf.AsdfFile.open(tempname) as newf:
+        newf.resolve_and_inline()
+
+    with pytest.raises(OSError):
+        newf.tree['random'][0]