From 655d521134bd87a8038d731c9c6df265c88d86f3 Mon Sep 17 00:00:00 2001 From: Fergus Henderson Date: Tue, 2 Apr 2024 19:54:46 +0100 Subject: [PATCH] Avoid ODR violations with flatbuffers::Verifier. Fix "One Definition Rule" violation when using flatbuffers::Verifier with FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE defined in some compilation units and not defined in other compilation units. The fix is to make Verifier a template class, with a boolean template parameter replacing the "#ifdef" conditionals; to rename it as VerifierTemplate; and then to use "#ifdef" only for a "using" declaration that defines the original name Verifier an an alias for the instantiated template. In this way, even if FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE is defined in some compilation units and not in others, as long as clients only reference flatbuffers::Verifier in .cc files, not header files, there will be no ODR violation, since the only part whose definition varies is the "using" declaration, which does not have external linkage. There is still some possibility of clients creating ODR violations if the client header files (rather than .cc files) reference flatbuffers::Verifier. To avoid that, this change also deprecates FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE, and instead introduces flatbuffers::SizeVerifier as a public name for the template instance with the boolean parameter set to true, so that clients don't need to define the macro at all. --- include/flatbuffers/verifier.h | 108 +++++++++++++++++++++------------ 1 file changed, 70 insertions(+), 38 deletions(-) diff --git a/include/flatbuffers/verifier.h b/include/flatbuffers/verifier.h index de1146be92e..6df923be45c 100644 --- a/include/flatbuffers/verifier.h +++ b/include/flatbuffers/verifier.h @@ -23,7 +23,8 @@ namespace flatbuffers { // Helper class to verify the integrity of a FlatBuffer -class Verifier FLATBUFFERS_FINAL_CLASS { +template +class VerifierTemplate FLATBUFFERS_FINAL_CLASS { public: struct Options { // The maximum nesting of tables and vectors before we call it invalid. @@ -40,17 +41,18 @@ class Verifier FLATBUFFERS_FINAL_CLASS { bool assert = false; }; - explicit Verifier(const uint8_t *const buf, const size_t buf_len, - const Options &opts) + explicit VerifierTemplate(const uint8_t *const buf, const size_t buf_len, + const Options &opts) : buf_(buf), size_(buf_len), opts_(opts) { FLATBUFFERS_ASSERT(size_ < opts.max_size); } - // Deprecated API, please construct with Verifier::Options. - Verifier(const uint8_t *const buf, const size_t buf_len, - const uoffset_t max_depth = 64, const uoffset_t max_tables = 1000000, - const bool check_alignment = true) - : Verifier(buf, buf_len, [&] { + // Deprecated API, please construct with VerifierTemplate::Options. + VerifierTemplate(const uint8_t *const buf, const size_t buf_len, + const uoffset_t max_depth = 64, + const uoffset_t max_tables = 1000000, + const bool check_alignment = true) + : VerifierTemplate(buf, buf_len, [&] { Options opts; opts.max_depth = max_depth; opts.max_tables = max_tables; @@ -62,25 +64,25 @@ class Verifier FLATBUFFERS_FINAL_CLASS { bool Check(const bool ok) const { // clang-format off #ifdef FLATBUFFERS_DEBUG_VERIFICATION_FAILURE - if (opts_.assert) { FLATBUFFERS_ASSERT(ok); } - #endif - #ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE - if (!ok) - upper_bound_ = 0; + if (opts_.assert) { FLATBUFFERS_ASSERT(ok); } #endif // clang-format on + if (TrackVerifierBufferSize) { + if (!ok) { + upper_bound_ = 0; + } + } return ok; } // Verify any range within the buffer. bool Verify(const size_t elem, const size_t elem_len) const { - // clang-format off - #ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE + if (TrackVerifierBufferSize) { auto upper_bound = elem + elem_len; - if (upper_bound_ < upper_bound) + if (upper_bound_ < upper_bound) { upper_bound_ = upper_bound; - #endif - // clang-format on + } + } return Check(elem_len < size_ && elem <= size_ - elem_len); } @@ -210,14 +212,14 @@ class Verifier FLATBUFFERS_FINAL_CLASS { // Call T::Verify, which must be in the generated code for this type. const auto o = VerifyOffset(start); - return Check(o != 0) && - reinterpret_cast(buf_ + start + o)->Verify(*this) - // clang-format off - #ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE - && GetComputedSize() - #endif - ; - // clang-format on + if (!Check(o != 0)) return false; + if (!(reinterpret_cast(buf_ + start + o)->Verify(*this))) { + return false; + } + if (TrackVerifierBufferSize) { + if (GetComputedSize() == 0) return false; + } + return true; } template @@ -232,7 +234,8 @@ class Verifier FLATBUFFERS_FINAL_CLASS { // If there is a nested buffer, it must be greater than the min size. if (!Check(buf->size() >= FLATBUFFERS_MIN_BUFFER_SIZE)) return false; - Verifier nested_verifier(buf->data(), buf->size(), opts_); + VerifierTemplate nested_verifier( + buf->data(), buf->size(), opts_); return nested_verifier.VerifyBuffer(identifier); } @@ -286,21 +289,27 @@ class Verifier FLATBUFFERS_FINAL_CLASS { return true; } - // Returns the message size in bytes + // Returns the message size in bytes. + // + // This should only be called after first calling VerifyBuffer or + // VerifySizePrefixedBuffer. + // + // This method should only be called for VerifierTemplate instances + // where the TrackVerifierBufferSize template parameter is true, + // i.e. for SizeVerifier. For instances where TrackVerifierBufferSize + // is false, this fails at runtime or returns zero. size_t GetComputedSize() const { - // clang-format off - #ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE + if (TrackVerifierBufferSize) { uintptr_t size = upper_bound_; // Align the size to uoffset_t size = (size - 1 + sizeof(uoffset_t)) & ~(sizeof(uoffset_t) - 1); return (size > size_) ? 0 : size; - #else - // Must turn on FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE for this to work. - (void)upper_bound_; - FLATBUFFERS_ASSERT(false); - return 0; - #endif - // clang-format on + } + // Must use SizeVerifier, or (deprecated) turn on + // FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE, for this to work. + (void)upper_bound_; + FLATBUFFERS_ASSERT(false); + return 0; } std::vector *GetFlexReuseTracker() { return flex_reuse_tracker_; } @@ -323,10 +332,33 @@ class Verifier FLATBUFFERS_FINAL_CLASS { // Specialization for 64-bit offsets. template<> -inline size_t Verifier::VerifyOffset(const size_t start) const { +template<> +inline size_t VerifierTemplate::VerifyOffset( + const size_t start) const { + return VerifyOffset(start); +} +template<> +template<> +inline size_t VerifierTemplate::VerifyOffset( + const size_t start) const { return VerifyOffset(start); } +// Instance of VerifierTemplate that supports GetComputedSize(). +using SizeVerifier = VerifierTemplate; + +// The FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE build configuration macro is +// deprecated, and should not be defined, since it is easy to misuse in ways +// that result in ODR violations. Rather than using Verifier and defining +// FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE, please use SizeVerifier instead. +#ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE // Deprecated, see above. +using Verifier = SizeVerifier; +#else +// Instance of VerifierTemplate that is slightly faster, but does not +// support GetComputedSize(). +using Verifier = VerifierTemplate; +#endif + } // namespace flatbuffers #endif // FLATBUFFERS_VERIFIER_H_