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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Dec 23 2018, 15:23
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 23 2018, 15:23
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox requested changes to this revision.Fri, Dec 28, 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.Fri, Dec 28, 22:33
jasonbcox added inline comments.Tue, Jan 1, 00:06
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

deadalnix updated this revision to Diff 6495.Thu, Jan 3, 21:28
  • BITS => STATUS_BITS
  • Add a comment on the static assert.
Fabien requested changes to this revision.Fri, Jan 4, 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.Fri, Jan 4, 09:21
deadalnix requested review of this revision.Fri, Jan 4, 14:42

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

Fabien accepted this revision.Fri, Jan 4, 15:59

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

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