Page MenuHomePhabricator

fix ubsan for rpc method calling
ClosedPublic

Authored by matiu on May 4 2018, 17:46.

Details

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

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 &)'
deadalnix requested changes to this revision.May 4 2018, 18:10

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
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.

matiu retitled this revision from WIP:fix ubsan for rpc method calling to fix ubsan for rpc method calling.May 4 2018, 21:51
src/rpc/server.cpp
207 ↗(On Diff #3771)

Please review if this is ok.

src/rpc/server.cpp
175

Why not a set of CRPCCommand & ?

Better std:set for rpc help

updated std::set. Thanks for the review.

deadalnix requested changes to this revision.May 5 2018, 17:10

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.

This revision now requires changes to proceed.May 5 2018, 17:10

comments and formatting nits

This revision is now accepted and ready to land.May 5 2018, 18:10
This revision was automatically updated to reflect the committed changes.