metrics: Use unsigned type for length field
This brings the changes from the Chrome OS CL
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1341794
over to the Chrome repository.
BUG=chromium:906622
TEST=Compiles and passes tests.
Change-Id: I36b4c6d156322ca56d8678b10ac7b46103e3c670
Reviewed-on: https://chromium-review.googlesource.com/c/1346397
Commit-Queue: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619913}diff --git a/components/metrics/serialization/serialization_utils.cc b/components/metrics/serialization/serialization_utils.cc
index 7214b5d..21931d0 100644
--- a/components/metrics/serialization/serialization_utils.cc
+++ b/components/metrics/serialization/serialization_utils.cc
@@ -14,6 +14,7 @@
#include "base/files/file_util.h"
#include "base/files/scoped_file.h"
#include "base/logging.h"
+#include "base/numerics/safe_math.h"
#include "base/strings/string_split.h"
#include "base/strings/string_util.h"
#include "components/metrics/serialization/metric_sample.h"
@@ -35,11 +36,11 @@
CHECK(message);
int result;
- int32_t message_size;
- const int32_t message_header_size = sizeof(message_size);
+ uint32_t encoded_size;
+ const size_t message_header_size = sizeof(uint32_t);
// The file containing the metrics does not leave the device so the writer and
// the reader will always have the same endianness.
- result = HANDLE_EINTR(read(fd, &message_size, message_header_size));
+ result = HANDLE_EINTR(read(fd, &encoded_size, message_header_size));
if (result < 0) {
DPLOG(ERROR) << "reading metrics message header";
return false;
@@ -48,17 +49,19 @@
// This indicates a normal EOF.
return false;
}
- if (result < message_header_size) {
+ if (base::checked_cast<size_t>(result) < message_header_size) {
DLOG(ERROR) << "bad read size " << result << ", expecting "
- << sizeof(message_size);
+ << message_header_size;
return false;
}
// kMessageMaxLength applies to the entire message: the 4-byte
// length field and the content.
+ size_t message_size = base::checked_cast<size_t>(encoded_size);
if (message_size > SerializationUtils::kMessageMaxLength) {
DLOG(ERROR) << "message too long : " << message_size;
- if (HANDLE_EINTR(lseek(fd, message_size - 4, SEEK_CUR)) == -1) {
+ if (HANDLE_EINTR(lseek(fd, message_size - message_header_size, SEEK_CUR)) ==
+ -1) {
DLOG(ERROR) << "error while skipping message. abort";
return false;
}
@@ -192,17 +195,19 @@
}
std::string msg = sample.ToString();
- int32_t size = msg.length() + sizeof(int32_t);
- if (size > kMessageMaxLength) {
+ size_t size = 0;
+ if (!base::CheckAdd(msg.length(), sizeof(uint32_t)).AssignIfValid(&size) ||
+ size > kMessageMaxLength) {
DPLOG(ERROR) << "cannot write message: too long: " << filename;
return false;
}
// The file containing the metrics samples will only be read by programs on
// the same device so we do not check endianness.
+ uint32_t encoded_size = base::checked_cast<uint32_t>(size);
if (!base::WriteFileDescriptor(file_descriptor.get(),
- reinterpret_cast<char*>(&size),
- sizeof(size))) {
+ reinterpret_cast<char*>(&encoded_size),
+ sizeof(uint32_t))) {
DPLOG(ERROR) << "error writing message length: " << filename;
return false;
}
diff --git a/components/metrics/serialization/serialization_utils.h b/components/metrics/serialization/serialization_utils.h
index c741cb2f..2052686 100644
--- a/components/metrics/serialization/serialization_utils.h
+++ b/components/metrics/serialization/serialization_utils.h
@@ -40,7 +40,7 @@
bool WriteMetricToFile(const MetricSample& sample, const std::string& filename);
// Maximum length of a serialized message
-static const int kMessageMaxLength = 1024;
+static const size_t kMessageMaxLength = 1024;
} // namespace SerializationUtils
} // namespace metrics
diff --git a/components/metrics/serialization/serialization_utils_unittest.cc b/components/metrics/serialization/serialization_utils_unittest.cc
index 5685a1f..6bf3c8c 100644
--- a/components/metrics/serialization/serialization_utils_unittest.cc
+++ b/components/metrics/serialization/serialization_utils_unittest.cc
@@ -135,6 +135,50 @@
EXPECT_TRUE(crash->IsEqual(*samples[0]));
}
+TEST_F(SerializationUtilsTest, NegativeLengthTest) {
+ // This input is specifically constructed to yield a single crash sample when
+ // parsed by a buggy version of the code but fails to parse and doesn't yield
+ // samples when parsed by a correct implementation.
+ constexpr uint8_t kInput[] = {
+ // Length indicating that next length field is the negative one below.
+ // This sample is invalid as it contains more than three null bytes.
+ 0x14,
+ 0x00,
+ 0x00,
+ 0x00,
+ // Encoding of a valid crash sample.
+ 0x0c,
+ 0x00,
+ 0x00,
+ 0x00,
+ 0x63,
+ 0x72,
+ 0x61,
+ 0x73,
+ 0x68,
+ 0x00,
+ 0x61,
+ 0x00,
+ // Invalid sample that jumps past the negative length bytes below.
+ 0x08,
+ 0x00,
+ 0x00,
+ 0x00,
+ // This is -16 in two's complement interpretation, pointing to the valid
+ // crash sample before.
+ 0xf0,
+ 0xff,
+ 0xff,
+ 0xff,
+ };
+ CHECK(base::WriteFile(filepath, reinterpret_cast<const char*>(kInput),
+ sizeof(kInput)));
+
+ std::vector<std::unique_ptr<MetricSample>> samples;
+ SerializationUtils::ReadAndTruncateMetricsFromFile(filename, &samples);
+ ASSERT_EQ(0U, samples.size());
+}
+
TEST_F(SerializationUtilsTest, WriteReadTest) {
std::unique_ptr<MetricSample> hist =
MetricSample::HistogramSample("myhist", 1, 2, 3, 4);
