Page MenuHomePhabricator

[avalanche] Add some safety to AvalancheBlockUpdate using static asserts
ClosedPublic

Authored by deadalnix on Dec 23 2018, 15:23.

Details

Summary

AvalancheBlockUpdate rely on the two lowers bits of a CBlockIndex* to be zero to work properly. While we know this is true, enforcing this is the case using a static assert is preferable.

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jasonbcox requested changes to this revision.Dec 28 2018, 22:33
jasonbcox added a subscriber: jasonbcox.

Nits on the variable naming to make it explicit what is being masked. Looks good otherwise.

src/avalanche.h
150 ↗(On Diff #6419)

BITS -> STATUS_BITS

151 ↗(On Diff #6419)

MASK -> STATUS_MASK

This revision now requires changes to proceed.Dec 28 2018, 22:33
src/avalanche.h
153 ↗(On Diff #6419)

See the build failure:

In file included from ../../src/avalanche.cpp:5:0:
../../src/avalanche.h:153:52: error: expected ‘,’ before ‘)’ token
     static_assert(alignof(CBlockIndex) >= 1 << BITS);

static_assert(statement, message) needs an error message

  • BITS => STATUS_BITS
  • Add a comment on the static assert.
Fabien requested changes to this revision.Jan 4 2019, 09:21
Fabien added a subscriber: Fabien.

I don't think you're checking anything useful here.
If the arch does not support unaligned access, the static_assert is redundant with the compiler behavior.
If the address is forced unaligned (e.g. using packing), the static_assert will not detect it anyway, and your status and block index will get corrupted.

You can only check at runtime that pindexIn is aligned to a multiple of (1 << STATUS_BITS).

This revision now requires changes to proceed.Jan 4 2019, 09:21

Unaligned pointers is undefined behavior. See: https://stackoverflow.com/a/28895321/672906

Got explanation from offline discussion : this check will prevent breaking the code if CBlockIndex get modified so thath its size gets <(1 << STATUS_BITS)

This revision is now accepted and ready to land.Jan 6 2019, 02:16
This revision was automatically updated to reflect the committed changes.