BUG: Ensure `__array_finalize__` cannot back-mangle shape #10314

mhvk · 2018-01-02T19:44:29Z

Just a small step, but __array_finalize__ should not be allowed to change the shape of any obj passed in. Arguably, it should not change the shape of the mask at all, but removing that currently breaks quite a bit more, so perhaps it is best to try to remedy that in a second step.

Need to think a bit about a test case that shows that this actually prevents problems.

eric-wieser · 2018-01-02T22:56:31Z

numpy/ma/core.py

@@ -3351,6 +3354,8 @@ def __setattr__(self, attr, value):
                self._mask.shape = self.shape
            except (AttributeError, TypeError):
                pass
+        elif attr == 'shape' and getmask(self) is not nomask:


Might be clearer to add dtype and shape @properties that fall back on ndarray.shape.__get__ etc

eric-wieser · 2018-01-03T12:48:45Z

numpy/ma/tests/test_old_ma.py

@@ -273,7 +273,8 @@ def test_testCopySize(self):
        assert_(y1.mask is m)

        y1a = array(y1, copy=0)
-        assert_(y1a.mask is y1.mask)
+        assert_(y1a._mask.__array_interface__ ==
+                y1._mask.__array_interface__)


Hmmm... I'd actually expect the stronger result of y1a is y1 here

This is #4043, I think

Maybe add a comment referencing that issue here?

eric-wieser · 2018-01-03T13:01:00Z

numpy/ma/tests/test_core.py

