diff --git a/absl/log/CMakeLists.txt b/absl/log/CMakeLists.txt index e0a307b3aff..f53a9ee6995 100644 --- a/absl/log/CMakeLists.txt +++ b/absl/log/CMakeLists.txt @@ -182,6 +182,22 @@ absl_cc_library( absl::span ) +absl_cc_test( + NAME + log_internal_proto_test + SRCS + "internal/proto_test.cc" + COPTS + ${ABSL_TEST_COPTS} + LINKOPTS + ${ABSL_DEFAULT_LINKOPTS} + DEPS + absl::config + absl::log_internal_proto + absl::span + GTest::gmock_main +) + absl_cc_library( NAME log_internal_message diff --git a/absl/log/internal/BUILD.bazel b/absl/log/internal/BUILD.bazel index 83c5eab5baf..732a145c5f5 100644 --- a/absl/log/internal/BUILD.bazel +++ b/absl/log/internal/BUILD.bazel @@ -446,6 +446,18 @@ cc_library( ], ) +cc_test( + name = "proto_test", + srcs = ["proto_test.cc"], + deps = [ + ":proto", + "//absl/base:config", + "//absl/types:span", + "@googletest//:gtest", + "@googletest//:gtest_main", + ], +) + cc_library( name = "fnmatch", srcs = ["fnmatch.cc"], diff --git a/absl/log/internal/proto.cc b/absl/log/internal/proto.cc index d0c57d69900..689c888aa12 100644 --- a/absl/log/internal/proto.cc +++ b/absl/log/internal/proto.cc @@ -163,12 +163,17 @@ void EncodeMessageLength(absl::Span msg, const absl::Span *buf) { namespace { uint64_t DecodeVarint(absl::Span *buf) { + // A base-128 varint for a 64-bit value occupies at most 10 bytes. Stop after + // that many so the shift below can never reach the width of `value`, which + // would be undefined behavior on a malformed or over-long input. The 64-bit + // and 32-bit decoders below cap their loops for the same reason. + constexpr size_t kMaxVarintBytes = 10; uint64_t value = 0; size_t s = 0; while (s < buf->size()) { value |= static_cast(static_cast((*buf)[s]) & 0x7f) << 7 * s; - if (!((*buf)[s++] & 0x80)) break; + if (!((*buf)[s++] & 0x80) || s == kMaxVarintBytes) break; } buf->remove_prefix(s); return value; diff --git a/absl/log/internal/proto_test.cc b/absl/log/internal/proto_test.cc new file mode 100644 index 00000000000..59dbb48cc53 --- /dev/null +++ b/absl/log/internal/proto_test.cc @@ -0,0 +1,68 @@ +// +// Copyright 2024 The Abseil Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "absl/log/internal/proto.h" + +#include +#include +#include + +#include "gtest/gtest.h" +#include "absl/base/config.h" +#include "absl/types/span.h" + +namespace absl { +ABSL_NAMESPACE_BEGIN +namespace log_internal { +namespace { + +TEST(ProtoTest, DecodeVarintRoundTrips) { + std::vector buf(BufferSizeFor(1, WireType::kVarint)); + absl::Span encode_span(buf); + ASSERT_TRUE(EncodeVarint(1, uint64_t{300}, &encode_span)); + + absl::Span decode_span(buf.data(), + buf.size() - encode_span.size()); + ProtoField field; + ASSERT_TRUE(field.DecodeFrom(&decode_span)); + EXPECT_EQ(field.tag(), 1); + EXPECT_EQ(field.type(), WireType::kVarint); + EXPECT_EQ(field.uint64_value(), 300); + EXPECT_TRUE(decode_span.empty()); +} + +// A varint whose bytes all set the continuation bit is malformed, but the +// decoder is meant to tolerate arbitrary input. Before the length cap this +// drove the shift exponent past the width of the accumulator (`<< 70`), which +// is undefined behavior that UBSan flags. The decoder must stop after the +// 10-byte maximum of a 64-bit varint instead. +TEST(ProtoTest, DecodeOverlongVarintDoesNotOverflowShift) { + std::vector buf; + buf.push_back(static_cast(MakeTagType(1, WireType::kVarint))); + for (int i = 0; i < 11; ++i) buf.push_back(static_cast(0x80)); + buf.push_back(static_cast(0x01)); + + absl::Span decode_span(buf); + ProtoField field; + ASSERT_TRUE(field.DecodeFrom(&decode_span)); + EXPECT_EQ(field.type(), WireType::kVarint); + // At most the 10 varint bytes are consumed, leaving the trailing bytes. + EXPECT_FALSE(decode_span.empty()); +} + +} // namespace +} // namespace log_internal +ABSL_NAMESPACE_END +} // namespace absl