From ae5193c24ee046c5b8197ce76838a2b2c0e05e01 Mon Sep 17 00:00:00 2001 From: "benchan@chromium.org" Date: Wed, 21 Dec 2011 17:51:40 +0000 Subject: 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 --- Makefile.am | 7 +- Makefile.in | 62 ++++++++++++++- .../crash_generation/crash_generation_server.cc | 9 +-- src/client/linux/minidump_writer/linux_dumper.cc | 5 +- .../linux/minidump_writer/linux_dumper_unittest.cc | 7 +- .../minidump_writer/minidump_writer_unittest.cc | 5 +- src/common/linux/file_id_unittest.cc | 6 +- src/common/linux/safe_readlink.cc | 53 +++++++++++++ src/common/linux/safe_readlink.h | 65 ++++++++++++++++ src/common/linux/safe_readlink_unittest.cc | 89 ++++++++++++++++++++++ 10 files changed, 287 insertions(+), 21 deletions(-) create mode 100644 src/common/linux/safe_readlink.cc create mode 100644 src/common/linux/safe_readlink.h create mode 100644 src/common/linux/safe_readlink_unittest.cc diff --git a/Makefile.am b/Makefile.am index 61921602..47e7676f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -73,7 +73,8 @@ src_client_linux_libbreakpad_client_a_SOURCES = \ src/common/string_conversion.cc \ src/common/linux/file_id.cc \ src/common/linux/guid_creator.cc \ - src/common/linux/memory_mapped_file.cc + src/common/linux/memory_mapped_file.cc \ + src/common/linux/safe_readlink.cc endif LINUX_HOST if !DISABLE_PROCESSOR @@ -314,6 +315,7 @@ src_client_linux_linux_client_unittest_LDADD = \ src/common/linux/file_id.o \ src/common/linux/guid_creator.o \ src/common/linux/memory_mapped_file.o \ + src/common/linux/safe_readlink.o \ src/common/string_conversion.o src_client_linux_linux_client_unittest_DEPENDENCIES = src/client/linux/linux_dumper_unittest_helper src/client/linux/libbreakpad_client.a src/libbreakpad.a @@ -334,6 +336,7 @@ src_tools_linux_dump_syms_dump_syms_SOURCES = \ src/common/linux/elf_symbols_to_module.cc \ src/common/linux/file_id.cc \ src/common/linux/memory_mapped_file.cc \ + src/common/linux/safe_readlink.cc \ src/tools/linux/dump_syms/dump_syms.cc src_tools_linux_md2core_minidump_2_core_SOURCES = \ @@ -384,6 +387,8 @@ src_common_dumper_unittest_SOURCES = \ src/common/linux/synth_elf_unittest.cc \ src/common/linux/file_id.cc \ src/common/linux/file_id_unittest.cc \ + src/common/linux/safe_readlink.cc \ + src/common/linux/safe_readlink_unittest.cc \ src/testing/gtest/src/gtest-all.cc \ src/testing/gtest/src/gtest_main.cc \ src/testing/src/gmock-all.cc diff --git a/Makefile.in b/Makefile.in index 001ac84d..785233ca 100644 --- a/Makefile.in +++ b/Makefile.in @@ -174,7 +174,8 @@ am__src_client_linux_libbreakpad_client_a_SOURCES_DIST = \ src/client/minidump_file_writer.cc src/common/convert_UTF.c \ src/common/md5.cc src/common/string_conversion.cc \ src/common/linux/file_id.cc src/common/linux/guid_creator.cc \ - src/common/linux/memory_mapped_file.cc + src/common/linux/memory_mapped_file.cc \ + src/common/linux/safe_readlink.cc am__dirstamp = $(am__leading_dot)dirstamp @LINUX_HOST_TRUE@am_src_client_linux_libbreakpad_client_a_OBJECTS = src/client/linux/crash_generation/crash_generation_client.$(OBJEXT) \ @LINUX_HOST_TRUE@ src/client/linux/handler/exception_handler.$(OBJEXT) \ @@ -186,7 +187,8 @@ am__dirstamp = $(am__leading_dot)dirstamp @LINUX_HOST_TRUE@ src/common/string_conversion.$(OBJEXT) \ @LINUX_HOST_TRUE@ src/common/linux/file_id.$(OBJEXT) \ @LINUX_HOST_TRUE@ src/common/linux/guid_creator.$(OBJEXT) \ -@LINUX_HOST_TRUE@ src/common/linux/memory_mapped_file.$(OBJEXT) +@LINUX_HOST_TRUE@ src/common/linux/memory_mapped_file.$(OBJEXT) \ +@LINUX_HOST_TRUE@ src/common/linux/safe_readlink.$(OBJEXT) src_client_linux_libbreakpad_client_a_OBJECTS = \ $(am_src_client_linux_libbreakpad_client_a_OBJECTS) src_libbreakpad_a_AR = $(AR) $(ARFLAGS) @@ -445,6 +447,8 @@ am__src_common_dumper_unittest_SOURCES_DIST = \ src/common/linux/synth_elf_unittest.cc \ src/common/linux/file_id.cc \ src/common/linux/file_id_unittest.cc \ + src/common/linux/safe_readlink.cc \ + src/common/linux/safe_readlink_unittest.cc \ src/testing/gtest/src/gtest-all.cc \ src/testing/gtest/src/gtest_main.cc \ src/testing/src/gmock-all.cc @@ -481,6 +485,8 @@ am__src_common_dumper_unittest_SOURCES_DIST = \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/src_common_dumper_unittest-synth_elf_unittest.$(OBJEXT) \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/src_common_dumper_unittest-file_id.$(OBJEXT) \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/src_common_dumper_unittest-file_id_unittest.$(OBJEXT) \ +@DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/src_common_dumper_unittest-safe_readlink.$(OBJEXT) \ +@DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.$(OBJEXT) \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/testing/gtest/src/src_common_dumper_unittest-gtest-all.$(OBJEXT) \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/testing/gtest/src/src_common_dumper_unittest-gtest_main.$(OBJEXT) \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/testing/src/src_common_dumper_unittest-gmock-all.$(OBJEXT) @@ -889,6 +895,7 @@ am__src_tools_linux_dump_syms_dump_syms_SOURCES_DIST = \ src/common/linux/elf_symbols_to_module.cc \ src/common/linux/file_id.cc \ src/common/linux/memory_mapped_file.cc \ + src/common/linux/safe_readlink.cc \ src/tools/linux/dump_syms/dump_syms.cc @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@am_src_tools_linux_dump_syms_dump_syms_OBJECTS = src/common/dwarf_cfi_to_module.$(OBJEXT) \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/dwarf_cu_to_module.$(OBJEXT) \ @@ -904,6 +911,7 @@ am__src_tools_linux_dump_syms_dump_syms_SOURCES_DIST = \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/elf_symbols_to_module.$(OBJEXT) \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/file_id.$(OBJEXT) \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/memory_mapped_file.$(OBJEXT) \ +@DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/safe_readlink.$(OBJEXT) \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/tools/linux/dump_syms/dump_syms.$(OBJEXT) src_tools_linux_dump_syms_dump_syms_OBJECTS = \ $(am_src_tools_linux_dump_syms_dump_syms_OBJECTS) @@ -1172,7 +1180,8 @@ lib_LIBRARIES = $(am__append_1) $(am__append_3) @LINUX_HOST_TRUE@ src/common/string_conversion.cc \ @LINUX_HOST_TRUE@ src/common/linux/file_id.cc \ @LINUX_HOST_TRUE@ src/common/linux/guid_creator.cc \ -@LINUX_HOST_TRUE@ src/common/linux/memory_mapped_file.cc +@LINUX_HOST_TRUE@ src/common/linux/memory_mapped_file.cc \ +@LINUX_HOST_TRUE@ src/common/linux/safe_readlink.cc @DISABLE_PROCESSOR_FALSE@src_libbreakpad_a_SOURCES = \ @DISABLE_PROCESSOR_FALSE@ src/google_breakpad/common/breakpad_types.h \ @@ -1342,6 +1351,7 @@ TESTS_ENVIRONMENT = @LINUX_HOST_TRUE@ src/common/linux/file_id.o \ @LINUX_HOST_TRUE@ src/common/linux/guid_creator.o \ @LINUX_HOST_TRUE@ src/common/linux/memory_mapped_file.o \ +@LINUX_HOST_TRUE@ src/common/linux/safe_readlink.o \ @LINUX_HOST_TRUE@ src/common/string_conversion.o @LINUX_HOST_TRUE@src_client_linux_linux_client_unittest_DEPENDENCIES = src/client/linux/linux_dumper_unittest_helper src/client/linux/libbreakpad_client.a src/libbreakpad.a @@ -1360,6 +1370,7 @@ TESTS_ENVIRONMENT = @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/elf_symbols_to_module.cc \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/file_id.cc \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/memory_mapped_file.cc \ +@DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/safe_readlink.cc \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/tools/linux/dump_syms/dump_syms.cc @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@src_tools_linux_md2core_minidump_2_core_SOURCES = \ @@ -1410,6 +1421,8 @@ TESTS_ENVIRONMENT = @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/synth_elf_unittest.cc \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/file_id.cc \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/file_id_unittest.cc \ +@DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/safe_readlink.cc \ +@DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/common/linux/safe_readlink_unittest.cc \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/testing/gtest/src/gtest-all.cc \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/testing/gtest/src/gtest_main.cc \ @DISABLE_TOOLS_FALSE@@LINUX_HOST_TRUE@ src/testing/src/gmock-all.cc @@ -2150,6 +2163,9 @@ src/common/linux/guid_creator.$(OBJEXT): \ src/common/linux/memory_mapped_file.$(OBJEXT): \ src/common/linux/$(am__dirstamp) \ src/common/linux/$(DEPDIR)/$(am__dirstamp) +src/common/linux/safe_readlink.$(OBJEXT): \ + src/common/linux/$(am__dirstamp) \ + src/common/linux/$(DEPDIR)/$(am__dirstamp) src/client/linux/$(am__dirstamp): @$(MKDIR_P) src/client/linux @: > src/client/linux/$(am__dirstamp) @@ -2496,6 +2512,12 @@ src/common/linux/src_common_dumper_unittest-file_id.$(OBJEXT): \ src/common/linux/src_common_dumper_unittest-file_id_unittest.$(OBJEXT): \ src/common/linux/$(am__dirstamp) \ src/common/linux/$(DEPDIR)/$(am__dirstamp) +src/common/linux/src_common_dumper_unittest-safe_readlink.$(OBJEXT): \ + src/common/linux/$(am__dirstamp) \ + src/common/linux/$(DEPDIR)/$(am__dirstamp) +src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.$(OBJEXT): \ + src/common/linux/$(am__dirstamp) \ + src/common/linux/$(DEPDIR)/$(am__dirstamp) src/testing/gtest/src/src_common_dumper_unittest-gtest-all.$(OBJEXT): \ src/testing/gtest/src/$(am__dirstamp) \ src/testing/gtest/src/$(DEPDIR)/$(am__dirstamp) @@ -2931,6 +2953,7 @@ mostlyclean-compile: -rm -f src/common/linux/guid_creator.$(OBJEXT) -rm -f src/common/linux/http_upload.$(OBJEXT) -rm -f src/common/linux/memory_mapped_file.$(OBJEXT) + -rm -f src/common/linux/safe_readlink.$(OBJEXT) -rm -f src/common/linux/src_common_dumper_unittest-dump_symbols.$(OBJEXT) -rm -f src/common/linux/src_common_dumper_unittest-dump_symbols_unittest.$(OBJEXT) -rm -f src/common/linux/src_common_dumper_unittest-elf_symbols_to_module.$(OBJEXT) @@ -2939,6 +2962,8 @@ mostlyclean-compile: -rm -f src/common/linux/src_common_dumper_unittest-file_id_unittest.$(OBJEXT) -rm -f src/common/linux/src_common_dumper_unittest-memory_mapped_file.$(OBJEXT) -rm -f src/common/linux/src_common_dumper_unittest-memory_mapped_file_unittest.$(OBJEXT) + -rm -f src/common/linux/src_common_dumper_unittest-safe_readlink.$(OBJEXT) + -rm -f src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.$(OBJEXT) -rm -f src/common/linux/src_common_dumper_unittest-synth_elf.$(OBJEXT) -rm -f src/common/linux/src_common_dumper_unittest-synth_elf_unittest.$(OBJEXT) -rm -f src/common/md5.$(OBJEXT) @@ -3160,6 +3185,7 @@ distclean-compile: @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/guid_creator.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/http_upload.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/memory_mapped_file.Po@am__quote@ +@AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/safe_readlink.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-dump_symbols.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-dump_symbols_unittest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-elf_symbols_to_module.Po@am__quote@ @@ -3168,6 +3194,8 @@ distclean-compile: @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-file_id_unittest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-memory_mapped_file.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-memory_mapped_file_unittest.Po@am__quote@ +@AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink.Po@am__quote@ +@AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink_unittest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-synth_elf.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/common/linux/$(DEPDIR)/src_common_dumper_unittest-synth_elf_unittest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@src/processor/$(DEPDIR)/address_map_unittest.Po@am__quote@ @@ -3986,6 +4014,34 @@ src/common/linux/src_common_dumper_unittest-file_id_unittest.obj: src/common/lin @AMDEP_TRUE@@am__fastdepCXX_FALSE@ DEPDIR=$(DEPDIR) $(CXXDEPMODE) $(depcomp) @AMDEPBACKSLASH@ @am__fastdepCXX_FALSE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -c -o src/common/linux/src_common_dumper_unittest-file_id_unittest.obj `if test -f 'src/common/linux/file_id_unittest.cc'; then $(CYGPATH_W) 'src/common/linux/file_id_unittest.cc'; else $(CYGPATH_W) '$(srcdir)/src/common/linux/file_id_unittest.cc'; fi` +src/common/linux/src_common_dumper_unittest-safe_readlink.o: src/common/linux/safe_readlink.cc +@am__fastdepCXX_TRUE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -MT src/common/linux/src_common_dumper_unittest-safe_readlink.o -MD -MP -MF src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink.Tpo -c -o src/common/linux/src_common_dumper_unittest-safe_readlink.o `test -f 'src/common/linux/safe_readlink.cc' || echo '$(srcdir)/'`src/common/linux/safe_readlink.cc +@am__fastdepCXX_TRUE@ $(am__mv) src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink.Tpo src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink.Po +@AMDEP_TRUE@@am__fastdepCXX_FALSE@ source='src/common/linux/safe_readlink.cc' object='src/common/linux/src_common_dumper_unittest-safe_readlink.o' libtool=no @AMDEPBACKSLASH@ +@AMDEP_TRUE@@am__fastdepCXX_FALSE@ DEPDIR=$(DEPDIR) $(CXXDEPMODE) $(depcomp) @AMDEPBACKSLASH@ +@am__fastdepCXX_FALSE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -c -o src/common/linux/src_common_dumper_unittest-safe_readlink.o `test -f 'src/common/linux/safe_readlink.cc' || echo '$(srcdir)/'`src/common/linux/safe_readlink.cc + +src/common/linux/src_common_dumper_unittest-safe_readlink.obj: src/common/linux/safe_readlink.cc +@am__fastdepCXX_TRUE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -MT src/common/linux/src_common_dumper_unittest-safe_readlink.obj -MD -MP -MF src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink.Tpo -c -o src/common/linux/src_common_dumper_unittest-safe_readlink.obj `if test -f 'src/common/linux/safe_readlink.cc'; then $(CYGPATH_W) 'src/common/linux/safe_readlink.cc'; else $(CYGPATH_W) '$(srcdir)/src/common/linux/safe_readlink.cc'; fi` +@am__fastdepCXX_TRUE@ $(am__mv) src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink.Tpo src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink.Po +@AMDEP_TRUE@@am__fastdepCXX_FALSE@ source='src/common/linux/safe_readlink.cc' object='src/common/linux/src_common_dumper_unittest-safe_readlink.obj' libtool=no @AMDEPBACKSLASH@ +@AMDEP_TRUE@@am__fastdepCXX_FALSE@ DEPDIR=$(DEPDIR) $(CXXDEPMODE) $(depcomp) @AMDEPBACKSLASH@ +@am__fastdepCXX_FALSE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -c -o src/common/linux/src_common_dumper_unittest-safe_readlink.obj `if test -f 'src/common/linux/safe_readlink.cc'; then $(CYGPATH_W) 'src/common/linux/safe_readlink.cc'; else $(CYGPATH_W) '$(srcdir)/src/common/linux/safe_readlink.cc'; fi` + +src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.o: src/common/linux/safe_readlink_unittest.cc +@am__fastdepCXX_TRUE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -MT src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.o -MD -MP -MF src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink_unittest.Tpo -c -o src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.o `test -f 'src/common/linux/safe_readlink_unittest.cc' || echo '$(srcdir)/'`src/common/linux/safe_readlink_unittest.cc +@am__fastdepCXX_TRUE@ $(am__mv) src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink_unittest.Tpo src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink_unittest.Po +@AMDEP_TRUE@@am__fastdepCXX_FALSE@ source='src/common/linux/safe_readlink_unittest.cc' object='src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.o' libtool=no @AMDEPBACKSLASH@ +@AMDEP_TRUE@@am__fastdepCXX_FALSE@ DEPDIR=$(DEPDIR) $(CXXDEPMODE) $(depcomp) @AMDEPBACKSLASH@ +@am__fastdepCXX_FALSE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -c -o src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.o `test -f 'src/common/linux/safe_readlink_unittest.cc' || echo '$(srcdir)/'`src/common/linux/safe_readlink_unittest.cc + +src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.obj: src/common/linux/safe_readlink_unittest.cc +@am__fastdepCXX_TRUE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -MT src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.obj -MD -MP -MF src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink_unittest.Tpo -c -o src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.obj `if test -f 'src/common/linux/safe_readlink_unittest.cc'; then $(CYGPATH_W) 'src/common/linux/safe_readlink_unittest.cc'; else $(CYGPATH_W) '$(srcdir)/src/common/linux/safe_readlink_unittest.cc'; fi` +@am__fastdepCXX_TRUE@ $(am__mv) src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink_unittest.Tpo src/common/linux/$(DEPDIR)/src_common_dumper_unittest-safe_readlink_unittest.Po +@AMDEP_TRUE@@am__fastdepCXX_FALSE@ source='src/common/linux/safe_readlink_unittest.cc' object='src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.obj' libtool=no @AMDEPBACKSLASH@ +@AMDEP_TRUE@@am__fastdepCXX_FALSE@ DEPDIR=$(DEPDIR) $(CXXDEPMODE) $(depcomp) @AMDEPBACKSLASH@ +@am__fastdepCXX_FALSE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -c -o src/common/linux/src_common_dumper_unittest-safe_readlink_unittest.obj `if test -f 'src/common/linux/safe_readlink_unittest.cc'; then $(CYGPATH_W) 'src/common/linux/safe_readlink_unittest.cc'; else $(CYGPATH_W) '$(srcdir)/src/common/linux/safe_readlink_unittest.cc'; fi` + src/testing/gtest/src/src_common_dumper_unittest-gtest-all.o: src/testing/gtest/src/gtest-all.cc @am__fastdepCXX_TRUE@ $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(src_common_dumper_unittest_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -MT src/testing/gtest/src/src_common_dumper_unittest-gtest-all.o -MD -MP -MF src/testing/gtest/src/$(DEPDIR)/src_common_dumper_unittest-gtest-all.Tpo -c -o src/testing/gtest/src/src_common_dumper_unittest-gtest-all.o `test -f 'src/testing/gtest/src/gtest-all.cc' || echo '$(srcdir)/'`src/testing/gtest/src/gtest-all.cc @am__fastdepCXX_TRUE@ $(am__mv) src/testing/gtest/src/$(DEPDIR)/src_common_dumper_unittest-gtest-all.Tpo src/testing/gtest/src/$(DEPDIR)/src_common_dumper_unittest-gtest-all.Po diff --git a/src/client/linux/crash_generation/crash_generation_server.cc b/src/client/linux/crash_generation/crash_generation_server.cc index 2f7edb69..76c9d9cd 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 @@ GetInodeForProcPath(ino_t* inode_out, const char* path) 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 @@ FindProcessHoldingSocket(pid_t* pid_out, ino_t socket_inode) for (std::vector::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 f4ab5b1f..a119abaf 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 @@ bool LinuxDumper::HandleDeletedFileInMapping(char* path) const { 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 1eee9a18..6f4e0e44 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 @@ typedef testing::Test LinuxDumperTest; 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 @@ TEST(LinuxDumperTest, FileIDsMatch) { // 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 41465268..ee021ae3 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 @@ TEST(MinidumpWriterTest, DeletedBinary) { // 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 977f97d2..94bc80e4 100644 --- a/src/common/linux/file_id_unittest.cc +++ b/src/common/linux/file_id_unittest.cc @@ -33,12 +33,14 @@ #include #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 @@ TEST(FileIDStripTest, StripSelf) { // 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 00000000..870c28af --- /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 + +#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(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 00000000..4ae131b5 --- /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 + +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 +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 00000000..191fb9f1 --- /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)); +} -- cgit v1.2.1