The initial motivation for this change was to fix a bug found by fuzzing. The old fuzz test (built on `cc_fuzz_target()`) detected an infinite loop if a bytes field default has an unterminated `\x` escape. To fix this bug while expanding fuzz coverage, I created a fuzz test that verifies that we can do a lossless round trip from descriptor -> DefPool -> descriptor. We use C++ as the source of truth for whether a descriptor is valid or not, and what the canonical serialization back to protobuf form should be. I wrote the new fuzz test using go/FuzzTest, which makes it easier and more readable to use an arbitrary `FileDescriptorSet` as input, while adding test cases for regressions. The fuzz test highlighted a handful of errors that I subsequently fixed and added regression tests for: 1. The aforementioned unterminated `\x` bug. 2. We were not propagating the `edition` field. 3. We were missing the CheckIdent() check in a few places. 4. We were rejecting files with empty name, whereas C++ allows this. 5. There were a few bugs with escaping string defaults. Since FuzzTest is Clang-only, I split the `FUZZ_TEST()` invocation from the regression tests, since the latter are portable and should be run on all platforms. Only `FUZZ_TEST()` itself is in a google3/Clang-only file. PiperOrigin-RevId: 506997362pull/13171/head
Joshua Haberman
2 years ago
committed by
Copybara-Service
parent
0cc83d1adf
commit
6e1cbdfe09
19 changed files with 517 additions and 113 deletions
@ -0,0 +1,37 @@ |
||||
#ifndef UPB_UPB_TEST_PARSE_TEXT_PROTO_H_ |
||||
#define UPB_UPB_TEST_PARSE_TEXT_PROTO_H_ |
||||
|
||||
#include <string> |
||||
|
||||
#include "gtest/gtest.h" |
||||
#include "google/protobuf/message.h" |
||||
#include "google/protobuf/text_format.h" |
||||
|
||||
namespace upb_test { |
||||
|
||||
// Replacement for Google ParseTextProtoOrDie.
|
||||
// Only to be used in unit tests.
|
||||
// Usage: MyMessage msg = ParseTextProtoOrDie(my_text_proto);
|
||||
class ParseTextProtoOrDie { |
||||
public: |
||||
explicit ParseTextProtoOrDie(absl::string_view text_proto) |
||||
: text_proto_(text_proto) {} |
||||
|
||||
template <class T> |
||||
operator T() { // NOLINT: Needed to support parsing text proto as appropriate
|
||||
// type.
|
||||
T message; |
||||
if (!google::protobuf::TextFormat::ParseFromString(text_proto_, &message)) { |
||||
ADD_FAILURE() << "Failed to parse textproto: " << text_proto_; |
||||
abort(); |
||||
} |
||||
return message; |
||||
} |
||||
|
||||
private: |
||||
std::string text_proto_; |
||||
}; |
||||
|
||||
} // namespace upb_test
|
||||
|
||||
#endif // UPB_UPB_TEST_PARSE_TEXT_PROTO_H_
|
||||
@ -0,0 +1,26 @@ |
||||
|
||||
#include <string> |
||||
|
||||
#include "google/protobuf/descriptor.proto.h" |
||||
#include "gtest/gtest.h" |
||||
#include "testing/fuzzing/fuzztest.h" |
||||
#include "upb/util/def_to_proto_test.h" |
||||
|
||||
namespace upb_test { |
||||
|
||||
FUZZ_TEST(FuzzTest, RoundTripDescriptor) |
||||
.WithDomains( |
||||
::fuzztest::Arbitrary<google::protobuf::FileDescriptorSet>().WithProtobufField( |
||||
"file", |
||||
::fuzztest::Arbitrary<google::protobuf::FileDescriptorProto>() |
||||
// upb_FileDef_ToProto() does not attempt to preserve
|
||||
// source_code_info.
|
||||
.WithFieldUnset("source_code_info") |
||||
.WithProtobufField( |
||||
"service", |
||||
::fuzztest::Arbitrary<google::protobuf::ServiceDescriptorProto>() |
||||
// streams are google3-only, and we do not currently
|
||||
// attempt to preserve them.
|
||||
.WithFieldUnset("stream")))); |
||||
|
||||
} // namespace upb_test
|
||||
@ -0,0 +1,117 @@ |
||||
#ifndef UPB_UTIL_DEF_TO_PROTO_TEST_H_ |
||||
#define UPB_UTIL_DEF_TO_PROTO_TEST_H_ |
||||
|
||||
#include <string> |
||||
|
||||
#include "google/protobuf/descriptor.pb.h" |
||||
#include "google/protobuf/descriptor.upb.h" |
||||
#include "gmock/gmock.h" |
||||
#include "gtest/gtest.h" |
||||
#include "google/protobuf/descriptor.h" |
||||
#include "google/protobuf/dynamic_message.h" |
||||
#include "google/protobuf/util/field_comparator.h" |
||||
#include "upb/reflection/def.hpp" |
||||
#include "upb/upb.hpp" |
||||
#include "upb/util/def_to_proto.h" |
||||
|
||||
namespace upb_test { |
||||
|
||||
// A gtest matcher that verifies that a proto is equal to `proto`. Both `proto`
|
||||
// and `arg` must be messages of type `msgdef_func` (a .upbdefs.h function that
|
||||
// loads a known msgdef into the given defpool).
|
||||
MATCHER_P(EqualsProtoTreatNansAsEqual, proto, |
||||
negation ? "are not equal" : "are equal") { |
||||
upb::DefPool defpool; |
||||
google::protobuf::DescriptorPool pool; |
||||
google::protobuf::DynamicMessageFactory factory; |
||||
std::string differences; |
||||
google::protobuf::util::DefaultFieldComparator comparator; |
||||
comparator.set_treat_nan_as_equal(true); |
||||
google::protobuf::util::MessageDifferencer differencer; |
||||
differencer.set_field_comparator(&comparator); |
||||
differencer.ReportDifferencesToString(&differences); |
||||
bool eq = differencer.Equals(proto, arg); |
||||
if (!eq) { |
||||
*result_listener << differences; |
||||
} |
||||
return eq; |
||||
} |
||||
|
||||
class NullErrorCollector : public google::protobuf::DescriptorPool::ErrorCollector { |
||||
void AddError(const std::string& filename, const std::string& element_name, |
||||
const google::protobuf::Message* descriptor, ErrorLocation location, |
||||
const std::string& message) override {} |
||||
void RecordWarning(absl::string_view filename, absl::string_view element_name, |
||||
const google::protobuf::Message* descriptor, ErrorLocation location, |
||||
absl::string_view message) override {} |
||||
}; |
||||
|
||||
static void AddFile(google::protobuf::FileDescriptorProto& file, upb::DefPool* pool, |
||||
google::protobuf::DescriptorPool* desc_pool) { |
||||
NullErrorCollector collector; |
||||
const google::protobuf::FileDescriptor* file_desc = |
||||
desc_pool->BuildFileCollectingErrors(file, &collector); |
||||
|
||||
if (file_desc != nullptr) { |
||||
// The file descriptor was valid according to proto2.
|
||||
google::protobuf::FileDescriptorProto normalized_file; |
||||
file_desc->CopyTo(&normalized_file); |
||||
std::string serialized; |
||||
normalized_file.SerializeToString(&serialized); |
||||
upb::Arena arena; |
||||
upb::Status status; |
||||
google_protobuf_FileDescriptorProto* proto = google_protobuf_FileDescriptorProto_parse( |
||||
serialized.data(), serialized.size(), arena.ptr()); |
||||
ASSERT_NE(proto, nullptr); |
||||
upb::FileDefPtr file_def = pool->AddFile(proto, &status); |
||||
|
||||
// Ideally we could assert that file_def is present here. After all, any
|
||||
// descriptor accepted by C++ should be by definition valid. However C++
|
||||
// performs some of its validation at the .proto file parser level instead
|
||||
// of when validating descriptors. As as result, C++ will accept some
|
||||
// unreasonable descriptors like:
|
||||
// file { name: "" package: "0" }
|
||||
//
|
||||
// There is no .proto file that will produce this descriptor, but
|
||||
// BuildFile() accepts it. We should probably clean up these cases so C++
|
||||
// will reject them too.
|
||||
if (!file_def) return; |
||||
|
||||
ASSERT_TRUE(status.ok()) << status.error_message(); |
||||
google_protobuf_FileDescriptorProto* upb_proto = |
||||
upb_FileDef_ToProto(file_def.ptr(), arena.ptr()); |
||||
size_t size; |
||||
const char* buf = |
||||
google_protobuf_FileDescriptorProto_serialize(upb_proto, arena.ptr(), &size); |
||||
google::protobuf::FileDescriptorProto google_proto; |
||||
bool ok = google_proto.ParseFromArray(buf, size); |
||||
ASSERT_TRUE(ok); |
||||
EXPECT_THAT(google_proto, EqualsProtoTreatNansAsEqual(normalized_file)); |
||||
} else { |
||||
// This file was invalid according to proto2. When we parse it with upb,
|
||||
// it may or may not be accepted, since upb does not perform as much
|
||||
// validation as proto2. However it must not crash.
|
||||
std::string serialized; |
||||
file.SerializeToString(&serialized); |
||||
upb::Arena arena; |
||||
upb::Status status; |
||||
google_protobuf_FileDescriptorProto* proto = google_protobuf_FileDescriptorProto_parse( |
||||
serialized.data(), serialized.size(), arena.ptr()); |
||||
ASSERT_NE(proto, nullptr); |
||||
pool->AddFile(proto, &status); |
||||
} |
||||
} |
||||
|
||||
inline void RoundTripDescriptor(const google::protobuf::FileDescriptorSet& set) { |
||||
upb::DefPool defpool; |
||||
google::protobuf::DescriptorPool desc_pool; |
||||
desc_pool.EnforceWeakDependencies(true); |
||||
for (const auto& file : set.file()) { |
||||
google::protobuf::FileDescriptorProto mutable_file(file); |
||||
AddFile(mutable_file, &defpool, &desc_pool); |
||||
} |
||||
} |
||||
|
||||
} // namespace upb_test
|
||||
|
||||
#endif // UPB_UTIL_DEF_TO_PROTO_TEST_H_
|
||||
Loading…
Reference in new issue