From b46584d7836bee011facdf946fd7241c748d66f0 Mon Sep 17 00:00:00 2001 From: Alex Coplan Date: Fri, 21 Aug 2020 14:26:11 +0100 Subject: [PATCH] driver: Fix several memory leaks [PR63854] This patch fixes several memory leaks in the driver, all of which relate to the handling of static specs. We introduce functions set_static_spec_{shared,owned}() which are used to enforce proper memory management when updating the strings in the static_specs table. This is achieved by making use of the alloc_p field in the table entries. Similarly to set_spec(), each time we update an entry, we check whether alloc_p is set, and free the old value if so. We then set alloc_p correctly based on whether we "own" this memory or whether we're just taking a pointer to a shared string which we shouldn't free. The following table shows the number of leaks found by AddressSanitizer when running a minimal libgccjit program on AArch64. The test program does the whole libgccjit compilation cycle in a loop (including acquiring and releasing the context), and the table below shows the number of leaks for different iterations of that loop. +--------------+-----+-----+------+---------------+ | # of runs > | 1 | 2 | 3 | Leaks per run | +--------------+-----+-----+------+---------------+ | Before patch | 463 | 940 | 1417 | 477 | +--------------+-----+-----+------+---------------+ | After patch | 416 | 846 | 1276 | 430 | +--------------+-----+-----+------+---------------+ gcc/ChangeLog: PR jit/63854 * gcc.c (set_static_spec): New. (set_static_spec_owned): New. (set_static_spec_shared): New. (driver::maybe_putenv_COLLECT_LTO_WRAPPER): Use set_static_spec_owned() to take ownership of lto_wrapper_file such that it gets freed in driver::finalize. (driver::maybe_run_linker): Use set_static_spec_shared() to ensure that we don't try and free() the static string "ld", also ensuring that any previously-allocated string in linker_name_spec is freed. Likewise with argv0. (driver::finalize): Use set_static_spec_shared() when resetting specs that previously had allocated strings; remove if(0) around call to free(). --- gcc/gcc.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 52 insertions(+), 8 deletions(-) diff --git a/gcc/gcc.c b/gcc/gcc.c index 10bc9881aed..a38d684f34d 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -1908,6 +1908,51 @@ init_spec (void) specs = sl; } + +/* Update the entry for SPEC in the static_specs table to point to VALUE, + ensuring that we free the previous value if necessary. Set alloc_p for the + entry to ALLOC_P: this determines whether we take ownership of VALUE (i.e. + whether we need to free it later on). */ +static void +set_static_spec (const char **spec, const char *value, bool alloc_p) +{ + struct spec_list *sl = NULL; + + for (unsigned i = 0; i < ARRAY_SIZE (static_specs); i++) + { + if (static_specs[i].ptr_spec == spec) + { + sl = static_specs + i; + break; + } + } + + gcc_assert (sl); + + if (sl->alloc_p) + { + const char *old = *spec; + free (const_cast (old)); + } + + *spec = value; + sl->alloc_p = alloc_p; +} + +/* Update a static spec to a new string, taking ownership of that + string's memory. */ +static void set_static_spec_owned (const char **spec, const char *val) +{ + return set_static_spec (spec, val, true); +} + +/* Update a static spec to point to a new value, but don't take + ownership of (i.e. don't free) that string. */ +static void set_static_spec_shared (const char **spec, const char *val) +{ + return set_static_spec (spec, val, false); +} + /* Change the value of spec NAME to SPEC. If SPEC is empty, then the spec is removed; If the spec starts with a + then SPEC is added to the end of the @@ -8344,7 +8389,7 @@ driver::maybe_putenv_COLLECT_LTO_WRAPPER () const if (lto_wrapper_file) { lto_wrapper_file = convert_white_space (lto_wrapper_file); - lto_wrapper_spec = lto_wrapper_file; + set_static_spec_owned (<o_wrapper_spec, lto_wrapper_file); obstack_init (&collect_obstack); obstack_grow (&collect_obstack, "COLLECT_LTO_WRAPPER=", sizeof ("COLLECT_LTO_WRAPPER=") - 1); @@ -8851,7 +8896,7 @@ driver::maybe_run_linker (const char *argv0) const { char *s = find_a_file (&exec_prefixes, "collect2", X_OK, false); if (s == NULL) - linker_name_spec = "ld"; + set_static_spec_shared (&linker_name_spec, "ld"); } #if HAVE_LTO_PLUGIN > 0 @@ -8875,7 +8920,7 @@ driver::maybe_run_linker (const char *argv0) const linker_plugin_file_spec = convert_white_space (temp_spec); } #endif - lto_gcc_spec = argv0; + set_static_spec_shared (<o_gcc_spec, argv0); } /* Rebuild the COMPILER_PATH and LIBRARY_PATH environment variables @@ -10817,9 +10862,9 @@ driver::finalize () just_machine_suffix = 0; gcc_exec_prefix = 0; gcc_libexec_prefix = 0; - md_exec_prefix = MD_EXEC_PREFIX; - md_startfile_prefix = MD_STARTFILE_PREFIX; - md_startfile_prefix_1 = MD_STARTFILE_PREFIX_1; + set_static_spec_shared (&md_exec_prefix, MD_EXEC_PREFIX); + set_static_spec_shared (&md_startfile_prefix, MD_STARTFILE_PREFIX); + set_static_spec_shared (&md_startfile_prefix_1, MD_STARTFILE_PREFIX_1); multilib_dir = 0; multilib_os_dir = 0; multiarch_dir = 0; @@ -10843,8 +10888,7 @@ driver::finalize () spec_list *sl = &static_specs[i]; if (sl->alloc_p) { - if (0) - free (const_cast (*(sl->ptr_spec))); + free (const_cast (*(sl->ptr_spec))); sl->alloc_p = false; } *(sl->ptr_spec) = sl->default_ptr; -- 2.30.2