Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit fd8dd0b6 authored by Christopher Ferris's avatar Christopher Ferris Committed by The Android Automerger
Browse files

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

(cherry picked from commit 626efb78)

Change-Id: Iba60a926cf5a1d6b517a6bfd8c797d724f093010
parent 12f74e63

libcutils/tests/Android.mk

+2 −1
Original line number Diff line number Diff line
@@ -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





#

libcutils/tests/trace-dev_test.cpp

0 → 100644
+295 −0
Original line number Diff line number Diff line 
/*

 * 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 <sys/types.h>

#include <unistd.h>



#include <memory>

#include <string>



#include <android-base/file.h>

#include <android-base/stringprintf.h>

#include <android-base/test_utils.h>

#include <gtest/gtest.h>



#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());

}

libcutils/trace-dev.c

+24 −26
Original line number Diff line number Diff line
@@ -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);

}