Replace readlink calls with a safer version that guarantees NULL-termination.
This patch is part of a bigger patch that helps merging the breakpad code
with the modified version in Chromium OS.
Specifically, this patch makes the following changes:
1. Add a SafeReadLink function that wraps sys_readlink() to resolve a
symbolic link but guarantees the result is NULL-terminated on success.
2. Refactor other source code to use SafeReadLink instead of readlink()
or sys_readlink().
BUG=455
TEST=Tested the following:
1. Build on 32-bit and 64-bit Linux with gcc 4.4.3 and gcc 4.6.
2. Build on Mac OS X 10.6.8 with gcc 4.2 and clang 3.0 (with latest gmock).
3. All unit tests pass.
4. Run minidump-2-core to covnert a minidump file to a core file.
Review URL: http://breakpad.appspot.com/334001
git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@896 4c0a9323-5329-0410-9bdc-e9ce6186880e
diff --git a/src/client/linux/crash_generation/crash_generation_server.cc b/src/client/linux/crash_generation/crash_generation_server.cc
index 2f7edb6..76c9d9c 100644
--- a/src/client/linux/crash_generation/crash_generation_server.cc
+++ b/src/client/linux/crash_generation/crash_generation_server.cc
@@ -47,6 +47,7 @@
#include "client/linux/minidump_writer/minidump_writer.h"
#include "common/linux/eintr_wrapper.h"
#include "common/linux/guid_creator.h"
+#include "common/linux/safe_readlink.h"
static const char kCommandQuit = 'x';
@@ -79,12 +80,10 @@
assert(inode_out);
assert(path);
- char buf[256];
- const ssize_t n = readlink(path, buf, sizeof(buf) - 1);
- if (n == -1) {
+ char buf[PATH_MAX];
+ if (!SafeReadLink(path, buf)) {
return false;
}
- buf[n] = 0;
if (0 != memcmp(kSocketLinkPrefix, buf, sizeof(kSocketLinkPrefix) - 1)) {
return false;
@@ -130,7 +129,7 @@
for (std::vector<pid_t>::const_iterator
i = pids.begin(); i != pids.end(); ++i) {
const pid_t current_pid = *i;
- char buf[256];
+ char buf[PATH_MAX];
snprintf(buf, sizeof(buf), "/proc/%d/fd", current_pid);
DIR* fd = opendir(buf);
if (!fd)
diff --git a/src/client/linux/minidump_writer/linux_dumper.cc b/src/client/linux/minidump_writer/linux_dumper.cc
index f4ab5b1..a119aba 100644
--- a/src/client/linux/minidump_writer/linux_dumper.cc
+++ b/src/client/linux/minidump_writer/linux_dumper.cc
@@ -57,6 +57,7 @@
#include "common/linux/file_id.h"
#include "common/linux/linux_libc_support.h"
#include "common/linux/memory_mapped_file.h"
+#include "common/linux/safe_readlink.h"
#include "third_party/lss/linux_syscall_support.h"
static const char kMappedFileUnsafePrefix[] = "/dev/";
@@ -536,10 +537,8 @@
char exe_link[NAME_MAX];
char new_path[NAME_MAX];
BuildProcPath(exe_link, pid_, "exe");
- ssize_t new_path_len = sys_readlink(exe_link, new_path, NAME_MAX);
- if (new_path_len <= 0 || new_path_len == NAME_MAX)
+ if (!SafeReadLink(exe_link, new_path))
return false;
- new_path[new_path_len] = '\0';
if (my_strcmp(path, new_path) != 0)
return false;
diff --git a/src/client/linux/minidump_writer/linux_dumper_unittest.cc b/src/client/linux/minidump_writer/linux_dumper_unittest.cc
index 1eee9a1..6f4e0e4 100644
--- a/src/client/linux/minidump_writer/linux_dumper_unittest.cc
+++ b/src/client/linux/minidump_writer/linux_dumper_unittest.cc
@@ -43,6 +43,7 @@
#include "client/linux/minidump_writer/linux_dumper.h"
#include "common/linux/eintr_wrapper.h"
#include "common/linux/file_id.h"
+#include "common/linux/safe_readlink.h"
#include "common/memory.h"
using std::string;
@@ -54,7 +55,7 @@
string GetHelperBinary() {
// Locate helper binary next to the current binary.
char self_path[PATH_MAX];
- if (readlink("/proc/self/exe", self_path, sizeof(self_path) - 1) == -1) {
+ if (!SafeReadLink("/proc/self/exe", self_path)) {
return "";
}
string helper_path(self_path);
@@ -379,9 +380,7 @@
// FileID::ElfFileIdentifier and LinuxDumper::ElfFileIdentifierForMapping
// and ensure that we get the same result from both.
char exe_name[PATH_MAX];
- ssize_t len = readlink("/proc/self/exe", exe_name, PATH_MAX - 1);
- ASSERT_NE(len, -1);
- exe_name[len] = '\0';
+ ASSERT_TRUE(SafeReadLink("/proc/self/exe", exe_name));
int fds[2];
ASSERT_NE(-1, pipe(fds));
diff --git a/src/client/linux/minidump_writer/minidump_writer_unittest.cc b/src/client/linux/minidump_writer/minidump_writer_unittest.cc
index 4146526..ee021ae 100644
--- a/src/client/linux/minidump_writer/minidump_writer_unittest.cc
+++ b/src/client/linux/minidump_writer/minidump_writer_unittest.cc
@@ -42,6 +42,7 @@
#include "client/linux/minidump_writer/minidump_writer.h"
#include "common/linux/eintr_wrapper.h"
#include "common/linux/file_id.h"
+#include "common/linux/safe_readlink.h"
#include "common/tests/auto_tempdir.h"
#include "google_breakpad/processor/minidump.h"
@@ -287,8 +288,8 @@
// Locate helper binary next to the current binary.
char self_path[PATH_MAX];
- if (readlink("/proc/self/exe", self_path, sizeof(self_path) - 1) == -1) {
- FAIL() << "readlink failed: " << strerror(errno);
+ if (!SafeReadLink("/proc/self/exe", self_path)) {
+ FAIL() << "readlink failed";
exit(1);
}
string helper_path(self_path);
diff --git a/src/common/linux/file_id_unittest.cc b/src/common/linux/file_id_unittest.cc
index 977f97d..94bc80e 100644
--- a/src/common/linux/file_id_unittest.cc
+++ b/src/common/linux/file_id_unittest.cc
@@ -33,12 +33,14 @@
#include <stdlib.h>
#include "common/linux/file_id.h"
+#include "common/linux/safe_readlink.h"
#include "common/linux/synth_elf.h"
#include "common/test_assembler.h"
#include "common/tests/auto_tempdir.h"
#include "breakpad_googletest_includes.h"
using namespace google_breakpad;
+using google_breakpad::SafeReadLink;
using google_breakpad::synth_elf::BuildIDNote;
using google_breakpad::synth_elf::ELF;
using google_breakpad::test_assembler::kLittleEndian;
@@ -61,9 +63,7 @@
// FileID::ElfFileIdentifier, then make a copy of this binary,
// strip it, and ensure that the result is the same.
char exe_name[PATH_MAX];
- ssize_t len = readlink("/proc/self/exe", exe_name, PATH_MAX - 1);
- ASSERT_NE(len, -1);
- exe_name[len] = '\0';
+ ASSERT_TRUE(SafeReadLink("/proc/self/exe", exe_name));
// copy our binary to a temp file, and strip it
AutoTempDir temp_dir;
diff --git a/src/common/linux/safe_readlink.cc b/src/common/linux/safe_readlink.cc
new file mode 100644
index 0000000..870c28a
--- /dev/null
+++ b/src/common/linux/safe_readlink.cc
@@ -0,0 +1,53 @@
+// Copyright (c) 2011, Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+// * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+// * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+// * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// safe_readlink.cc: Implement google_breakpad::SafeReadLink.
+// See safe_readlink.h for details.
+
+#include <stddef.h>
+
+#include "third_party/lss/linux_syscall_support.h"
+
+namespace google_breakpad {
+
+bool SafeReadLink(const char* path, char* buffer, size_t buffer_size) {
+ // sys_readlink() does not add a NULL byte to |buffer|. In order to return
+ // a NULL-terminated string in |buffer|, |buffer_size| should be at least
+ // one byte longer than the expected path length. Also, sys_readlink()
+ // returns the actual path length on success, which does not count the
+ // NULL byte, so |result_size| should be less than |buffer_size|.
+ ssize_t result_size = sys_readlink(path, buffer, buffer_size);
+ if (result_size >= 0 && static_cast<size_t>(result_size) < buffer_size) {
+ buffer[result_size] = '\0';
+ return true;
+ }
+ return false;
+}
+
+} // namespace google_breakpad
diff --git a/src/common/linux/safe_readlink.h b/src/common/linux/safe_readlink.h
new file mode 100644
index 0000000..4ae131b
--- /dev/null
+++ b/src/common/linux/safe_readlink.h
@@ -0,0 +1,65 @@
+// Copyright (c) 2011, Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+// * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+// * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+// * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// safe_readlink.h: Define the google_breakpad::SafeReadLink function,
+// which wraps sys_readlink and gurantees the result is NULL-terminated.
+
+#ifndef COMMON_LINUX_SAFE_READLINK_H_
+#define COMMON_LINUX_SAFE_READLINK_H_
+
+#include <stddef.h>
+
+namespace google_breakpad {
+
+// This function wraps sys_readlink() and performs the same functionalty,
+// but guarantees |buffer| is NULL-terminated if sys_readlink() returns
+// no error. It takes the same arguments as sys_readlink(), but unlike
+// sys_readlink(), it returns true on success.
+//
+// |buffer_size| specifies the size of |buffer| in bytes. As this function
+// always NULL-terminates |buffer| on success, |buffer_size| should be
+// at least one byte longer than the expected path length (e.g. PATH_MAX,
+// which is typically defined as the maximum length of a path name
+// including the NULL byte).
+//
+// The implementation of this function calls sys_readlink() instead of
+// readlink(), it can thus be used in the context where calling to libc
+// functions is discouraged.
+bool SafeReadLink(const char* path, char* buffer, size_t buffer_size);
+
+// Same as the three-argument version of SafeReadLink() but deduces the
+// size of |buffer| if it is a char array of known size.
+template <size_t N>
+bool SafeReadLink(const char* path, char (&buffer)[N]) {
+ return SafeReadLink(path, buffer, sizeof(buffer));
+}
+
+} // namespace google_breakpad
+
+#endif // COMMON_LINUX_SAFE_READLINK_H_
diff --git a/src/common/linux/safe_readlink_unittest.cc b/src/common/linux/safe_readlink_unittest.cc
new file mode 100644
index 0000000..191fb9f
--- /dev/null
+++ b/src/common/linux/safe_readlink_unittest.cc
@@ -0,0 +1,89 @@
+// Copyright (c) 2011, Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+// * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+// * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+// * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// safe_readlink_unittest.cc: Unit tests for google_breakpad::SafeReadLink.
+
+#include "breakpad_googletest_includes.h"
+#include "common/linux/safe_readlink.h"
+
+using google_breakpad::SafeReadLink;
+
+TEST(SafeReadLinkTest, ZeroBufferSize) {
+ char buffer[1];
+ EXPECT_FALSE(SafeReadLink("/proc/self/exe", buffer, 0));
+}
+
+TEST(SafeReadLinkTest, BufferSizeTooSmall) {
+ char buffer[1];
+ EXPECT_FALSE(SafeReadLink("/proc/self/exe", buffer, 1));
+}
+
+TEST(SafeReadLinkTest, BoundaryBufferSize) {
+ char buffer[PATH_MAX];
+ EXPECT_TRUE(SafeReadLink("/proc/self/exe", buffer, sizeof(buffer)));
+ size_t path_length = strlen(buffer);
+ EXPECT_LT(0, path_length);
+ EXPECT_GT(sizeof(buffer), path_length);
+
+ // Buffer size equals to the expected path length plus 1 for the NULL byte.
+ char buffer2[PATH_MAX];
+ EXPECT_TRUE(SafeReadLink("/proc/self/exe", buffer2, path_length + 1));
+ EXPECT_EQ(path_length, strlen(buffer2));
+ EXPECT_EQ(0, strncmp(buffer, buffer2, PATH_MAX));
+
+ // Buffer size equals to the expected path length.
+ EXPECT_FALSE(SafeReadLink("/proc/self/exe", buffer, path_length));
+}
+
+TEST(SafeReadLinkTest, NonexistentPath) {
+ char buffer[PATH_MAX];
+ EXPECT_FALSE(SafeReadLink("nonexistent_path", buffer, sizeof(buffer)));
+}
+
+TEST(SafeReadLinkTest, NonSymbolicLinkPath) {
+ char actual_path[PATH_MAX];
+ EXPECT_TRUE(SafeReadLink("/proc/self/exe", actual_path, sizeof(actual_path)));
+
+ char buffer[PATH_MAX];
+ EXPECT_FALSE(SafeReadLink(actual_path, buffer, sizeof(buffer)));
+}
+
+TEST(SafeReadLinkTest, DeduceBufferSizeFromCharArray) {
+ char buffer[PATH_MAX];
+ char* buffer_pointer = buffer;
+ EXPECT_TRUE(SafeReadLink("/proc/self/exe", buffer_pointer, sizeof(buffer)));
+ size_t path_length = strlen(buffer);
+
+ // Use the template version of SafeReadLink to deduce the buffer size
+ // from the char array.
+ char buffer2[PATH_MAX];
+ EXPECT_TRUE(SafeReadLink("/proc/self/exe", buffer2));
+ EXPECT_EQ(path_length, strlen(buffer2));
+ EXPECT_EQ(0, strncmp(buffer, buffer2, PATH_MAX));
+}