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
Branch
avacheckalign
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4406
Build 6877: Bitcoin ABC Buildbot (legacy)
Build 6876: arc lint + arc unit

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.