test_eigen.py test_nonunit_stride_to_python bug fix (ASAN failure)

[rwgk]

Description

The bug is in the implementation of "block" in test_eigen.cpp, which returns a dangling reference.

Initial analysis provided by @hawkinsp:

---

We are passing a C-contiguous array, but pybind11 ends up copying it to a temporary F-contiguous array so it can pass it to Eigen. The eigen reference returned by "block" points into this temporary array, but the temporary array does not outlive the call.

e.g., this works around the problem by avoiding the temporary copy:

    rf = np.asarray(ref, order='F')
    assert np.all(m.block(rf, 2, 1, 3, 3) == ref[2:5, 1:4])

---

Note that this bug was discovered only after patching the numpy sources Google-internally, to turn off caching when testing with sanitizers. The valgrind tests in the pybind11 github CI use the original numpy sources, therefore valgrind is blind to this bug.

Suggested changelog entry:

[rwgk]

@hawkinsp, could you please review this PR, including the description?

[hawkinsp]

Something I would double check: is this test still actually testing what it is supposed to? The idea of the test is that a subblock of a larger matrix has non-trivial striding. If you copy that block, it may not: you may end up with a contiguous matrix once more.

[rwgk]

I want back to square one (undid the previous change).

Here is a completely different way to deal with the situation: 537574e

It's basically just @hawkinsp's original idea (PR description) slightly more fancy:

-    assert np.all(m.block(ref, 2, 1, 3, 3) == ref[2:5, 1:4])
-    assert np.all(m.block(ref, 1, 4, 4, 2) == ref[1:, 4:])
-    assert np.all(m.block(ref, 1, 4, 3, 2) == ref[1:4, 4:])
+    # Must be order="F", otherwise the type_caster will make a copy and
+    # m.block() will return a dangling reference (heap-use-after-free).
+    rof = np.asarray(ref, order="F")
+    assert np.all(m.block(rof, 2, 1, 3, 3) == rof[2:5, 1:4])
+    assert np.all(m.block(rof, 1, 4, 4, 2) == rof[1:, 4:])
+    assert np.all(m.block(rof, 1, 4, 3, 2) == rof[1:4, 4:])

I believe that's the best we can do to preserve the original intent of the test, but what if someone makes the same mistake again?

The answer is the rest of that commit. It is clearly on the crazy side, but was kind of fun to whip up and maybe entertaining / educational?

Keep or drop?

Other thoughts:

• I cannot think of an easy way to defuse the time bomb that 1. the caster sometimes makes copies and 2. it is possible to return references. I fear this will continue to blow up in people's faces, unfortunately. And even if the caster didn't make copies, without keep_alive (added) it is still unsafe (I think but didn't verify).

• I need the ASAN error to go away.

• But I don't have the time or background to fix the inherently unsafe behavior.

[rwgk]

For completeness: I ran the core fix (rof in test_eigen.py) by @hawkinsp and he found it acceptable.

[rwgk]

I deleted the needs changelog label, thinking mentioning this fix in a test will be more distracting than helpful there. Mentioning the root problem, that it is not obvious at all that a dangling reference is created, seems inappropriate for the changelog.