@@ -397,7 +397,8 @@ def test_copy(self):
        y1a = array(y1)
        assert_(y1a._data.__array_interface__ ==


Wait, what? This doesn't seem sensible to me at all (#10318)

mhvk · 2018-01-03T16:12:16Z

@eric-wieser - I agree the changes in behaviour for np.ma.array are not good, and I probably should work around them in the initializer.

Also agreed that setters for shape and dtype would be better than overriding setting for all attributes with __setattr__. I wasn't quite sure how to properly do this for properties - but a bit of googling suggests one has to do something like super().shape.__set__(<shape>).

mhvk · 2018-03-25T19:28:12Z

@eric-wieser - your comment reminded me about the existence of this branch... I now added a second commit replacing __setattr__ with shape and dtype properties and setters. I'm not sure I like it that much more... If you are also in doubt, perhaps I should just remove it so this one can focus on just the fix for the back-mangling of shapes.

I also still need to think of a test case that shows this actually prevents any problems!

eric-wieser · 2018-03-25T19:33:59Z

numpy/ma/core.py

+
+    @shape.setter
+    def shape(self, shape):
+        super(MaskedArray, MaskedArray).shape.__set__(self, shape)


I think this should be just super(MaskedArray).shape or perhaps just ndarray.shape

Edit: I'm wrong. TIL how super(cls, cls) works

Yes, this annoyance is in part why I wondered if it was a good idea to replace __setattr__ with this...

The advantage is that subclasses are now able to call Maskedarray.shape.__set__ in the unlikely even they override shape, and it won't accidentally call ndarray.shape.__set__

Actually, I think this should be super(MaskedArray, type(self)), which correctly walks the MRO.

eric-wieser · 2018-03-25T19:41:46Z

Is this enough for a test? On master:

>>> a = np.ma.array([1, 2, 3, 4], mask=True)
>>> d = a.data
>>> m = a.mask
>>> a.shape = (2, 2)
>>> d
array([1, 2, 3, 4])
>>> m  # your patch stops this changing shape, which is consistent with d
array([[ True,  True],
       [ True,  True]], dtype=bool)

mhvk · 2018-03-26T17:43:04Z

Hmm, that particular case is not solved by this PR - it needs your original one, where the mask is always viewed. Indeed, I found it very hard to far to get to a case where my PR matters... I'll try a bit more, but if it fails, perhaps best to split it into two - and at least get rid of the __setattr__ override.

eric-wieser · 2018-05-16T08:40:20Z

Ok, we now have a concrete problem caused by this, as documented in #3140

Since dtype and shape are properties, this needs a somewhat ugly super construction; see https://bugs.python.org/issue14965

mhvk · 2018-05-16T15:05:44Z

Thanks for realizing there was a test case after all! I now added it, and rebased for good measure (and fixed the super following your comment).

eric-wieser · 2018-05-29T07:49:13Z

numpy/ma/core.py

            try:
                self._mask.shape = self.shape
            except (AttributeError, TypeError):
                pass

+    @property
+    def shape(self):
+        return super(MaskedArray, self).shape


Pretty sure this needs to be super(MaskedArray, type(self)).shape.__get__(self, type(self)), unless there's something weird about base classes in C

Ok, seems there's something weird about base classes. If this were shadowing another property (rather than a getset_descriptor) , you'd need what I have above.

I don't think that's true - I;m fairly sure one can shadow a property getter as I have it above, just not the setter.

eric-wieser · 2018-05-29T07:54:40Z

Thanks @mhvk!

mhvk added 00 - Bug component: numpy.ma masked arrays labels Jan 2, 2018

mhvk requested a review from eric-wieser January 2, 2018 19:44

mhvk mentioned this pull request Jan 2, 2018

API: Make MaskedArray.mask return a view, rather than the underlying mask #10308

Merged

eric-wieser reviewed Jan 2, 2018

View reviewed changes

eric-wieser reviewed Jan 3, 2018

View reviewed changes

mhvk force-pushed the ma-array-finalize-mask-view branch 2 times, most recently from e39107a to 97628c2 Compare March 25, 2018 19:24

eric-wieser reviewed Mar 25, 2018

View reviewed changes

eric-wieser added the 59 - Needs tests label Mar 26, 2018

mhvk mentioned this pull request May 16, 2018

numpy.ma.tests.test_regression.TestRegression.test_var_sets_maskedarray_scalar is racy #10270

Open

Marten H. van Kerkwijk and others added 2 commits May 16, 2018 11:01

BUG: Ensure __array_finalize__ cannot back-mangle shape

6756edf

MAINT: Replace ma.__setattr__ with dtype and shape overrides.

b779bae

Since dtype and shape are properties, this needs a somewhat ugly super construction; see https://bugs.python.org/issue14965

mhvk force-pushed the ma-array-finalize-mask-view branch from 97628c2 to b779bae Compare May 16, 2018 15:01

mhvk removed the 59 - Needs tests label May 16, 2018

eric-wieser reviewed May 29, 2018

View reviewed changes

eric-wieser merged commit 6ccb03d into numpy:master May 29, 2018

mhvk deleted the ma-array-finalize-mask-view branch May 29, 2018 13:28

mhvk mentioned this pull request May 29, 2018

np.tile modifies the mask of the inputted masked array #3140

Closed

charris changed the title ~~BUG: Ensure __array_finalize__ cannot back-mangle shape~~ BUG: Ensure __array_finalize__ cannot back-mangle shape Jun 16, 2018

mhvk mentioned this pull request Sep 19, 2018

BUG: ndarray.base is not yet set in __array_finalize__, causing use-after-free #11237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Ensure `__array_finalize__` cannot back-mangle shape #10314

BUG: Ensure `__array_finalize__` cannot back-mangle shape #10314

mhvk commented Jan 2, 2018

eric-wieser Jan 2, 2018

eric-wieser Jan 3, 2018

eric-wieser Jan 3, 2018

eric-wieser Mar 25, 2018

eric-wieser Jan 3, 2018

mhvk commented Jan 3, 2018

mhvk commented Mar 25, 2018

eric-wieser Mar 25, 2018

eric-wieser Mar 25, 2018

mhvk Mar 26, 2018

eric-wieser Mar 26, 2018

eric-wieser May 16, 2018 •

edited

Loading

eric-wieser commented Mar 25, 2018 •

edited

Loading

mhvk commented Mar 26, 2018

eric-wieser commented May 16, 2018

mhvk commented May 16, 2018

eric-wieser May 29, 2018 •

edited

Loading

eric-wieser May 29, 2018

mhvk May 29, 2018

eric-wieser commented May 29, 2018

		@@ -397,7 +397,8 @@ def test_copy(self):
		y1a = array(y1)
		assert_(y1a._data.__array_interface__ ==

BUG: Ensure __array_finalize__ cannot back-mangle shape #10314

BUG: Ensure __array_finalize__ cannot back-mangle shape #10314

Conversation

mhvk commented Jan 2, 2018

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

mhvk commented Jan 3, 2018

mhvk commented Mar 25, 2018

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

eric-wieser May 16, 2018 • edited Loading

Choose a reason for hiding this comment

eric-wieser commented Mar 25, 2018 • edited Loading

mhvk commented Mar 26, 2018

eric-wieser commented May 16, 2018

mhvk commented May 16, 2018

eric-wieser May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented May 29, 2018

BUG: Ensure `__array_finalize__` cannot back-mangle shape #10314

BUG: Ensure `__array_finalize__` cannot back-mangle shape #10314

eric-wieser May 16, 2018 •

edited

Loading

eric-wieser commented Mar 25, 2018 •

edited

Loading

eric-wieser May 29, 2018 •

edited

Loading