From f5ea989d50a3c4965667ad40807f18737f231b91 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Tue, 25 Oct 2016 19:24:01 +0000 Subject: 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/input.c | 47 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 8 deletions(-) (limited to 'gcc/input.c') diff --git a/gcc/input.c b/gcc/input.c index 6131659..c2042e8 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. */ -- cgit v1.1