{"action":"created","issue":{"url":"https://api.github.com/repos/numpy/numpy/issues/5548","labels_url":"https://api.github.com/repos/numpy/numpy/issues/5548/labels{/name}","comments_url":"https://api.github.com/repos/numpy/numpy/issues/5548/comments","events_url":"https://api.github.com/repos/numpy/numpy/issues/5548/events","html_url":"https://github.com/numpy/numpy/pull/5548","id":56991631,"number":5548,"title":"ENH: structured datatype safety checks","user":{"login":"ahaldane","id":9040124,"avatar_url":"https://avatars.githubusercontent.com/u/9040124?v=3","gravatar_id":"","url":"https://api.github.com/users/ahaldane","html_url":"https://github.com/ahaldane","followers_url":"https://api.github.com/users/ahaldane/followers","following_url":"https://api.github.com/users/ahaldane/following{/other_user}","gists_url":"https://api.github.com/users/ahaldane/gists{/gist_id}","starred_url":"https://api.github.com/users/ahaldane/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ahaldane/subscriptions","organizations_url":"https://api.github.com/users/ahaldane/orgs","repos_url":"https://api.github.com/users/ahaldane/repos","events_url":"https://api.github.com/users/ahaldane/events{/privacy}","received_events_url":"https://api.github.com/users/ahaldane/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/numpy/numpy/labels/component:%20numpy.core","name":"component: numpy.core","color":"ff0000"},{"url":"https://api.github.com/repos/numpy/numpy/labels/Maintenance","name":"Maintenance","color":"02e10c"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/numpy/numpy/milestones/35","html_url":"https://github.com/numpy/numpy/milestones/1.10.0%20release","labels_url":"https://api.github.com/repos/numpy/numpy/milestones/35/labels","id":1055363,"number":35,"title":"1.10.0 release","description":"Marker for PRs that should go into Numpy 1.10.1.","creator":{"login":"charris","id":77272,"avatar_url":"https://avatars.githubusercontent.com/u/77272?v=3","gravatar_id":"","url":"https://api.github.com/users/charris","html_url":"https://github.com/charris","followers_url":"https://api.github.com/users/charris/followers","following_url":"https://api.github.com/users/charris/following{/other_user}","gists_url":"https://api.github.com/users/charris/gists{/gist_id}","starred_url":"https://api.github.com/users/charris/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/charris/subscriptions","organizations_url":"https://api.github.com/users/charris/orgs","repos_url":"https://api.github.com/users/charris/repos","events_url":"https://api.github.com/users/charris/events{/privacy}","received_events_url":"https://api.github.com/users/charris/received_events","type":"User","site_admin":false},"open_issues":16,"closed_issues":27,"state":"open","created_at":"2015-04-07T00:02:10Z","updated_at":"2015-05-15T13:04:43Z","due_on":null,"closed_at":null},"comments":13,"created_at":"2015-02-09T05:49:41Z","updated_at":"2015-06-03T21:35:57Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/numpy/numpy/pulls/5548","html_url":"https://github.com/numpy/numpy/pull/5548","diff_url":"https://github.com/numpy/numpy/pull/5548.diff","patch_url":"https://github.com/numpy/numpy/pull/5548.patch"},"body":"This PR tries to solve some issues related structured datatypes, especially view/getfield/setfield. Previously views of structured arrays containing objects were completely disabled.  This commit adds more lenient check for whether an object-array view is allowed, and adds similar checks to getfield/setfield\r\n\r\n(This PR has been edited around 3/4/15. I originally planned other changes which I will submit as separate PRs)\r\n    \r\nI believe this PR solves issues https://github.com/numpy/numpy/issues/2599, https://github.com/numpy/numpy/issues/3256, https://github.com/numpy/numpy/issues/3286, https://github.com/numpy/numpy/issues/2346, https://github.com/numpy/numpy/issues/3253. Issue https://github.com/numpy/numpy/issues/5081 is also now partially fixed, but requires further thought (please take a look at the example there). It also fixes the new scipy issue https://github.com/scipy/scipy/issues/4589.\r\n\r\nOnly one test needed modification to pass: `test_objectarray_setfield` checks that `setfield` with objects is disallowed, and expects a `RuntimeError` but I give it `TypeError`.\r\n\r\n### Summary\r\n\r\nPreviously views of structured arrays containing objects were completely disabled, which was a problem since views are needed in a few important cases such as when indexing multiple fields of a structured array, or in `__getitem__` in record arrays.\r\n\r\nWith help from @jaimefrio I now have functions that carefully check whether a view into array would interpret non-Object memory as an object, or vice versa, and allow the view if there is no problem. Big thanks to Jaime for help, code, and explanations on this. I think the checks are compatible with his PR https://github.com/numpy/numpy/pull/5508. I check that 1) No memory that is not an object will be interpreted as a object, 2) No memory containing an object will be interpreted as an arbitrary type. Both cases can cause segfaults.\r\n\r\nI also added similar checks to `PyArray_GetField` and `PyArray_SetField`. Previsouly, the user could view arbitrary data as a python Object, and it was fairly easy to make cases where a segfault would happen.\r\n\r\nNote, however, that there are still \"unsafe\" operations (wrt Objects) in numpy which I hope to examine in future PRs, when I have time.\r\n\r\n### How to keep track of objects\r\n\r\nThere is a design choice here worth discussing. As part of the check for objects when taking a view, we need to go though the new dtypes's fields and make sure none of them overlap with memory containing an object in the old dtype (unless the field is also an object). Consider this potential view:\r\n\r\n    >>> basearr = np.array([(1,2,3),(4,5,6)], \r\n    ...                    dtype=[('f0', 'i8'), ('f1', 'O'), ('f2', 'i8')])\r\n    >>> varr = basearr.view({'names': ['f0','f2'], \r\n    ...                      'formats': ['i8','i8'], \r\n    ...                      'offsets': [0,16]})\r\n    >>> v2arr = varr.view({'names': ['f0'], \r\n    ...                    'formats': ['i8'], \r\n    ...                    'offsets': [8]})  \r\n\r\nThe problem would be that writing to v2arr will overwrite the Object in basearr. We need to prevent this, but the challenge is that `varr` doesn't \"know\" that there is an object at position 8. This raises the question: How do we figure out where the objects are in memory to prevent them from being viewed?\r\n\r\nThe strategy I have taken in this PR is to only allow views into fields \"known\" to the parent array. In the example above, the `v2arr` view would be disallowed, and only views of `varr` into offsets 0 and 16 would be allowed. (specifically, views into bytes 0-8 and 16-24). The downside is that many views are no longer \"reversible\", since once you take a view that \"forgets\" what was at an offset, you can never view the memory at that offset again in further views. While this constraint only really has to apply to object arrays (as tested with `NPY_ITEM_HASOBJECT`, which I think is inherited in all views) I think for consistency it should apply to all structured arrays.. That is, all 'partial' views (views that have less fields than the original array) would be irreversible.\r\n\r\nAn alternate strategy is to too look up all the object positions in the \"base\" object of the array, `varr.base` in this case. I assume that the \"base\" array (if it is an array) contains the list of all object offsets, which I use to check for view overlaps. I implemented this in a separate branch (partial version [here](https://github.com/ahaldane/numpy/tree/view_objects_base)), but it was difficult because `arr.base` does not always lead to an array object. For example it may be a buffer, in which case I disallow views containing object types, but that's not a big problem because users aren't allowed to create object arrays from buffers anyway. Another problem is (void) scalar structured types, as they do not list their \"base\" object even after being converted to a 0-d array. To fix this I made void scalar types' base attribute point to the original array. However, there are still some cases where I cannot get to the original array - for example `nditer` buffers the source array, which causes about 5 unit tests to fail, which I have not solved yet (but I could try). The advantage would be that views are reversible.\r\n\r\nThe irreversible view strategy seems better because it touches less parts, has fewer edge cases, and seems to me less likely to go wrong. It also works right now. I also have not imagined any scenarios where inability to reverse a view would be a big problem, and no related unit tests fail.\r\n\r\nNote that even if the 'base array' strategy is not good, perhaps it's still a good idea to make void scalars \"base\" attribute point to the original array, as another PR? I admit I am not totally clear on the true purposes of \"base\" objects.\r\n\r\n\r\nIn this PR I also made some small tweaks to recarrays: I don't think they need to implement their own `view` function as a result of my last PRs.\r\n"},"comment":{"url":"https://api.github.com/repos/numpy/numpy/issues/comments/108622898","html_url":"https://github.com/numpy/numpy/pull/5548#issuecomment-108622898","issue_url":"https://api.github.com/repos/numpy/numpy/issues/5548","id":108622898,"user":{"login":"charris","id":77272,"avatar_url":"https://avatars.githubusercontent.com/u/77272?v=3","gravatar_id":"","url":"https://api.github.com/users/charris","html_url":"https://github.com/charris","followers_url":"https://api.github.com/users/charris/followers","following_url":"https://api.github.com/users/charris/following{/other_user}","gists_url":"https://api.github.com/users/charris/gists{/gist_id}","starred_url":"https://api.github.com/users/charris/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/charris/subscriptions","organizations_url":"https://api.github.com/users/charris/orgs","repos_url":"https://api.github.com/users/charris/repos","events_url":"https://api.github.com/users/charris/events{/privacy}","received_events_url":"https://api.github.com/users/charris/received_events","type":"User","site_admin":false},"created_at":"2015-06-03T21:35:56Z","updated_at":"2015-06-03T21:35:56Z","body":"@ahaldane Could you make the fixes, squash the commits, and rebase this? Might rebase #4652 also."}}