Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING22d0c8b3a2d9: fix ubsan for rpc method calling
rABC22d0c8b3a2d9: fix ubsan for rpc method calling

Summary

Fixes UndefinedBehaviorSanitizer (ubsan) check

WIP: Because if awful as hell.

Original error:

../../src/test/rpc_tests.cpp:32:27: runtime error: call to function getexcessiveblock(Config const&, JSONRPCRequest const&) through pointer to incorrect function type 'UniValue (*)(Config &, const JSONRPCRequest &)'

when running: ../configure --enable-ubsan && make && ./src/test/test_bitcoin

(this error general name is: "Indirect call of a function through a function pointer of the wrong type", and it is only reported on Darwin/Linux, C++ and x86/x86_64 only, according: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html).

Test Plan

../configure --enable-ubsan && make && ./src/test/test_bitcoin
no error should appear

Diff Detail

Repository

rABC Bitcoin ABC

Branch

ubsan

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 2510
Build 3144: Bitcoin ABC Buildbot (legacy)
Build 3143: arc lint + arc unit

Event Timeline

matiu created this revision.May 4 2018, 17:46

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 4 2018, 17:46

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

revert abc.cpp

As background, I was able to reproduce the reported problem with:

class C {
    public:
        int a;
};

void fn (const C& c){
    int a = c.a;
}

typedef  void (*fnP_type) (C& c);
int main(int argc, char **argv) {
    C c;
    fnP_type fnp;

    fnp=reinterpret_cast<fnP_type> (fn);
    (*fnp)(c);
}

compiled with

	clang++ -fsanitize=undefined kk.cc && ./a.out

throws:

kk.cc:19:5: runtime error: call to function fn(C const&) through pointer to incorrect function type 'void (*)(C &)'

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2499: Diff 3760.May 4 2018, 18:08

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

This solves the problem nicely, however the code structure is not ideal.

src/rpc/server.h
153 ↗	(On Diff #3761)	You should add a `call` method on `CRPCCommand` and do the magic in it and `useConstConfig` and `actor` can become private. Because we need to cast back and forth, it would actually make sense for actor to be an union.

This revision now requires changes to proceed.May 4 2018, 18:10

deadalnix added inline comments.May 4 2018, 18:13

src/rpc/server.h
166 ↗	(On Diff #3761)	This comments needs to be updated to reflect that it's in fact UB, so we need to jump through hoops.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2500: Diff 3761.May 4 2018, 18:30

refactor CRPCCommand

matiu retitled this revision from WIP:fix ubsan for rpc method calling to fix ubsan for rpc method calling.May 4 2018, 21:51

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

Harbormaster failed remote builds in B2509: Diff 3771!May 4 2018, 21:51

matiu added inline comments.May 4 2018, 21:53

src/rpc/server.cpp
207 ↗	(On Diff #3771)	Please review if this is ok.

update comments

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Harbormaster failed remote builds in B2510: Diff 3772!May 4 2018, 21:56

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

deadalnix added inline comments.May 5 2018, 12:05

src/rpc/server.cpp
175	Why not a set of `CRPCCommand &` ?

Better std:set for rpc help

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

updated std::set. Thanks for the review.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

Harbormaster failed remote builds in B2513: Diff 3775!May 5 2018, 12:30

A few small changes, but I think the solution looks pretty good now.

src/rpc/server.cpp
207 ↗	(On Diff #3775)	Braces
src/rpc/server.h
153 ↗	(On Diff #3775)	Please keep the struct layout as before. The order in which you declare fields in C++ is significant as it changes how the structure is represented in memory.
170 ↗	(On Diff #3775)	Please explains why? This is obviously not obvious because this was wrong in the past and required a fair amount of digging to get there.