Skip to content

Allow unknown fields in corpus parser#170

Merged
vitalybuka merged 3 commits intogoogle:masterfrom
nareddyt:unknown-fields
Apr 23, 2020
Merged

Allow unknown fields in corpus parser#170
vitalybuka merged 3 commits intogoogle:masterfrom
nareddyt:unknown-fields

Conversation

@nareddyt
Copy link
Contributor

As discussed in envoyproxy/envoy#10796, this will allow breaking wire-compatibility changes in the input proto. The pre-existing corpus will still function, but old fields will be ignored.

Risk: Typos in the text proto will cause the fuzzer to run on an incomplete proto. Previously, this would log an error message and skip fuzzing with that test case.

Testing: I'm not sure how to add it to mutator_test.cc. I manually tested that unknown text format files will be parsed without ParseTextMessage returning false.

As discussed in envoyproxy/envoy#10796, this will allow breaking wire-compatibility changes in the input proto. The pre-existing corpus will still function, but old fields will be ignored.

Risk: Typos in the text proto will cause the fuzzer to run on an incomplete proto. Previously, this would log an error message and skip fuzzing with that test case.
@nareddyt
Copy link
Contributor Author

cc @asraa @vitalybuka

@nareddyt
Copy link
Contributor Author

nareddyt commented Apr 17, 2020

You may want to check this out and run the tests to verify my PR. I keep getting a test failure on my workstation:

96% tests passed, 1 tests failed out of 28

Total Test time (real) = 288.44 sec

The following tests FAILED:
         27 - test.libfuzzer_example_test (Failed)
Errors while running CTest
ninja: build stopped: subcommand failed.

Edit: Oh, it happens on master too. Nevermind

Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt nareddyt requested a review from vitalybuka April 20, 2020 17:05
unknown_field: "test unknown field"
)";

const char kUnknownFieldExpected[] = R"(optional_bool: true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline matters here FYI (there shouldn't be one)

@vitalybuka
Copy link
Collaborator

vitalybuka commented Apr 24, 2020

Looks like we need to rollback this and fix issue in proto

Now it triggers stack overflows like

  | #5 0x816706 in google::protobuf::TextFormat::Parser::ParserImpl::SkipFieldValue() libprotobuf-mutator/build/external.protobuf/src/external.protobuf/src/google/protobuf/text_format.cc:827:9
  | #6 0x816d39 in google::protobuf::TextFormat::Parser::ParserImpl::SkipFieldValue() libprotobuf-mutator/build/external.protobuf/src/external.protobuf/src/google/protobuf/text_format.cc:830:11
  | #7 0x816d39 in google::protobuf::TextFormat::Parser::ParserImpl::SkipFieldValue() libprotobuf-mutator/build/external.protobuf/src/external.protobuf/src/google/protobuf/text_format.cc:830:11
  | #8 0x816d39 in google::protobuf::TextFormat::Parser::ParserImpl::SkipFieldValue() libprotobuf-mutator/build/external.protobuf/src/external.protobuf/src/google/protobuf/text_format.cc:830:11
  | #9 0x816d39 in google::protobuf::TextFormat::Parser::ParserImpl::SkipFieldValue() libprotobuf-mutator/build/external.protobuf/src/external.protobuf/src/google/protobuf/text_format.cc:830:11
  | #10 0x816d39 in google::protobuf::TextFormat::Parser::ParserImpl::SkipFieldValue() libprotobuf-mutator/build/external.protobuf/src/external.protobuf/src/google/protobuf/text_format.cc:830:11
  | #11 0x816d39 in google::protobuf::TextFormat::Parser::ParserImpl::SkipFieldValue() libprotobuf-mutator/build/external.protobuf/src/external.protobuf/src/google/protobuf/text_format.cc:830:11
  | #12 0x816d39 in google::protobuf::TextFormat::Parser::ParserImpl::SkipFieldValue() libprotobuf-mutator/build/external.protobuf/src/external.protobuf/src/google/protobuf/text_format.cc:830:11

@nareddyt nareddyt deleted the unknown-fields branch April 24, 2020 20:30
@nareddyt nareddyt mentioned this pull request May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants