Repository navigation
Add FuzzTest-based fuzzing for schema serde and fix the codec bugs it… #3973
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| # SPDX-License-Identifier: Apache-2.0. | ||
| # | ||
|
|
||
| set(SCHEMA_FUZZTEST_GIT_TAG "305251260059201f842e5de6bb10e7b73efb1a6e" CACHE STRING "google/fuzztest commit used by ENABLE_SCHEMA_FUZZ_TESTS") | ||
| option(SCHEMA_FUZZ_UNIT_SANITIZERS "Build the non-fuzzing (unit mode) schema fuzz tests with ASan and UBSan" ON) | ||
|
|
||
| # Must run before the CRT and core are defined so fuzzing mode instruments them. | ||
| macro(setup_schema_fuzz_tests) | ||
| if (NOT CMAKE_SYSTEM_NAME STREQUAL "Linux") | ||
| message(FATAL_ERROR "ENABLE_SCHEMA_FUZZ_TESTS requires Linux (use tools/scripts/run_fuzz.sh)") | ||
| endif () | ||
| if (NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang") | ||
| message(FATAL_ERROR "ENABLE_SCHEMA_FUZZ_TESTS requires Clang; got ${CMAKE_CXX_COMPILER_ID}") | ||
| endif () | ||
| if (BUILD_SHARED_LIBS) | ||
| message(FATAL_ERROR "ENABLE_SCHEMA_FUZZ_TESTS requires -DBUILD_SHARED_LIBS=OFF") | ||
| endif () | ||
| if (ENABLE_TESTING) | ||
| message(FATAL_ERROR "ENABLE_SCHEMA_FUZZ_TESTS cannot be combined with ENABLE_TESTING: the vendored gtest and FuzzTest's gtest collide. Use -DENABLE_TESTING=OFF") | ||
| endif () | ||
| if (CMAKE_VERSION VERSION_LESS 3.25) | ||
| message(FATAL_ERROR "ENABLE_SCHEMA_FUZZ_TESTS requires CMake >= 3.25 (FuzzTest, FetchContent SYSTEM)") | ||
| endif () | ||
|
|
||
| include(FetchContent) | ||
| FetchContent_Declare(fuzztest | ||
| GIT_REPOSITORY https://github.com/google/fuzztest.git | ||
| GIT_TAG ${SCHEMA_FUZZTEST_GIT_TAG} | ||
| # Keeps FuzzTest and Abseil headers out of the SDK warning set. | ||
| SYSTEM) | ||
| FetchContent_MakeAvailable(fuzztest) | ||
|
|
||
| fuzztest_setup_fuzzing_flags() | ||
| if (FUZZTEST_FUZZING_MODE) | ||
| # -fno-jump-tables: on AArch64, switch lookup tables of __sancov_cntrs pointers built after SanitizerCoverage | ||
| # land outside COMDAT groups and fail to link with "defined in discarded section". | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-omit-frame-pointer -fno-jump-tables") | ||
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fno-omit-frame-pointer -fno-jump-tables") | ||
| endif () | ||
| endmacro() | ||
|
|
||
| function(add_schema_fuzz_tests) | ||
| add_subdirectory(${CMAKE_SOURCE_DIR}/tests/fuzz ${CMAKE_BINARY_DIR}/tests/fuzz) | ||
| endfunction() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,9 @@ | |
| namespace smithy { | ||
| namespace schema { | ||
|
|
||
| // Container nesting limit, root included, shared by every serializer and deserializer. | ||
| constexpr int MAX_NESTING_DEPTH = 1000; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why in this class and why exported? seems like this should be a implemetnation detail in a cpp file |
||
|
|
||
| class SchemaBuilder; | ||
|
|
||
| enum class ShapeType : uint8_t { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,28 @@ | ||
| #pragma once | ||
|
|
||
| #include <aws/core/client/AWSError.h> | ||
| #include <aws/core/client/CoreErrors.h> | ||
| #include <aws/core/utils/Array.h> | ||
| #include <aws/core/utils/DateTime.h> | ||
| #include <aws/core/utils/StringUtils.h> | ||
| #include <aws/core/utils/memory/stl/AWSString.h> | ||
| #include <aws/crt/Optional.h> | ||
| #include <smithy/Smithy_EXPORTS.h> | ||
| #include <smithy/client/schema/Document.h> | ||
| #include <smithy/client/schema/Schema.h> | ||
| #include <smithy/client/schema/SerdeTraits.h> | ||
|
|
||
| #include <cmath> | ||
| #include <cstdint> | ||
| #include <functional> | ||
| #include <limits> | ||
| #include <memory> | ||
|
|
||
| namespace smithy { | ||
| namespace schema { | ||
|
|
||
| using DeserializerError = Aws::Client::AWSError<Aws::Client::CoreErrors>; | ||
|
|
||
| class SMITHY_API ShapeDeserializer { | ||
| public: | ||
| virtual ~ShapeDeserializer() = default; | ||
|
|
@@ -37,10 +45,117 @@ class SMITHY_API ShapeDeserializer { | |
| virtual Aws::Crt::Optional<Aws::String> ReadString(const Schema& schema) = 0; | ||
| virtual Aws::Crt::Optional<Aws::Utils::DateTime> ReadTimestamp(const Schema& schema) = 0; | ||
| virtual Aws::Crt::Optional<Aws::Utils::ByteBuffer> ReadBlob(const Schema& schema) = 0; | ||
| // Null when the member is absent, unparsable, or the protocol does not support documents. | ||
| virtual std::shared_ptr<const Document> ReadDocument(const Schema& schema) = 0; | ||
|
|
||
| virtual bool IsNull() = 0; | ||
|
|
||
| // First failure wins; once set, reads return empty. | ||
| virtual Aws::Crt::Optional<DeserializerError> GetError() const { return m_error; } | ||
|
|
||
| protected: | ||
| void Fail(const Aws::String& message) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a lot of this protected code should be hidden in the implementation using pimpl, or at least in the cpp of the translation unit
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think we're about to create a godclass here. you are making this protected so a child could use them. I think you either want a utils class (final class, nonstructible with static functions), or namespace functions to avoid this. |
||
| if (!m_error.has_value()) { | ||
| m_error = DeserializerError(Aws::Client::CoreErrors::INTERNAL_FAILURE, "SerializationException", message, false); | ||
| } | ||
| } | ||
| bool HasFailed() const { return m_error.has_value(); } | ||
| bool AtRoot() const { return m_depth == 0; } | ||
|
|
||
| void FailUnrepresentable(const char* type) { Fail(Aws::String("Value out of range for ") + type); } | ||
|
|
||
| // Call before delivering each non-null modeled member; a second member of a union fails the read. | ||
| bool AdmitMember(const Schema& schema, bool& delivered) { | ||
| if (delivered && schema.GetType() == ShapeType::Union) { | ||
| Fail("Union has more than one member set"); | ||
| return false; | ||
| } | ||
| delivered = true; | ||
| return true; | ||
| } | ||
|
|
||
| Aws::Crt::Optional<int> NarrowToInt(int64_t value) { | ||
| if (value < (std::numeric_limits<int>::min)() || value > (std::numeric_limits<int>::max)()) { | ||
| FailUnrepresentable("integer"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah anything that has like a is screaming to me to us a outcome for better reprsenatation. if we can change the API we should |
||
| return {}; | ||
| } | ||
| return static_cast<int>(value); | ||
| } | ||
|
|
||
| // Float text printed to 9 digits can parse just above FLT_MAX, so values below the rounding point to infinity clamp. | ||
| Aws::Crt::Optional<float> NarrowToFloat(double value) { | ||
| const float floatMax = (std::numeric_limits<float>::max)(); | ||
| const double roundsToInfinity = static_cast<double>(floatMax) + std::ldexp(1.0, 103); | ||
| if (std::isfinite(value) && std::fabs(value) > static_cast<double>(floatMax)) { | ||
| if (std::fabs(value) >= roundsToInfinity) { | ||
| FailUnrepresentable("float"); | ||
| return {}; | ||
| } | ||
| return value < 0 ? -floatMax : floatMax; | ||
| } | ||
| return static_cast<float>(value); | ||
| } | ||
|
|
||
| // Out-of-range integer text fails the read; other non-integer text just returns empty. | ||
| Aws::Crt::Optional<int64_t> ParseLong(const Aws::String& text) { | ||
| const auto value = Aws::Utils::StringUtils::ParseInt64(text); | ||
| if (!value.has_value() && IsPlainInteger(text)) { | ||
| FailUnrepresentable("long"); | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| // Must accept what strtoll does. | ||
| static bool IsPlainInteger(const Aws::String& text) { | ||
| size_t start = text.find_first_not_of(" \t\n\v\f\r"); | ||
| if (start == Aws::String::npos) { | ||
| return false; | ||
| } | ||
| if (text[start] == '-' || text[start] == '+') { | ||
| ++start; | ||
| } | ||
| return text.size() > start && text.find_first_not_of("0123456789", start) == Aws::String::npos; | ||
| } | ||
|
|
||
| Aws::Crt::Optional<Aws::Utils::DateTime> EpochSecondsToTimestamp(double seconds) { | ||
| auto parsed = EpochSecondsToDateTime(seconds); | ||
| if (!parsed.has_value()) { | ||
| FailUnrepresentable("timestamp"); | ||
| } | ||
| return parsed; | ||
| } | ||
|
|
||
| class ContainerScope final { | ||
| public: | ||
| explicit ContainerScope(ShapeDeserializer& owner) : m_owner(owner), m_entered(owner.EnterContainer()) {} | ||
| ~ContainerScope() { | ||
| if (m_entered) { | ||
| --m_owner.m_depth; | ||
| } | ||
| } | ||
| ContainerScope(const ContainerScope&) = delete; | ||
| ContainerScope& operator=(const ContainerScope&) = delete; | ||
| bool Entered() const { return m_entered; } | ||
|
|
||
| private: | ||
| ShapeDeserializer& m_owner; | ||
| bool m_entered; | ||
| }; | ||
|
|
||
| private: | ||
| bool EnterContainer() { | ||
| if (HasFailed()) { | ||
| return false; | ||
| } | ||
| if (m_depth >= MAX_NESTING_DEPTH) { | ||
| Fail("Maximum nesting depth exceeded"); | ||
| return false; | ||
| } | ||
| ++m_depth; | ||
| return true; | ||
| } | ||
|
|
||
| Aws::Crt::Optional<DeserializerError> m_error; | ||
| int m_depth = 0; | ||
| }; | ||
|
|
||
| } // namespace schema | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this get the last error? the idea with the API was that optional empty would be error, where the original comment was "we need to figure out how to show an error, either through a optional, or a outcome". do want chained outcomes on the api?