Some ASan-related fixes

[Bob131]

A couple of commits somewhat related to #2890. I've submitted it as a draft since there was one test timeout that I want to investigate but I didn't want to wait until tomorrow to follow up on #2890.

I would appreciate some extra scrutiny around the 64bit_child test, since I'm not familiar with the details of how the kernel decides task arch-ness.

[Bob131]

The timeout was my bad, this looks ready to go (pending review, of course ;)

[bernhardu]

Edit: it is about the test/64bit_child: determine bitness at compile-time patch.

This iteration was added in #2784 to make tests be skipped
for a "-Dforce32bit=true" build running at a 64-bit kernel.
This change is intended to detect bitness of rr.

• 64-bit rr with 64-bit test at 64-bit linux --> librrpage.so --> test can be run
• 64-bit rr with 32-bit test at 64-bit linux --> librrpage_32.so --> test needs to be run
• 32-bit rr with 32-bit test at 64-bit linux --> librrpage.so --> test should be skipped
• 32-bit rr with 32-bit test at 32-bit linux --> librrpage.so --> test can be run (but bash would be 32-bit therefore no 64-bit child)

But I guess this would now set rr_is_32bit = 1 for the 64bit_child-32 test with a 64-bit rr too?

(I hope I remembered it correctly, I should have added a comment about this.)

[Bob131]

Oh, okay... yeah, I see the issue (Restating it for the sake of future me: the purpose of 64bit_child is checking that a 64-bit rr can handle a 32-bit process execing a 64-bit binary). So the proposed change would entirely negate the purpose of the test, right?

My first attempt at changing the test involved having rr expose a new environment variable (RR_ARCH); maybe this should be revisited? Or do you think it would be better maybe to just search for librrpreload_32.so or something like that?

(btw, thanks for the review!)

[bernhardu]

Oh, okay... yeah, I see the issue (Restating it for the sake of future me: the purpose of 64bit_child is checking that a 64-bit rr can handle a 32-bit process execing a 64-bit binary). So the proposed change would entirely negate the purpose of the test, right?

I guess that's it, exactly.

My first attempt at changing the test involved having rr expose a new environment variable (RR_ARCH); maybe this should be revisited? Or do you think it would be better maybe to just search for librrpreload_32.so or something like that?

If I understand right, looking for librrpage.so would no longer work, because it got renamed in the other patch to linux-vdso.so.1?
Then I guess looking for librrpreload.so would serve our purpose equally.

[Bob131]

How's that look @bernhardu?

[bernhardu]

How's that look @bernhardu?

To test it, I removed the "verify_asan_link_order=0" patch from my bernhardu:cmake-option-asan branch,
and added your soname and 64bit_child patch on top.
But I received still the "ASan runtime does not come first" error, e.g. in the "nested_detach" test.
If I understand it right, shouldn't the soname patch make that go away?

But I cannot judge if that approach in general is desired by the project ... @rocallahan ?

[Bob131]

To test it, I removed the "verify_asan_link_order=0" patch from my bernhardu:cmake-option-asan branch,
and added your soname and 64bit_child patch on top.
But I received still the "ASan runtime does not come first" error, e.g. in the "nested_detach" test.
If I understand it right, shouldn't the soname patch make that go away?

No, since those tests are non-ASan binaries that exec rr. At present, rr only has support for ASan binaries when they're started by rr directly; this patch restores this functionality (so it fixes, for example rr record rr replay ...).

As I said before, getting ASan working properly when the ASan binary has been exec'd by a child requires patching LD_PRELOAD at exec-time (which seems to be non-trivial; I haven't been working on it, but I have since realised that trying to patch it pre-exec runs into problems with script interpreters and/or binfmt).

[bernhardu]

Sorry for the delay, now I think I got it.
I tried to come up with a test for this, but either CMakeLists.txt gets more complicated,
or the test has to build a binary itself.
Your patches make the above test succeed.

I just wonder if something like "linux-vdso-rr.so.1" would be sufficient, to make it distinguishable?

[Bob131]

I tried to come up with a test for this, but either CMakeLists.txt gets more complicated,
or the test has to build a binary itself.

My thought was that running the test suite with an ASan-instrumented rr would effectively be the test for rr ASan support. However, that assumes ASan builds will be used a lot in future runs, which is not necessarily the case.

I think having the test runner script build the binary would break test suite installation and possibly end up putting build artefacts outside the build tree; that said, the alternatives (complicating the build for a single test or just not testing it) are not exactly ideal either... Sorry, I don't really have any useful feedback there.

I just wonder if something like "linux-vdso-rr.so.1" would be sufficient, to make it distinguishable?

Hmm, this is probably a matter of taste: I don't really see the need to make it distinguishable (other than some sort of just-in-case measure, lest it be handy for some future hack), meanwhile making it indistinguishable is a just-in-case measure for some future version of ASan (or some other piece of software) that checks more of the SONAME when searching for the vDSO. I've only a mild preference for the latter, so I'm not too fussed either way.

[bernhardu]

My thought was that running the test suite with an ASan-instrumented rr would effectively be the test for rr ASan support. However, that assumes ASan builds will be used a lot in future runs, which is not necessarily the case.

I think having the test runner script build the binary would break test suite installation and possibly end up putting build artefacts outside the build tree; that said, the alternatives (complicating the build for a single test or just not testing it) are not exactly ideal either... Sorry, I don't really have any useful feedback there.

Yes, I was just testing an option to have a test in the regular rr test run. I am not really happy with it but thought this might be less invasive as complicating CMakeLists.txt by adding a single asan enabled test ...

Hmm, this is probably a matter of taste: I don't really see the need to make it distinguishable (other than some sort of just-in-case measure, lest it be handy for some future hack), meanwhile making it indistinguishable is a just-in-case measure for some future version of ASan (or some other piece of software) that checks more of the SONAME when searching for the vDSO. I've only a mild preference for the latter, so I'm not too fussed either way.

I saw now e.g. gdb in Debian has a patch to detect it, so it might be better to just use plain "linux-vdso.so.1".

---

Back to the patch "rrpage: use Linux vDSO soname", just if someone wants to test.
It makes a difference for the tests ignore_nested*, record_replay* and clone_interruption-32 for me.
With it these tests succeed. (Tested with this branch.)

Splitting off the smaller patches into separate pull requests could probably simplify discussion and make merging more likely.

[bernhardu]

@rocallahan Have you some comments on this PR? What would be needed to progress here?

[rocallahan]

These changes look fine. I assume tests pass with them?

[bernhardu]

Sorry for the delay. I guess in that case, @Bob131 would you mind rebasing this PR on top of current tip?

[Bob131]

Ah, yes, I'm sorry about that; I hadn't realised the activity was on my PR rather than bernhardu's. I'm pretty sure I would have run the test suite before submitting the PR, but I might just check again rebased atop master.

[Bob131]

Okay, tests pass; should be good to go.

Thanks!

[rocallahan]

Thanks!