From f5ea989d50a3c4965667ad40807f18737f231b91 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Tue, 25 Oct 2016 19:24:01 +0000 Subject: [PATCH] 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 --- gcc/ChangeLog | 10 ++++++++++ gcc/input.c | 47 +++++++++++++++++++++++++++++++++++++++-------- libcpp/ChangeLog | 4 ++++ libcpp/files.c | 1 + 4 files changed, 54 insertions(+), 8 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index f44656fac3c..a1a920038af 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,13 @@ +2016-10-25 David Malcolm + + * 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 * toplev.c (toplev::main): Remove call to diff --git a/gcc/input.c b/gcc/input.c index 61316599cbd..c2042e8e211 100644 --- 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 5ff0aad08f8..9083bdab980 100644 --- a/libcpp/ChangeLog +++ b/libcpp/ChangeLog @@ -1,3 +1,7 @@ +2016-10-25 David Malcolm + + * files.c (destroy_cpp_file): Free file->path. + 2016-10-25 David Malcolm * include/line-map.h (line_maps::~line_maps): New dtor. diff --git a/libcpp/files.c b/libcpp/files.c index e859dfe3693..2c50e6c27e1 100644 --- 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); } -- 2.30.2