Valgrind/Memcheck false positive fixes for Rust

language design Unsafe Code Guidelines
1

I heard some time back that Rust folks had stopped using Valgrind/Memcheck to check unsafe Rust, because of false positives resulting from LLVM (correctly) generating branches on undefined values. I've been slowly working to fix this, by making Memcheck able to recover the underlying &&/|| expressions and instrumenting them more accurately.

So I'm looking for Rust folks who might be interested to try out this fixed version and provide feedback. There may be other sources of false positives, but my overall understanding is that we should be able to get close to zero false positives for run-of-the-mill unsafe Rust (and C++), possibly following extra work at this end. The relevant branch is obtained thusly [1] and a summary of what's changed is at [2].

[1] git clone git://sourceware.org/git/valgrind.git ;
cd valgrind ; git checkout grail

[2] https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=7204d087d266acf4f42dbf1db9a21c6fd0640dd3

23 Likes
2

This sounds great! Just to clarify, last time I checked valgrind didn't really work on recent macOS. Has that been fixed, or are you specifically looking for people who can test on Linux?

3

The OSX port status hasn't changed much recently. I'm primarily looking for people to test it on Linux right now.

4

I've given a brief look to the explanation and I disagree with the explanation. It says

No, it is correct transformation, period. If isRect returns true, but leaves isClosed undefined, the program triggers Undefined Behaviour, so that case is irrelevant, and all the other cases are handled correctly.

I seriously doubt it does any interprocedural analysis here. It only needs to prove that the speculative evaluation of A has no side-effect (it does not; it is a simple load) and that B's side-effects always happen (they do; the function call does happen before the tests).

That said, combining the blocks to a bitwise and should catch the UB case just fine, so the code is probably right (I don't understand valgrind internals to actually review that).

5

Edit: I thought I disagreed with this statement but I totally misread it. I concur with it, actually.

6

What does that post have to do with what @jan_hudec said? That post is only about the case where short-circuiting happens, i.e., where isRect returns false, not true.

To me it seems like the main complication here is considering what happens if isRect changes isClosed... the compiler has to make sure that the correct, updated value of isClosed is used then.

7

Derp, I misread it. You’re right.

8

For clarity (if only to un-confuse myself) –

The original code:

bool isClosed;
if (src.isRect(..., &isClosed, ...) && isClosed) {

Let's extract the function call to a variable, resulting in this fully equivalent version:

bool isClosed;
bool isRect = src.isRect(..., &isClosed, ...);
if (isRect && isClosed) {

Then, what the compiler is doing is effectively swapping the operands to the &&, making it instead

if (isClosed && isRect) {

The swapped version would be undefined behavior if written in C or Rust source code. But it's perfectly legal at the assembly level: since assembly doesn't have a concept of undefined bytes, we can say that isClosed has some unknown concrete value, and the original and swapped expressions are equivalent regardless of what that value is. On the other hand, Memcheck tries to retrofit a concept of undefined bytes onto assembly, leading to the false positive.

4 Likes