[C++] grpc + flatbuffers causes some errors.

[MizukiSonoko]

Hi

I write grpc + flatbuffers application. In https://github.com/hyperledger/iroha (Environments information is in https://github.com/MizukiSonoko/test_flatbuffers , Sorry)

1) This commit(https://github.com/google/flatbuffers/commit/6cc2307c71eec0ad690ad69f03a1d89655e526c8) happens this compile error

root@6f25b0087098:/tmp/sandbox# make
flatc --cpp --grpc --gen-object-api sample.fbs
g++ -std=c++14 sample.grpc.fb.cc grpctest.cpp  -lgrpc++ -lgrpc++_reflection -lgpr -pthread -o grpctest
In file included from sample.grpc.fb.cc:12:0:
/usr/local/include/grpc++/impl/codegen/method_handler_impl.h: In instantiation of 'void grpc::RpcMethodHandler<ServiceType, RequestType, ResponseType>::RunHandler(const grpc::MethodHandler::HandlerParameter&) [with ServiceType = sample::SampleService::Service; RequestType = flatbuffers::BufferRef<sample::SampleRoot>; ResponseType = flatbuffers::BufferRef<sample::Response>]':
sample.grpc.fb.cc:60:1:   required from here
/usr/local/include/grpc++/impl/codegen/method_handler_impl.h:55:66: error: no matching function for call to 'grpc::SerializationTraits<flatbuffers::BufferRef<sample::SampleRoot>, void>::Deserialize(grpc_byte_buffer* const&, flatbuffers::BufferRef<sample::SampleRoot>*, const int&)'
     Status status = SerializationTraits<RequestType>::Deserialize(
                                                                  ^
In file included from sample.grpc.fb.h:8:0,
                 from sample.grpc.fb.cc:6:
/usr/local/include/flatbuffers/grpc.h:46:23: note: candidate: static grpc::Status grpc::SerializationTraits<T, typename std::enable_if<std::is_base_of<flatbuffers::BufferRefBase, T>::value>::type>::Deserialize(grpc_byte_buffer*, T*) [with T = flatbuffers::BufferRef<sample::SampleRoot>; grpc_byte_buffer = grpc_byte_buffer]
   static grpc::Status Deserialize(grpc_byte_buffer *buffer, T *msg) {
                       ^
/usr/local/include/flatbuffers/grpc.h:46:23: note:   candidate expects 2 arguments, 3 provided
In file included from /usr/local/include/grpc++/impl/codegen/async_stream.h:37:0,
                 from sample.grpc.fb.h:10,
                 from sample.grpc.fb.cc:6:
/usr/local/include/grpc++/impl/codegen/call.h: In instantiation of 'void grpc::CallOpRecvMessage<R>::FinishOp(bool*, int) [with R = flatbuffers::BufferRef<sample::Response>]':
/usr/local/include/grpc++/impl/codegen/call.h:613:5:   required from 'bool grpc::CallOpSet<Op1, Op2, Op3, Op4, Op5, Op6>::FinalizeResult(void**, bool*) [with Op1 = grpc::CallOpRecvInitialMetadata; Op2 = grpc::CallOpRecvMessage<flatbuffers::BufferRef<sample::Response> >; Op3 = grpc::CallOpClientRecvStatus; Op4 = grpc::CallNoOp<4>; Op5 = grpc::CallNoOp<5>; Op6 = grpc::CallNoOp<6>]'
sample.grpc.fb.cc:60:1:   required from here
/usr/local/include/grpc++/impl/codegen/call.h:308:68: error: no matching function for call to 'grpc::SerializationTraits<flatbuffers::BufferRef<sample::Response>, void>::Deserialize(grpc_byte_buffer*&, flatbuffers::BufferRef<sample::Response>*&, int&)'
         got_message = *status = SerializationTraits<R>::Deserialize(
                                                                    ^
In file included from sample.grpc.fb.h:8:0,
                 from sample.grpc.fb.cc:6:
/usr/local/include/flatbuffers/grpc.h:46:23: note: candidate: static grpc::Status grpc::SerializationTraits<T, typename std::enable_if<std::is_base_of<flatbuffers::BufferRefBase, T>::value>::type>::Deserialize(grpc_byte_buffer*, T*) [with T = flatbuffers::BufferRef<sample::Response>; grpc_byte_buffer = grpc_byte_buffer]
   static grpc::Status Deserialize(grpc_byte_buffer *buffer, T *msg) {
                       ^
/usr/local/include/flatbuffers/grpc.h:46:23: note:   candidate expects 2 arguments, 3 provided
Makefile:8: recipe for target 'grpctest' failed
make: *** [grpctest] Error 1

2) I think in the case union in union like

table Object1{
  text: string (required);
  boolean: bool;  
}
table Object2{
  text: string (required);
  integer: int;
}
table Object3{
  text:   string;
}
union Object  { Object1, Object2, Object3 }

table Wrapper1{
  object: Object (required);
}
table Wrapper2{
  text:     string (required);
  object:   Object (required);
}
table Wrapper3{
  object: Object (required);
}
union Wrapper { Wrapper1, Wrapper2, Wrapper3 }

Is happen errors. https://github.com/MizukiSonoko/test_flatbuffers#bug

I think maybe this code correct. so May I rewrite Reset?

inline void ObjectUnion::Reset() {
  /*
  switch (type) {
    case Object_Object1: {
      auto ptr = reinterpret_cast<Object1T *>(table);
      delete ptr;
      break;
    }
    case Object_Object2: {
      auto ptr = reinterpret_cast<Object2T *>(table);
      delete ptr;
      break;
    }
    case Object_Object3: {
      auto ptr = reinterpret_cast<Object3T *>(table);
      delete ptr;
      break;
    }
    default: break;
  }
  */
  delete table;
  table = nullptr;
  type = Object_NONE;
}

Thanks

[aardappel]

1) Do you have the latest GRPC? It seems that the current signature for Deserialize has 2 arguments, which changed recently.

2) I see there are memory errors, but those could also result from your usage. For example, you're calling std::move on a raw pointer (the result of UnPack), which I am not even sure what that would do. You're better of calling UnpackSampleRoot instead, which gives you a unique_ptr, which is probably safer.

Also, not sure why you're using the object API, the standard API is much more efficient for your use case.

What would your suggested change to ObjectUnion::Reset() improve? You'd be calling delete on the wrong pointer type. These types have no vtables to dynamically call the right destructor.

[MizukiSonoko]

1. Sorry, update grpc and compile successful !

2. I wrote

   // auto sampleRootT = std::move(*request->GetRoot()->UnPack()); 
   auto sampleRootT = UnPackSampleRoot(request);

   But

   ==18640==
   ==18640==
   ==18640== Process terminating with default action of signal 11 (SIGSEGV)
   ==18640==  Access not within mapped region at address 0x101A4FE0
   ==18640==    at 0x410F1E: int flatbuffers::ReadScalar<int>(void const*) (in /tmp/sandbox/grpctest)
   ==18640==    by 0x40D4AC: flatbuffers::Table::GetVTable() const (in /tmp/sandbox/grpctest)
   ==18640==    by 0x40D4D9: flatbuffers::Table::GetOptionalFieldOffset(unsigned short) const (in /tmp/sandbox/grpctest)
   ==18640==    by 0x413701: flatbuffers::Vector<flatbuffers::Offset<sample::BaseObject> > const* flatbuffers::Table::GetPointer<flatbuffers::Vector<flatbuffers::Offset<sample::BaseObject> > const*>(unsigned short) (in /tmp/sandbox/grpctest)
   ==18640==    by 0x411613: flatbuffers::Vector<flatbuffers::Offset<sample::BaseObject> > const* flatbuffers::Table::GetPointer<flatbuffers::Vector<flatbuffers::Offset<sample::BaseObject> > const*>(unsigned short) const (in /tmp/sandbox/grpctest)
   ==18640==    by 0x40E568: sample::SampleRoot::baseObjects() const (in /tmp/sandbox/grpctest)
   ==18640==    by 0x40F507: sample::SampleRoot::UnPackTo(sample::SampleRootT*, std::function<void (void**, unsigned long)> const*) const (in /tmp/sandbox/grpctest)
   ==18640==    by 0x40F4CC: sample::SampleRoot::UnPack(std::function<void (void**, unsigned long)> const*) const (in /tmp/sandbox/grpctest)
   ==18640==    by 0x40FF2E: sample::UnPackSampleRoot(void const*, std::function<void (void**, unsigned long)> const*) (in /tmp/sandbox/grpctest)
   ==18640==    by 0x40FFEB: ServiceImpl::Endpoint(grpc::ServerContext*, flatbuffers::BufferRef<sample::SampleRoot> const*, flatbuffers::BufferRef<sample::Response>*) (in /tmp/sandbox/grpctest)
   ==18640==    by 0x40896E: grpc::Status std::_Mem_fn_base<grpc::Status (sample::SampleService::Service::*)(grpc::ServerContext*, flatbuffers::BufferRef<sample::SampleRoot> const*, flatbuffers::BufferRef<sample::Response>*), true>::operator()<grpc::ServerContext*, flatbuffers::BufferRef<sample::SampleRoot> const*, flatbuffers::BufferRef<sample::Response>*, void>(sample::SampleService::Service*, grpc::ServerContext*&&, flatbuffers::BufferRef<sample::SampleRoot> const*&&, flatbuffers::BufferRef<sample::Response>*&&) const (in /tmp/sandbox/grpctest)
   ==18640==    by 0x407ED6: std::_Function_handler<grpc::Status (sample::SampleService::Service*, grpc::ServerContext*, flatbuffers::BufferRef<sample::SampleRoot> const*, flatbuffers::BufferRef<sample::Response>*), std::_Mem_fn<grpc::Status (sample::SampleService::Service::*)(grpc::ServerContext*, flatbuffers::BufferRef<sample::SampleRoot> const*, flatbuffers::BufferRef<sample::Response>*)> >::_M_invoke(std::_Any_data const&, sample::SampleService::Service*&&, grpc::ServerContext*&&, flatbuffers::BufferRef<sample::SampleRoot> const*&&, flatbuffers::BufferRef<sample::Response>*&&) (in /tmp/sandbox/grpctest)
   ==18640==  If you believe this happened as a result of a stack
   ==18640==  overflow in your program's main thread (unlikely but
   ==18640==  possible), you can try to increase the size of the
   ==18640==  main thread stack using the --main-stacksize= flag.
   ==18640==  The main thread stack size used in this run was 8388608.
   ==18640==
   ==18640== HEAP SUMMARY:
   ==18640==     in use at exit: 4,597,615 bytes in 262 blocks
   ==18640==   total heap usage: 582 allocs, 320 frees, 4,641,389 bytes allocated
   ==18640==
   ==18640== LEAK SUMMARY:
   ==18640==    definitely lost: 0 bytes in 0 blocks
   ==18640==    indirectly lost: 0 bytes in 0 blocks
   ==18640==      possibly lost: 788,133 bytes in 9 blocks
   ==18640==    still reachable: 3,809,482 bytes in 253 blocks
   ==18640==         suppressed: 0 bytes in 0 blocks
   ==18640== Rerun with --leak-check=full to see details of leaked memory

   Sorry...

I think some approach.

First, I have question, Why ***Union is not extend "flatbuffers::NativeTable"? I think table's type is flatbuffers::NativeTable, so I should write this.

struct ObjectUnion: public flatbuffers::NativeTable

If above approach is no problem, I will rewrite this https://github.com/google/flatbuffers/blob/2df3d1c9654ab0ddd8c58be51609dbad86109b15/src/idl_gen_cpp.cpp#L649 and Pull request.

And

==15195==
==15195== Invalid free() / delete / delete[] / realloc()
==15195==    at 0x4C2F24B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15195==    by 0x40F226: sample::ObjectUnion::Reset() (in /tmp/sandbox/grpctest)
==15195==    by 0x40D4B9: sample::ObjectUnion::~ObjectUnion() (in /tmp/sandbox/gr

It's cause by double free. So I think I should write it

inline void ObjectUnion::Reset() {
  if(table != nullptr){ // Add check 
    switch (type) {
      case Object_Object1: {
        auto ptr = reinterpret_cast<Object1T *>(table);
        delete ptr;
        break;
      }
      case Object_Object2: {
        auto ptr = reinterpret_cast<Object2T *>(table);
        delete ptr;
        break;
      }
      case Object_Object3: {
        auto ptr = reinterpret_cast<Object3T *>(table);
        delete ptr;
        break;
      }
      default: break;
    }
  }
  table = nullptr;
  type = Object_NONE;
}

In now, there is memory leak.

==18532== 56 bytes in 1 blocks are definitely lost in loss record 3 of 8
==18532==    at 0x4C2E0EF: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18532==    by 0x412532: void sample::WrapperUnion::Set<sample::Wrapper1T>(sample::Wrapper1T&&) (in /tmp/sandbox/grpctest)
==18532==    by 0x40BB1D: main (in /tmp/sandbox/grpctest)

==18532== LEAK SUMMARY:
==18532==    definitely lost: 324 bytes in 4 blocks
==18532==    indirectly lost: 128 bytes in 2 blocks
==18532==      possibly lost: 0 bytes in 0 blocks
==18532==    still reachable: 72,728 bytes in 2 blocks

I'm fixing now. thanks.

[aardappel]

Your SIGSEGV you posted is happening while reading a FlatBuffer, so has unrelated to the union. You may fist want to find out why that is happening. It sounds like the memory pointed to by request is somehow not valid, or contains something of a different type.

Why ***Union is not extend "flatbuffers::NativeTable"? Because unions are not tables. They are a pair of a type and a table pointer.

Doing a delete nullptr is valid C++ (see e.g. http://stackoverflow.com/questions/6731331/is-it-still-safe-to-delete-nullptr-in-c0x), so I am not sure why we should add an if-check just to please valgrind (are these messages coming from valgrind?).

I still don't understand where your leak comes from. Do you have more information?

[lebdron]

• UnPackSampleRoot should not be used on request, since it requires buffer as argument. Moving an object pointed by request->GetRoot()->UnPack() also does not seem like a good solution to me, since it introduces a memory leak by not freeing memory allocated in UnPack. The solution that worked for me:

  
  auto sampleRootT = request->GetRoot()->UnPack();

assert(sampleRootT->wrapper.AsWrapper1() != nullptr); assert(sampleRootT->wrapper.AsWrapper1()->object.AsObject1() != nullptr); assert(sampleRootT->wrapper.AsWrapper1()->object.AsObject1()->text == "object1_text"); assert(sampleRootT->wrapper.AsWrapper1()->object.AsObject1()->boolean);

delete sampleRootT;

- Double free problem described in https://github.com/MizukiSonoko/test_flatbuffers/blob/master/README.md is caused by move constructor of union leaving the pointer in the old object. Hopefully this gdb log will describe it.

108 wrapper1T.object.Set(std::move(object1T)); (gdb) 110 auto sampleRootT = SampleRootT(); (gdb) p object1T $1 = { = {}, text = "", boolean = true} (gdb) p (Object1T)wrapper1T.object.table $3 = { = {}, text = "object1_text", boolean = true} (gdb) 111 sampleRootT.wrapper.Set(std::move(wrapper1T)); (gdb) 113 flatbuffers::FlatBufferBuilder fbb; (gdb) p wrapper1T $4 = { = {}, object = {type = sample::Object_Object1, table = 0x636310}} (gdb) p (Wrapper1T)sampleRootT.wrapper.table $6 = { = {}, object = {type = sample::Object_Object1, table = 0x636310}}

As you can see, object1T is moved correctly, while wrapper1T data is still present in old object.

[aardappel]

Thanks @lebdron :)