

PVS-Studio: Checking Bitcoin - ProgC
http://www.viva64.com/en/b/0268/

======
chrismorgan
I find it interesting comparing C/C++ with Rust in these cases, and I’m happy
to say that in this case all of the problems reported would have been caught
by the Rust compiler, either as errors or warnings, or are simply not
possible.

\- The first one is a standard unreachable code lint, which is by default a
warning in Rust. (I’m a little surprised that it wouldn’t lead to warnings in
other languages/compilers—is that really so?)

\- Integer literals like 0x80 are not of a specific type in Rust, being rather
int, i32, i64, uint, u32 or u64 as the context dictates—and in this context it
would be inferred as i64. All these sorts of casts are explicit, too, fixing
the entire class of issue—anything like this that PVS-Studio would catch is a
compilation error in Rust.

\- The whole copy constructors and = operator stuff is not relevant as Rust
doesn’t have it (which, according to C++ programmers I have heard reports
from, is good).

I’m looking forward to Rust being significant enough that tools like PVS-
Studio get built around it. Although Rust prevents various classes of errors,
human ingenuity is too great to keep all errors out! (Incidentally, you can
write custom lints already.)

~~~
simias
Actually, in rust I get no warning with the functionally similar (and I think
idiomatic) code:

    
    
        let l = [1i, 2, 3];
    
        for i in l.iter() {
            println!("{}", i);
            break;
        }
    

Obviously there's no explicit update as in the C code but since using
iterators is the Rust way to iterate over collections I don't think it's fair
to say that the rust compiler would have saved them on that one.

~~~
chrismorgan
It’s not a break—it’s a return. So the stuff after the for-loop is
unreachable.

~~~
jzwinck
The stuff after the for-loop is reachable if the for-loop is never entered.
You can consider it to be an if-block instead, with a return at the end.

~~~
chrismorgan
Ohhh, yes. I forgot that.

------
simias
That reads a bit sloppy to me.

"This is some function related to cryptography. I don't know what it does
exactly, and maybe I have found a real EPIC FAIL."

Maybe before you wrote your 1000 word article with custom made illustration
you could have spent a little timing digging in the code and understanding
what this function actually does? Or at least ask some bitcoin devs their
opinion? If you have actually found a vulnerability in bitcoin and disclose it
that way it's not even sloppy, it's irresponsible.

~~~
AndreyKarpov
1) This is a joke. Perhaps unfortunate translation. Below it is written that
the risk is small: You see, it's a popular trend nowadays to find epic errors
related to security. But this one is most likely a tiny bug or even a correct
piece of code written purposely.

2) I can not attentive examine each piece of code. Estimate the amount of work
that I do :) -
[http://www.viva64.com/en/a/0084/](http://www.viva64.com/en/a/0084/) ,
[http://www.viva64.com/en/examples/](http://www.viva64.com/en/examples/)

------
ikken
Since they're not sure if they found a security related error or not, I would
rather make a disclosure to Bitcoin dev team fist before going public with it.

------
CmonDev
I think it's pretty obvious that static analysis is essential when code
correctness is necessary. Of course the use of C++ is questionable, unless
high performance is needed.

------
ranty
Hang on, how is this _not_ wrong? It's supposed to iterate through
mapCryptedKeys but never gets past the first...

~~~
brulez
From my understanding, that code is just a verification that the password you
entered to decrypt your wallet was correct. It works by testing one of the
addresses in the wallet (verifying that the secret key matches the pubkey
address).

If one works, they should all work. If they don't then the wallet file is
probably corrupted. So it'd be a nice check for corruption but doesn't seem to
have much of a security implication.

