n-y-z-o / nyzoVerifier

Verifier for the Nyzo cryptocurrency
The Unlicense
73 stars 42 forks source link

Potential bug - dereferencing null pointer #7

Closed glemercier closed 5 years ago

glemercier commented 5 years ago

In the following code:

https://github.com/n-y-z-o/nyzoVerifier/blob/master/src/main/java/co/nyzo/verifier/Verifier.java#L448

Block frozenEdge = BlockManager.frozenBlockForHeight(frozenEdgeHeight);
if (frozenEdge.getVerificationTimestamp() <=  System.currentTimeMillis() - Block.minimumVerificationInterval &&  frozenEdge.getBlockHeight() < BlockManager.openEdgeHeight(false)) {
    extendBlock(frozenEdge);
}

frozenEdge is not tested against null before dereferencing, although it seems from the implementation of frozenBlockForHeight that it may return null in some cases. Also the return of this function seems to be checked everywhere else in the code (example here), adding null checking would also add for better consistency.

n-y-z-o commented 5 years ago

The BlockManager.frozenBlockForHeight() method can, in one rare case, return a null reference after verifier initialization for the block height returned by BlockManager.getFrozenEdgeHeight() method. (The value of the frozenEdgeHeight variable was provided by that method.) This one case is due to the order in which frozenEdgeHeight = block.getBlockHeight(); and BlockManagerMap.addBlock(block); are performed in BlockManager.setFrozenEdge();. In the verifier loop, though, the process of creating and freezing blocks is totally serial, in order to prevent many weird situations that could occur due to a block being frozen while the verifier is trying to extend a different block. A null check is necessary from any thread other than the verifier thread, but it would be confusing from the verifier thread, because it would suggest that the frozen edge could be changing underneath the verifier.