protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard
Published 1 month ago • verified publisher icon protocolbuffers

C++: Valgrind: map.InitializationErrorString(): jump or move depends on uninitialised value

Open chrisse74 opened this issue 1 year ago • 3 comments

Calling InitializationErrorString() on a Protobuf map leads to Valgrind reporting a 'jump or move depends on uninitialised value'.

Note: We get the same error when calling DebugString() instead of InitializationErrorString().

What version of protobuf and what language are you using? Version: v29.2 Language: C++

What operating system (Linux, Windows, ...) and version? Red Hat Enterprise Linux 9.5

What runtime / compiler are you using (e.g., python version or gcc version) clang 19.1.6

What did you do? Steps to reproduce the behavior: We added debug symbols to the cmake release build to come up with the corresponding line numbers.

Sample Proto File:

syntax = "proto3";

option java_multiple_files = true;

package my.idl;

// This is a Map type documentation for StringMap
message StringMap
{
    map<string,string> string_map = 1;
}

Sample source file:

#include "MyTypesProto.grpc.pb.h"

int main(void)
{
    my::idl::StringMap stringMap;
    const std::string stringValue("test1234");
    stringMap.mutable_string_map()->insert({ stringValue, stringValue });

    stringMap.InitializationErrorString();

    google::protobuf::ShutdownProtobufLibrary();
}

What did you expect to see No errors detected

What did you see instead? See

Likely candidate is the is_repeated() call which is reported first in https://github.com/protocolbuffers/protobuf/blob/233098326bc268fc03b28725c941519fc77703e6/src/google/protobuf/descriptor.cc#L6646

==1000== Conditional jump or move depends on uninitialised value(s)
==1000==    at 0x1BC369: google::protobuf::DescriptorBuilder::BuildFieldOrExtension(google::protobuf::FieldDescriptorProto const&, google::protobuf::Descriptor*, google::protobuf::FieldDescriptor*, bool, google::protobuf::internal::FlatAllocator&) (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:6646)
==1000==    by 0x1B6D0E: BuildField (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:4420)
==1000==    by 0x1B6D0E: google::protobuf::DescriptorBuilder::BuildMessage(google::protobuf::DescriptorProto const&, google::protobuf::Descriptor const*, google::protobuf::Descriptor*, google::protobuf::internal::FlatAllocator&) (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:6349)
==1000==    by 0x1B51CB: google::protobuf::DescriptorBuilder::BuildFileImpl(google::protobuf::FileDescriptorProto const&, google::protobuf::internal::FlatAllocator&) (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:6129)
==1000==    by 0x1AFBF8: google::protobuf::DescriptorBuilder::BuildFile(google::protobuf::FileDescriptorProto const&) (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:5893)
==1000==    by 0x1A585D: operator() (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:4750)
==1000==    by 0x1A585D: google::protobuf::DescriptorPool::BuildFileFromDatabase(google::protobuf::FileDescriptorProto const&, google::protobuf::DescriptorPool::DeferredValidation&) const (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:4755)
==1000==    by 0x1A3CC1: google::protobuf::DescriptorPool::TryFindFileInFallbackDatabase(std::__1::basic_string_view<char, std::__1::char_traits<char> >, google::protobuf::DescriptorPool::DeferredValidation&) const (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:2717)
==1000==    by 0x1A3BC5: google::protobuf::DescriptorPool::FindFileByName(std::__1::basic_string_view<char, std::__1::char_traits<char> >) const (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:2274)
==1000==    by 0x23520D: google::protobuf::(anonymous namespace)::AssignDescriptorsImpl(google::protobuf::internal::DescriptorTable const*, bool) (grpc-1.69.0/third_party/protobuf/src/google/protobuf/generated_message_reflection.cc:3666)
==1000==    by 0x275DCB: operator() (grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:168)
==1000==    by 0x275DCB: __invoke<(lambda at grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:167:35)> (sharecompiler.HEAD/linux/clang19el9/bin/../include/c++/v1/__type_traits/invoke.h:149)
==1000==    by 0x275DCB: invoke<(lambda at grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:167:35)> (sharecompiler.HEAD/linux/clang19el9/bin/../include/c++/v1/__functional/invoke.h:28)
==1000==    by 0x275DCB: void absl::lts_20240722::base_internal::CallOnceImpl<google::protobuf::Message::GetMetadataImpl(google::protobuf::internal::ClassDataFull const&)::$_0>(std::__1::atomic<unsigned int>*, absl::lts_20240722::base_internal::SchedulingMode, google::protobuf::Message::GetMetadataImpl(google::protobuf::internal::ClassDataFull const&)::$_0&&) (grpc-1.69.0/third_party/abseil-cpp/absl/base/call_once.h:182)
==1000==    by 0x2740EF: call_once<(lambda at grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:167:35)> (grpc-1.69.0/third_party/abseil-cpp/absl/base/call_once.h:216)
==1000==    by 0x2740EF: google::protobuf::Message::GetMetadataImpl(google::protobuf::internal::ClassDataFull const&) (grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:167)
==1000==    by 0x1A391E: GetDescriptor (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.pb.h:10890)
==1000==    by 0x1A391E: descriptor (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.pb.h:10887)
==1000==    by 0x1A391E: google::protobuf::DescriptorPool::generated_pool() (grpc-1.69.0/third_party/protobuf/src/google/protobuf/descriptor.cc:2217)
==1000==    by 0x235126: MaybeInitializeLazyDescriptors (grpc-1.69.0/third_party/protobuf/src/google/protobuf/generated_message_reflection.cc:3693)
==1000==    by 0x235126: google::protobuf::internal::AssignDescriptorsOnceInnerCall(google::protobuf::internal::DescriptorTable const*) (grpc-1.69.0/third_party/protobuf/src/google/protobuf/generated_message_reflection.cc:3731)
==1000==    by 0x275DCB: operator() (grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:168)
==1000==    by 0x275DCB: __invoke<(lambda at grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:167:35)> (sharecompiler.HEAD/linux/clang19el9/bin/../include/c++/v1/__type_traits/invoke.h:149)
==1000==    by 0x275DCB: invoke<(lambda at grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:167:35)> (sharecompiler.HEAD/linux/clang19el9/bin/../include/c++/v1/__functional/invoke.h:28)
==1000==    by 0x275DCB: void absl::lts_20240722::base_internal::CallOnceImpl<google::protobuf::Message::GetMetadataImpl(google::protobuf::internal::ClassDataFull const&)::$_0>(std::__1::atomic<unsigned int>*, absl::lts_20240722::base_internal::SchedulingMode, google::protobuf::Message::GetMetadataImpl(google::protobuf::internal::ClassDataFull const&)::$_0&&) (grpc-1.69.0/third_party/abseil-cpp/absl/base/call_once.h:182)
==1000==    by 0x274092: call_once<(lambda at grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:167:35)> (grpc-1.69.0/third_party/abseil-cpp/absl/base/call_once.h:216)
==1000==    by 0x274092: GetMetadataImpl (grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:167)
==1000==    by 0x274092: google::protobuf::Message::GetMetadata() const (grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:156)
==1000==    by 0x282FE0: GetDescriptor (grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.h:362)
==1000==    by 0x282FE0: google::protobuf::internal::ReflectionOps::FindInitializationErrors(google::protobuf::Message const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >*) (grpc-1.69.0/third_party/protobuf/src/google/protobuf/reflection_ops.cc:382)
==1000==    by 0x273E65: FindInitializationErrors (grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:136)
==1000==    by 0x273E65: google::protobuf::Message::InitializationErrorString() const (grpc-1.69.0/third_party/protobuf/src/google/protobuf/message.cc:141)
==1000==    by 0x19BF94: main (main.cpp:9)

