input.c/libcpp: fix lifetimes of path buffers

Running "make selftest-valgrind" showed various leaks of the form:

408 bytes in 24 blocks are definitely lost in loss record 572 of 679
   at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x1B0D057: xmalloc (xmalloc.c:148)
   by 0x1ACCAA1: append_file_to_dir(char const*, cpp_dir*) [clone .isra.3] (files.c:1567)
   by 0x1ACD56F: _cpp_find_file (files.c:390)
   by 0x1ACF8FB: cpp_read_main_file(cpp_reader*, char const*) (init.c:632)
   by 0x1AB3D97: selftest::lexer_test::lexer_test(selftest::line_table_case const&, char const*, selftest::lexer_test_options*) (input.c:2014)
   by 0x1AB792B: selftest::test_lexer_string_locations_u8(selftest::line_table_case const&) (input.c:2713)
   by 0x1ABA22A: selftest::for_each_line_table_case(void (*)(selftest::line_table_case const&)) (input.c:3227)
   by 0x1ABA381: selftest::input_c_tests() (input.c:3260)
   by 0x1A295F1: selftest::run_tests() (selftest-run-tests.c:62)
   by 0xF20DC4: toplev::run_self_tests() (toplev.c:2076)
   by 0xF20FCD: toplev::main(int, char**) (toplev.c:2153)

Fix the leak by freeing the file->path in destroy_cpp_file.
However, doing so would lead to a use-after-free in input.c's file cache
since the filenames in this cache are the libcpp file->path buffers.

Hence we need to ensure that any references to the file in the input.c
cache are purged before cleaning up file->path.  This is normally done
by the temp_source_file dtor.  Hence we need to reorder things to that
the temp_source_file dtor runs before cleaning up the cpp_parser.  The
patch does this by introducing a wrapper class around cpp_parser *, so
that the dtor can run after the dtor for temp_source_file.

gcc/ChangeLog:
	* input.c (fcache::file_patch): Add comment about lifetime.
	(selftest::cpp_reader_ptr): New class.
	(selftest::lexer_test): Convert m_parser from cpp_reader *
	to a cpp_reader_ptr, and move m_tempfile to after it.
	(selftest::lexer_test::lexer_test): Update for above reordering.
	(lexer_test::~lexer_test): Move cleanup of m_parser to
	cpp_reader_ptr's dtor.

libcpp/ChangeLog:
	* files.c (destroy_cpp_file): Free file->path.

From-SVN: r241536

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index f44656fac3c7e861e543fcfd531482cd88131916..a1a920038afc6b7c5b0f1d931e936e35224f2a2a 100644 (file)
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,13 @@
+2016-10-25  David Malcolm  <dmalcolm@redhat.com>
+
+       * input.c (fcache::file_patch): Add comment about lifetime.
+       (selftest::cpp_reader_ptr): New class.
+       (selftest::lexer_test): Convert m_parser from cpp_reader *
+       to a cpp_reader_ptr, and move m_tempfile to after it.
+       (selftest::lexer_test::lexer_test): Update for above reordering.
+       (lexer_test::~lexer_test): Move cleanup of m_parser to
+       cpp_reader_ptr's dtor.
+
 2016-10-25  David Malcolm  <dmalcolm@redhat.com>
 
        * toplev.c (toplev::main): Remove call to

diff --git a/gcc/input.c b/gcc/input.c
index 61316599cbd25329e6536c2068d4821400b80080..c2042e8e211b1f2f591663233b6c20d03a3d4238 100644 (file)
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -63,6 +63,10 @@ struct fcache
      array.  */
   unsigned use_count;
 
+  /* The file_path is the key for identifying a particular file in
+     the cache.
+     For libcpp-using code, the underlying buffer for this field is
+     owned by the corresponding _cpp_file within the cpp_reader.  */
   const char *file_path;
 
   FILE *fp;
