Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fuzz] fix fuzz crashes in fmt format #10935

Merged
merged 8 commits into from
May 15, 2020
Merged

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Apr 24, 2020

Commit description: Fixes fuzz crashes in fmt::format (https://github.com/fmtlib/fmt/blob/0463665ef136d685fe07a564d93c782456456d3d/include/fmt/format.h#L703) on certain invalid protobuf inputs. fmt is patches with PR (fmtlib/fmt#1650) that replaces the in-house fuzzing resource management to an fmt specific fuzzing macro.
Additional Description: The regression test added shows that the proto in question is not unreasonably huge for Envoy. This is causing a high unexplained crash percentage for many fuzz tests on OSS-Fuzz. Also bump fmt.

Testing: Added regression test in server fuzz test, failed
bazel test test/server:server_fuzz_test --copt=-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION before
Related: #9623
Risk level: Low

Signed-off-by: Asra Ali [email protected]

@asraa
Copy link
Contributor Author

asraa commented Apr 24, 2020

@nareddyt

nareddyt
nareddyt previously approved these changes Apr 24, 2020
@@ -250,8 +250,8 @@ MissingFieldException::MissingFieldException(const std::string& field_name,

ProtoValidationException::ProtoValidationException(const std::string& validation_error,
const Protobuf::Message& message)
: EnvoyException(fmt::format("Proto constraint validation failed ({}): {}", validation_error,
message.DebugString())) {
: EnvoyException(absl::StrCat("Proto constraint validation failed (", validation_error,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@htuch FYI just limiting the validation_error and message.DebugString() to 10 characters doesn't solve the problem -- there's some other reason the the buffer allocation increases besides a large string

Copy link
Member

Choose a reason for hiding this comment

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

Weird. Is it possible to get a heap profile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, nothing obvious. I am starting to think this is a by-product of fuzz runner environment setup. Running all server config tests with -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION only produced failures in server/config fuzz tests, no other tests.

FYI the build fix (fmtlib/fmt#1650) will solve this problem and I could close out this PR if we patch fmt with this commit, but I think there is something weird if this is only happening in fuzz mode.

Some binary filenames not available. Symbolization may be incomplete.
Try setting PPROF_BINARY_PATH to the search path for local binaries.
File: server_fuzz_test
Type: inuse_space
Showing nodes accounting for 856.05kB, 98.73% of 867.06kB total
Dropped 170 nodes (cum <= 4.33kB)
      flat  flat%   sum%        cum   cum%
  219.88kB 25.36% 25.36%   219.88kB 25.36%  std::allocator_traits::allocate
  138.02kB 15.92% 41.28%   142.02kB 16.38%  google::protobuf::DescriptorPool::Tables::AllocateBytes
     120kB 13.84% 55.12%      120kB 13.84%  std::basic_filebuf::_M_allocate_internal_buffer
   88.41kB 10.20% 65.31%   150.37kB 17.34%  google::protobuf::DescriptorPool::Tables::AllocateString
      71kB  8.19% 73.50%       71kB  8.19%  _GLOBAL__sub_I_eh_alloc.cc
   48.75kB  5.62% 79.13%    48.75kB  5.62%  std::__cxx11::basic_string::_M_construct
   47.03kB  5.42% 84.55%    47.03kB  5.42%  std::__cxx11::basic_string::_M_mutate
   31.62kB  3.65% 88.20%    31.62kB  3.65%  google::protobuf::DescriptorPool::Tables::AllocateEmptyString[abi:cxx11]
   22.77kB  2.63% 90.82%    22.77kB  2.63%  google::protobuf::Arena::CreateMaybeMessage
   21.94kB  2.53% 93.35%    23.94kB  2.76%  google::protobuf::DescriptorPool::Tables::AllocateMessage
   15.67kB  1.81% 95.16%    27.45kB  3.17%  testing::internal::MakeAndRegisterTestInfo
    8.76kB  1.01% 96.17%    11.26kB  1.30%  google::protobuf::DescriptorPool::Tables::AllocateFileTables
    7.97kB  0.92% 97.09%     7.97kB  0.92%  google::protobuf::internal::ExtensionSet::GrowCapacity
    4.49kB  0.52% 97.61%     4.49kB  0.52%  std::__cxx11::basic_string::reserve
    4.38kB   0.5% 98.11%     4.38kB   0.5%  google::protobuf::AssignDescriptorsHelper::AssignMessageDescriptor
    2.30kB  0.27% 98.38%     7.05kB  0.81%  testing::internal::TestMetaFactory::CreateTestFactory
    1.84kB  0.21% 98.59%    10.73kB  1.24%  testing::TestInfo::TestInfo
    1.09kB  0.13% 98.72%     5.09kB  0.59%  google::protobuf::DescriptorPool::Tables::AllocateOnceDynamic
    0.08kB 0.009% 98.72%   401.13kB 46.26%  Envoy::Fuzz::Runner::setupEnvironment
    0.02kB 0.0027% 98.73%    11.27kB  1.30%  Envoy::Logger::Registry::allLoggers
    0.02kB 0.0027% 98.73%    10.63kB  1.23%  std::__shared_count::__shared_count
         0     0% 98.73%    12.74kB  1.47%  Envoy::Logger::Context::Context
         0     0% 98.73%    12.70kB  1.47%  Envoy::Logger::Context::activate
         0     0% 98.73%    11.27kB  1.30%  Envoy::Logger::Registry::setLogLevel
         0     0% 98.73%    10.55kB  1.22%  Envoy::Logger::StandardLogger::StandardLogger
         0     0% 98.73%   387.71kB 44.72%  Envoy::ProcessWide::ProcessWide
         0     0% 98.73%   387.59kB 44.70%  Envoy::Server::validateProtoDescriptors
         0     0% 98.73%     5.52kB  0.64%  __gnu_cxx::new_allocator::construct
         0     0% 98.73%   443.17kB 51.11%  __libc_start_main
         0     0% 98.73%   176.42kB 20.35%  __pthread_once_slow
         0     0% 98.73%   115.70kB 13.34%  google::protobuf::(anonymous namespace)::AssignDescriptorsImpl
         0     0% 98.73%   112.55kB 12.98%  google::protobuf::DescriptorBuilder::AddSymbol
         0     0% 98.73%   135.71kB 15.65%  google::protobuf::DescriptorBuilder::AllocateArray
         0     0% 98.73%    28.50kB  3.29%  google::protobuf::DescriptorBuilder::AllocateNameString
         0     0% 98.73%    57.92kB  6.68%  google::protobuf::DescriptorBuilder::AllocateOptions
         0     0% 98.73%    57.92kB  6.68%  google::protobuf::DescriptorBuilder::AllocateOptionsImpl
         0     0% 98.73%    38.29kB  4.42%  google::protobuf::DescriptorBuilder::BuildEnum
         0     0% 98.73%    30.29kB  3.49%  google::protobuf::DescriptorBuilder::BuildEnumValue
         0     0% 98.73%   247.65kB 28.56%  google::protobuf::DescriptorBuilder::BuildField
         0     0% 98.73%   247.65kB 28.56%  google::protobuf::DescriptorBuilder::BuildFieldOrExtension
         0     0% 98.73%   558.81kB 64.45%  google::protobuf::DescriptorBuilder::BuildFile
         0     0% 98.73%   558.78kB 64.45%  google::protobuf::DescriptorBuilder::BuildFileImpl
         0     0% 98.73%   439.13kB 50.65%  google::protobuf::DescriptorBuilder::BuildMessage
         0     0% 98.73%     6.11kB   0.7%  google::protobuf::DescriptorBuilder::BuildMethod
         0     0% 98.73%        6kB  0.69%  google::protobuf::DescriptorBuilder::BuildOneof
         0     0% 98.73%    11.01kB  1.27%  google::protobuf::DescriptorBuilder::BuildService
         0     0% 98.73%    52.70kB  6.08%  google::protobuf::DescriptorBuilder::CrossLinkField
         0     0% 98.73%    55.80kB  6.44%  google::protobuf::DescriptorBuilder::CrossLinkFile
         0     0% 98.73%    53.36kB  6.15%  google::protobuf::DescriptorBuilder::CrossLinkMessage
         0     0% 98.73%   558.81kB 64.45%  google::protobuf::DescriptorPool::BuildFileFromDatabase
         0     0% 98.73%    60.84kB  7.02%  google::protobuf::DescriptorPool::CrossLinkOnDemandHelper
         0     0% 98.73%   111.14kB 12.82%  google::protobuf::DescriptorPool::FindFileByName
         0     0% 98.73%   333.81kB 38.50%  google::protobuf::DescriptorPool::FindMessageTypeByName
         0     0% 98.73%    53.03kB  6.12%  google::protobuf::DescriptorPool::FindMethodByName
         0     0% 98.73%    55.94kB  6.45%  google::protobuf::DescriptorPool::Tables::AddSymbol
         0     0% 98.73%   139.05kB 16.04%  google::protobuf::DescriptorPool::Tables::AllocateArray
         0     0% 98.73%   447.68kB 51.63%  google::protobuf::DescriptorPool::Tables::FindByNameHelper
         0     0% 98.73%   111.14kB 12.82%  google::protobuf::DescriptorPool::TryFindFileInFallbackDatabase
         0     0% 98.73%   447.68kB 51.63%  google::protobuf::DescriptorPool::TryFindSymbolInFallbackDatabase
         0     0% 98.73%    60.84kB  7.02%  google::protobuf::FieldDescriptor::InternalTypeOnceInit
         0     0% 98.73%    60.84kB  7.02%  google::protobuf::FieldDescriptor::TypeOnceInit
         0     0% 98.73%    25.02kB  2.89%  google::protobuf::FieldOptions::_InternalParse
         0     0% 98.73%    64.68kB  7.46%  google::protobuf::FileDescriptorTables::AddAliasUnderParent
         0     0% 98.73%     4.80kB  0.55%  google::protobuf::FileDescriptorTables::AddEnumValueByNumber
         0     0% 98.73%    34.18kB  3.94%  google::protobuf::FileDescriptorTables::AddFieldByNumber
         0     0% 98.73%   156.80kB 18.08%  google::protobuf::InsertIfNotPresent
         0     0% 98.73%    22.77kB  2.63%  google::protobuf::MessageLite::CreateMaybeMessage
         0     0% 98.73%    33.98kB  3.92%  google::protobuf::MessageLite::ParseFrom
         0     0% 98.73%    33.98kB  3.92%  google::protobuf::MessageLite::ParseFromString
         0     0% 98.73%     7.97kB  0.92%  google::protobuf::internal::ExtensionSet::Insert
         0     0% 98.73%     7.97kB  0.92%  google::protobuf::internal::ExtensionSet::MaybeNewExtension
         0     0% 98.73%    14.66kB  1.69%  google::protobuf::internal::ExtensionSet::MutableMessage
         0     0% 98.73%    32.70kB  3.77%  google::protobuf::internal::ExtensionSet::ParseField
         0     0% 98.73%    32.70kB  3.77%  google::protobuf::internal::ExtensionSet::ParseFieldWithExtensionInfo
         0     0% 98.73%    33.98kB  3.92%  google::protobuf::internal::MergePartialFromImpl
         0     0% 98.73%    16.73kB  1.93%  google::protobuf::internal::ParseContext::ParseMessage
         0     0% 98.73%   443.20kB 51.12%  main
         0     0% 98.73%    35.98kB  4.15%  std::_Hashtable::_M_allocate_buckets
         0     0% 98.73%   156.80kB 18.08%  std::_Hashtable::_M_insert
         0     0% 98.73%    33.66kB  3.88%  std::_Hashtable::_M_insert_unique_node
         0     0% 98.73%    33.66kB  3.88%  std::_Hashtable::_M_rehash
         0     0% 98.73%    33.66kB  3.88%  std::_Hashtable::_M_rehash_aux
         0     0% 98.73%    50.85kB  5.86%  std::_Vector_base::_M_allocate
         0     0% 98.73%     9.91kB  1.14%  std::__allocate_guarded
         0     0% 98.73%   123.15kB 14.20%  std::__detail::_AllocNode::operator()
         0     0% 98.73%    35.98kB  4.15%  std::__detail::_Hashtable_alloc::_M_allocate_buckets
         0     0% 98.73%   123.15kB 14.20%  std::__detail::_Hashtable_alloc::_M_allocate_node
         0     0% 98.73%   156.80kB 18.08%  std::__detail::_Insert_base::insert
         0     0% 98.73%   176.54kB 20.36%  std::__invoke
         0     0% 98.73%   176.54kB 20.36%  std::__invoke_impl
         0     0% 98.73%    10.63kB  1.23%  std::__shared_ptr::__shared_ptr
         0     0% 98.73%    10.61kB  1.22%  std::allocate_shared
         0     0% 98.73%     5.45kB  0.63%  std::allocator_traits::construct
         0     0% 98.73%   115.70kB 13.34%  std::call_once(std::once_flag&, void (&)(google::protobuf::internal::DescriptorTable const*), google::protobuf::internal::DescriptorTable const*&)::{lambda()#1}::operator()
         0     0% 98.73%   115.70kB 13.34%  std::call_once(std::once_flag&, void (&)(google::protobuf::internal::DescriptorTable const*), google::protobuf::internal::DescriptorTable const*&)::{lambda()#2}::__invoke
         0     0% 98.73%   115.70kB 13.34%  std::call_once(std::once_flag&, void (&)(google::protobuf::internal::DescriptorTable const*), google::protobuf::internal::DescriptorTable const*&)::{lambda()#2}::operator()
         0     0% 98.73%    60.84kB  7.02%  std::call_once(std::once_flag&, void (*&&)(google::protobuf::FieldDescriptor const*), google::protobuf::FieldDescriptor const*&&)::{lambda()#1}::operator()
         0     0% 98.73%    60.76kB  7.01%  std::call_once(std::once_flag&, void (*&&)(google::protobuf::FieldDescriptor const*), google::protobuf::FieldDescriptor const*&&)::{lambda()#2}::__invoke
         0     0% 98.73%    60.76kB  7.01%  std::call_once(std::once_flag&, void (*&&)(google::protobuf::FieldDescriptor const*), google::protobuf::FieldDescriptor const*&&)::{lambda()#2}::operator()
         0     0% 98.73%    10.61kB  1.22%  std::make_shared
         0     0% 98.73%    10.63kB  1.23%  std::shared_ptr::shared_ptr
         0     0% 98.73%   156.80kB 18.08%  std::unordered_map::insert
         0     0% 98.73%    50.01kB  5.77%  std::vector::_M_realloc_insert
         0     0% 98.73%    53.66kB  6.19%  std::vector::emplace_back
         0     0% 98.73%     5.19kB   0.6%  std::vector::push_back
         0     0% 98.73%    34.75kB  4.01%  testing::InitGoogleTest
         0     0% 98.73%    34.75kB  4.01%  testing::internal::InitGoogleTestImpl
         0     0% 98.73%     4.74kB  0.55%  testing::internal::ParameterizedTestFactory::ParameterizedTestFactory
         0     0% 98.73%    34.50kB  3.98%  testing::internal::ParameterizedTestSuiteInfo::RegisterTests
         0     0% 98.73%    34.50kB  3.98%  testing::internal::ParameterizedTestSuiteRegistry::RegisterTests
         0     0% 98.73%    34.75kB  4.01%  testing::internal::UnitTestImpl::PostFlagParsingInit
         0     0% 98.73%    34.50kB  3.98%  testing::internal::UnitTestImpl::RegisterParameterizedTests
         0     0% 98.73%     5.34kB  0.62%  validate::FieldRules::New
         0     0% 98.73%    14.77kB  1.70%  validate::FieldRules::_InternalParse
         0     0% 98.73%     9.34kB  1.08%  validate::FieldRules::_internal_mutable_string

@stale
Copy link

stale bot commented May 6, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 6, 2020
@nareddyt
Copy link
Contributor

nareddyt commented May 8, 2020

Friendly ping @asraa? I see fmtlib/fmt#1650 is merged, will that unblock this PR?

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 8, 2020
@asraa
Copy link
Contributor Author

asraa commented May 8, 2020

I see fmtlib/fmt#1650 is merged, will that unblock this PR?

I was having trouble profiling the dependency when I was running this test, because I'm wary that this might be something to avoid triggering generally. I will add an fmt.patch and revert the absl replacement

Signed-off-by: Asra Ali <[email protected]>
@asraa
Copy link
Contributor Author

asraa commented May 11, 2020

Updated the PR description to reflect patch added. let me know what you think, thank you!

@htuch
Copy link
Member

htuch commented May 11, 2020

@asraa since the patch is merged upstream, why don't we just bump our fmt dependency to pick it up?

@asraa
Copy link
Contributor Author

asraa commented May 12, 2020

Done -- can't bump fmt to latest commit or sometime later, because otherwise spdlog doesn't build (is there some name clash? sdlog should pull in it's own fmt v6.1.2)

urls = ["https://github.com/fmtlib/fmt/archive/6.0.0.tar.gz"],
sha256 = "5014aacf55285bf79654539791de0d6925063fddf4dfdd597ef76b53eb994f86",
strip_prefix = "fmt-e2ff910675c7800e5c4e28e1509ca6a50bdceafa",
urls = ["https://github.com/fmtlib/fmt/archive/e2ff910675c7800e5c4e28e1509ca6a50bdceafa.tar.gz"],
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, can you put the date above this line (as with other master pins in this file)?

Copy link
Contributor Author

@asraa asraa May 14, 2020

Choose a reason for hiding this comment

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

all set (i had some trouble with the quiche/boringssl pins, but can try to find their dates better)

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit 63e6829 into envoyproxy:master May 15, 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.

None yet

3 participants