Anything else we should know about your project / environment It affects us as gRPC 1.69.0 calls InitializationErrorString() in grpcpp/impl/generic_serialize.h (line 84)

chrisse74 avatar Jan 17 '25 12:01 chrisse74

I believe this must be a valgrind false positive.

is_repeated() is defined as:

inline bool FieldDescriptor::is_repeated() const {
  ABSL_DCHECK_EQ(is_repeated_, label() == LABEL_REPEATED);
  return is_repeated_;
}

Both label and is_repeated_ are always assigned above this line in the same function.

I suspect this is a false positive based on is_repeated_ is a bit field (the :1 syntax on the definition site), valgrind is confused because other adjacent bit fields are uninitialized, and then the valgrind path doesn't understand that uninitialized adjacent bits are not (in the C++ semantics) part of the comparison that is happening. But, I can't immediately find a reference to valgrind having this false positive case, so we may want to double check that.

esrauchg avatar Jan 17 '25 14:01 esrauchg

The error goes away when a libprotobuf which was compiled without optimizations is used. (O1 does trigger it)

chrisse74 avatar Jan 20 '25 07:01 chrisse74

In fact there are various valgrind Bugs regarding bitfields.

  • https://bugs.kde.org/show_bug.cgi?id=232872 (false positive uninit for float=0.0 if an unrelated bitfield was touched earlier)
  • https://bugs.kde.org/show_bug.cgi?id=406674 (False positive when reading bitfield value on code compiled with clang 7.0)
  • https://bugs.kde.org/show_bug.cgi?id=117362 (false positive uninit for bitfields or partial word)
    • see also RedHat Bug https://bugzilla.redhat.com/show_bug.cgi?id=174324 (re_compile_fastmap_iter uses uninit value)

Maybe an initial clearing of the bitfield structure would be possible to mitigate this?

chrisse74 avatar Apr 02 '25 08:04 chrisse74

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago. This issue will be closed and archived after 14 additional days without activity.

github-actions[bot] avatar Sep 11 '25 10:09 github-actions[bot]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

github-actions[bot] avatar Sep 25 '25 10:09 github-actions[bot]