From 626efb78a6e1f0b2d637368f1eba175cfe89fb1c Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Tue, 23 Feb 2016 18:02:20 -0800 Subject: [PATCH] Fix incorrect handling of snprintf return value. The code assumed that snprintf would never return a value less than the passed in size of the buffer. This is not accurate, so fix all of the places this assumptions is made. Also, if the name is too large, then truncate just the name to make everything fit. Added a new set of tests for this code. Verified that the old code passes on the _normal and _exact version of the tests, but fails with the FORTIFY error on the _truncated version of the tests. All tests pass on the new code. Bug: 27324359 Change-Id: I1b64ddde6f5ff2ec7f6428b998d21d41a1236b14 --- libcutils/tests/Android.mk | 3 +- libcutils/tests/trace-dev_test.cpp | 295 +++++++++++++++++++++++++++++ libcutils/trace-dev.c | 50 +++-- 3 files changed, 321 insertions(+), 27 deletions(-) create mode 100644 libcutils/tests/trace-dev_test.cpp diff --git a/libcutils/tests/Android.mk b/libcutils/tests/Android.mk index 4da5ed6f0..52cf5f407 100644 --- a/libcutils/tests/Android.mk +++ b/libcutils/tests/Android.mk @@ -23,8 +23,9 @@ test_src_files_nonwindows := \ test_target_only_src_files := \ MemsetTest.cpp \ PropertiesTest.cpp \ + trace-dev_test.cpp \ -test_libraries := libcutils liblog +test_libraries := libcutils liblog libbase # diff --git a/libcutils/tests/trace-dev_test.cpp b/libcutils/tests/trace-dev_test.cpp new file mode 100644 index 000000000..edf981b39 --- /dev/null +++ b/libcutils/tests/trace-dev_test.cpp @@ -0,0 +1,295 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * 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 + * + * http://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 +#include + +#include +#include + +#include +#include +#include +#include + +#include "../trace-dev.c" + +class TraceDevTest : public ::testing::Test { + protected: + void SetUp() override { + lseek(tmp_file_.fd, 0, SEEK_SET); + atrace_marker_fd = tmp_file_.fd; + } + + void TearDown() override { + atrace_marker_fd = -1; + } + + TemporaryFile tmp_file_; + + static std::string MakeName(size_t length) { + std::string name; + for (size_t i = 0; i < length; i++) { + name += '0' + (i % 10); + } + return name; + } +}; + +TEST_F(TraceDevTest, atrace_begin_body_normal) { + atrace_begin_body("fake_name"); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("B|%d|fake_name", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_begin_body_exact) { + std::string expected = android::base::StringPrintf("B|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 1); + atrace_begin_body(name.c_str()); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_begin_body(name.c_str()); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_begin_body_truncated) { + std::string expected = android::base::StringPrintf("B|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_begin_body(name.c_str()); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 1; + expected += android::base::StringPrintf("%.*s", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_begin_body_normal) { + atrace_async_begin_body("fake_name", 12345); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("S|%d|fake_name|12345", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_begin_body_exact) { + std::string expected = android::base::StringPrintf("S|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 7); + atrace_async_begin_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name + "|12345"; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_async_begin_body(name.c_str(), 12345); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_begin_body_truncated) { + std::string expected = android::base::StringPrintf("S|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_async_begin_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 7; + expected += android::base::StringPrintf("%.*s|12345", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_end_body_normal) { + atrace_async_end_body("fake_name", 12345); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("F|%d|fake_name|12345", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_end_body_exact) { + std::string expected = android::base::StringPrintf("F|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 7); + atrace_async_end_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name + "|12345"; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_async_end_body(name.c_str(), 12345); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_async_end_body_truncated) { + std::string expected = android::base::StringPrintf("F|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_async_end_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 7; + expected += android::base::StringPrintf("%.*s|12345", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int_body_normal) { + atrace_int_body("fake_name", 12345); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("C|%d|fake_name|12345", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int_body_exact) { + std::string expected = android::base::StringPrintf("C|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 7); + atrace_int_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name + "|12345"; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_int_body(name.c_str(), 12345); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int_body_truncated) { + std::string expected = android::base::StringPrintf("C|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_int_body(name.c_str(), 12345); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 7; + expected += android::base::StringPrintf("%.*s|12345", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int64_body_normal) { + atrace_int64_body("fake_name", 17179869183L); + + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + std::string expected = android::base::StringPrintf("C|%d|fake_name|17179869183", getpid()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int64_body_exact) { + std::string expected = android::base::StringPrintf("C|%d|", getpid()); + std::string name = MakeName(ATRACE_MESSAGE_LENGTH - expected.length() - 13); + atrace_int64_body(name.c_str(), 17179869183L); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + expected += name + "|17179869183"; + ASSERT_STREQ(expected.c_str(), actual.c_str()); + + // Add a single character and verify we get the exact same value as before. + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + name += '*'; + atrace_int64_body(name.c_str(), 17179869183L); + EXPECT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} + +TEST_F(TraceDevTest, atrace_int64_body_truncated) { + std::string expected = android::base::StringPrintf("C|%d|", getpid()); + std::string name = MakeName(2 * ATRACE_MESSAGE_LENGTH); + atrace_int64_body(name.c_str(), 17179869183L); + + ASSERT_EQ(ATRACE_MESSAGE_LENGTH - 1, lseek(atrace_marker_fd, 0, SEEK_CUR)); + ASSERT_EQ(0, lseek(atrace_marker_fd, 0, SEEK_SET)); + + std::string actual; + ASSERT_TRUE(android::base::ReadFdToString(atrace_marker_fd, &actual)); + int expected_len = ATRACE_MESSAGE_LENGTH - expected.length() - 13; + expected += android::base::StringPrintf("%.*s|17179869183", expected_len, name.c_str()); + ASSERT_STREQ(expected.c_str(), actual.c_str()); +} diff --git a/libcutils/trace-dev.c b/libcutils/trace-dev.c index f025256f1..5df1c5a90 100644 --- a/libcutils/trace-dev.c +++ b/libcutils/trace-dev.c @@ -194,49 +194,47 @@ void atrace_setup() void atrace_begin_body(const char* name) { char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "B|%d|%s", getpid(), name); + int len = snprintf(buf, sizeof(buf), "B|%d|%s", getpid(), name); + if (len >= (int) sizeof(buf)) { + ALOGW("Truncated name in %s: %s\n", __FUNCTION__, name); + len = sizeof(buf) - 1; + } write(atrace_marker_fd, buf, len); } +#define WRITE_MSG(format_begin, format_end, pid, name, value) { \ + char buf[ATRACE_MESSAGE_LENGTH]; \ + int len = snprintf(buf, sizeof(buf), format_begin "%s" format_end, pid, \ + name, value); \ + if (len >= (int) sizeof(buf)) { \ + /* Given the sizeof(buf), and all of the current format buffers, \ + * it is impossible for name_len to be < 0 if len >= sizeof(buf). */ \ + int name_len = strlen(name) - (len - sizeof(buf)) - 1; \ + /* Truncate the name to make the message fit. */ \ + ALOGW("Truncated name in %s: %s\n", __FUNCTION__, name); \ + len = snprintf(buf, sizeof(buf), format_begin "%.*s" format_end, pid, \ + name_len, name, value); \ + } \ + write(atrace_marker_fd, buf, len); \ +} void atrace_async_begin_body(const char* name, int32_t cookie) { - char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "S|%d|%s|%" PRId32, - getpid(), name, cookie); - write(atrace_marker_fd, buf, len); + WRITE_MSG("S|%d|", "|%" PRId32, getpid(), name, cookie); } void atrace_async_end_body(const char* name, int32_t cookie) { - char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "F|%d|%s|%" PRId32, - getpid(), name, cookie); - write(atrace_marker_fd, buf, len); + WRITE_MSG("F|%d|", "|%" PRId32, getpid(), name, cookie); } void atrace_int_body(const char* name, int32_t value) { - char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "C|%d|%s|%" PRId32, - getpid(), name, value); - write(atrace_marker_fd, buf, len); + WRITE_MSG("C|%d|", "|%" PRId32, getpid(), name, value); } void atrace_int64_body(const char* name, int64_t value) { - char buf[ATRACE_MESSAGE_LENGTH]; - size_t len; - - len = snprintf(buf, ATRACE_MESSAGE_LENGTH, "C|%d|%s|%" PRId64, - getpid(), name, value); - write(atrace_marker_fd, buf, len); + WRITE_MSG("C|%d|", "|%" PRId64, getpid(), name, value); }