@@ -1918,6 +1922,29 @@ class lexer_test_options
   virtual void apply (lexer_test &) = 0;
 };
 
+/* Wrapper around an cpp_reader *, which calls cpp_finish and cpp_destroy
+   in its dtor.
+
+   This is needed by struct lexer_test to ensure that the cleanup of the
+   cpp_reader happens *after* the cleanup of the temp_source_file.  */
+
+class cpp_reader_ptr
+{
+ public:
+  cpp_reader_ptr (cpp_reader *ptr) : m_ptr (ptr) {}
+
+  ~cpp_reader_ptr ()
+  {
+    cpp_finish (m_ptr, NULL);
+    cpp_destroy (m_ptr);
+  }
+
+  operator cpp_reader * () const { return m_ptr; }
+
+ private:
+  cpp_reader *m_ptr;
+};
+
 /* A struct for writing lexer tests.  */
 
 struct lexer_test
@@ -1928,9 +1955,16 @@ struct lexer_test
 
   const cpp_token *get_token ();
 
-  temp_source_file m_tempfile;
+  /* The ordering of these fields matters.
+     The line_table_test must be first, since the cpp_reader_ptr
+     uses it.
+     The cpp_reader must be cleaned up *after* the temp_source_file
+     since the filenames in input.c's input cache are owned by the
+     cpp_reader; in particular, when ~temp_source_file evicts the
+     filename the filenames must still be alive.  */
   line_table_test m_ltt;
-  cpp_reader *m_parser;
+  cpp_reader_ptr m_parser;
+  temp_source_file m_tempfile;
   string_concat_db m_concats;
 };
 
@@ -1998,11 +2032,11 @@ ebcdic_execution_charset *ebcdic_execution_charset::s_singleton;
    start parsing the tempfile.  */
 
 lexer_test::lexer_test (const line_table_case &case_, const char *content,
-                       lexer_test_options *options) :
+                       lexer_test_options *options)
+: m_ltt (case_),
+  m_parser (cpp_create_reader (CLK_GNUC99, NULL, line_table)),
   /* Create a tempfile and write the text to it.  */
   m_tempfile (SELFTEST_LOCATION, ".c", content),
-  m_ltt (case_),
-  m_parser (cpp_create_reader (CLK_GNUC99, NULL, line_table)),
   m_concats ()
 {
   if (options)
@@ -2026,9 +2060,6 @@ lexer_test::~lexer_test ()
   tok = cpp_get_token_with_location (m_parser, &loc);
   ASSERT_NE (tok, NULL);
   ASSERT_EQ (tok->type, CPP_EOF);
-
-  cpp_finish (m_parser, NULL);
-  cpp_destroy (m_parser);
 }
 
 /* Get the next token from m_parser.  */

diff --git a/libcpp/ChangeLog b/libcpp/ChangeLog
index 5ff0aad08f876660c0d44aa91f030dcfc63f94fe..9083bdab980d77bd69ec4cae326c46bdbae60c49 100644 (file)
--- a/libcpp/ChangeLog
+++ b/libcpp/ChangeLog
@@ -1,3 +1,7 @@
+2016-10-25  David Malcolm  <dmalcolm@redhat.com>
+
+       * files.c (destroy_cpp_file): Free file->path.
+
 2016-10-25  David Malcolm  <dmalcolm@redhat.com>
 
        * include/line-map.h (line_maps::~line_maps): New dtor.

diff --git a/libcpp/files.c b/libcpp/files.c
index e859dfe369382da287140399ac2aeca1df029044..2c50e6c27e1b811fde42a2c989fd5c241d8e52c3 100644 (file)
--- a/libcpp/files.c
+++ b/libcpp/files.c
@@ -1132,6 +1132,7 @@ destroy_cpp_file (_cpp_file *file)
 {
   free ((void *) file->buffer_start);
   free ((void *) file->name);
+  free ((void *) file->path);
   free (file);
 }