From 9db74434cd34b2b875b3f9bfbab4f1e0b682e27c Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Sun, 26 Mar 2023 15:35:00 -0400 Subject: Clean up memory leak warnings from ASAN and Valgrind * src/main.c (main): Add "sanitize" to .FEATURES if ASAN is enabled. * src/expand.c (expand_variable_output): Remember "recursive" setting in case it's changed by the expansion of the variable. * src/file.c (rehash_file): If we drop a file from the global 'files' hash, remember it in rehashed_files. We can't free it because it's still being referenced (callers will invoke check_renamed()) but it will be a leak since it's no longer referenced by 'files'. * src/remake.c (update_file_1): If we drop a dependency, remember it in dropped_list. We can't free it because it's still being referenced by callers but it will be a leak since it's no longer referenced as a prerequisite. * tests/scripts/functions/guile: Don't run Guile tests when ASAN is enabled. * tests/scripts/functions/wildcard: Enabling ASAN causes glob(3) to break! Don't run this test. * tests/scripts/features/exec: Valgrind's exec() doesn't support scripts with no shbang. * tests/scripts/jobserver: Valgrind fails if TMPDIR is set to an invalid directory: skip those tests. * tests/scripts/features/output-sync: Ditto. * tests/scripts/features/temp_stdin: Ditto. --- src/expand.c | 8 ++++++-- src/file.c | 17 +++++++++++++++-- src/main.c | 3 +++ src/read.c | 26 ++++++++++++++------------ src/remake.c | 19 ++++++++++++++++--- tests/scripts/features/exec | 6 +++++- tests/scripts/features/jobserver | 3 ++- tests/scripts/features/output-sync | 18 ++++++++++++------ tests/scripts/features/temp_stdin | 19 ++++++++++++------- tests/scripts/functions/guile | 3 ++- tests/scripts/functions/wildcard | 3 ++- 11 files changed, 89 insertions(+), 36 deletions(-) diff --git a/src/expand.c b/src/expand.c index 01fff81e..533e7dfa 100644 --- a/src/expand.c +++ b/src/expand.c @@ -226,6 +226,7 @@ char * expand_variable_output (char *ptr, const char *name, size_t length) { struct variable *v; + unsigned int recursive; char *value; v = lookup_variable (name, length); @@ -237,11 +238,14 @@ expand_variable_output (char *ptr, const char *name, size_t length) if (!v || (v->value[0] == '\0' && !v->append)) return ptr; - value = v->recursive ? recursively_expand (v) : v->value; + /* Remember this since expansion could change it. */ + recursive = v->recursive; + + value = recursive ? recursively_expand (v) : v->value; ptr = variable_buffer_output (ptr, value, strlen (value)); - if (v->recursive) + if (recursive) free (value); return ptr; diff --git a/src/file.c b/src/file.c index 9828d283..f8e4fb91 100644 --- a/src/file.c +++ b/src/file.c @@ -60,6 +60,14 @@ file_hash_cmp (const void *x, const void *y) static struct hash_table files; +/* We can't free files we take out of the hash table, because they are still + likely pointed to in various places. The check_renamed() will be used if + we come across these, to find the new correct file. This is mainly to + prevent leak checkers from complaining. */ +static struct file **rehashed_files = NULL; +static size_t rehashed_files_len = 0; +#define REHASHED_FILES_INCR 5 + /* Whether or not .SECONDARY with no prerequisites was given. */ static int all_secondary = 0; @@ -217,8 +225,7 @@ rehash_file (struct file *from_file, const char *to_hname) /* Find the end of the renamed list for the "from" file. */ file_key.hname = from_file->hname; - while (from_file->renamed != 0) - from_file = from_file->renamed; + check_renamed (from_file); if (file_hash_cmp (from_file, &file_key)) /* hname changed unexpectedly!! */ abort (); @@ -331,6 +338,12 @@ rehash_file (struct file *from_file, const char *to_hname) to_file->builtin = 0; from_file->renamed = to_file; + + if (rehashed_files_len % REHASHED_FILES_INCR == 0) + rehashed_files = xrealloc (rehashed_files, + sizeof (struct file *) * (rehashed_files_len + REHASHED_FILES_INCR)); + + rehashed_files[rehashed_files_len++] = from_file; } /* Rename FILE to NAME. This is not as simple as resetting diff --git a/src/main.c b/src/main.c index 16bbb83a..bf74ab94 100644 --- a/src/main.c +++ b/src/main.c @@ -1466,6 +1466,9 @@ main (int argc, char **argv, char **envp) #endif #ifdef MAKE_MAINTAINER_MODE " maintainer" +#endif +#if defined(__SANITIZE_ADDRESS__) || defined(__SANITIZE_MEMORY__) + " sanitize" #endif ; diff --git a/src/read.c b/src/read.c index bae836f2..2d25be6a 100644 --- a/src/read.c +++ b/src/read.c @@ -3261,22 +3261,24 @@ parse_file_seq (char **stringp, size_t size, int stopmap, /* Strip leading "this directory" references. */ if (NONE_SET (flags, PARSEFS_NOSTRIP)) + { #if MK_OS_VMS - /* Skip leading '[]'s. should only be one set or bug somewhere else */ - if (p - s > 2 && s[0] == '[' && s[1] == ']') + /* Skip leading '[]'s. should only be one set or bug somewhere else */ + if (p - s > 2 && s[0] == '[' && s[1] == ']') s += 2; - /* Skip leading '<>'s. should only be one set or bug somewhere else */ - if (p - s > 2 && s[0] == '<' && s[1] == '>') + /* Skip leading '<>'s. should only be one set or bug somewhere else */ + if (p - s > 2 && s[0] == '<' && s[1] == '>') s += 2; #endif - /* Skip leading './'s. */ - while (p - s > 2 && s[0] == '.' && s[1] == '/') - { - /* Skip "./" and all following slashes. */ - s += 2; - while (*s == '/') - ++s; - } + /* Skip leading './'s. */ + while (p - s > 2 && s[0] == '.' && s[1] == '/') + { + /* Skip "./" and all following slashes. */ + s += 2; + while (*s == '/') + ++s; + } + } /* Extract the filename just found, and skip it. Set NAME to the string, and NLEN to its length. */ diff --git a/src/remake.c b/src/remake.c index d5cd3b38..dfe981c3 100644 --- a/src/remake.c +++ b/src/remake.c @@ -69,6 +69,13 @@ static struct dep *goal_dep; All files start with considered == 0. */ static unsigned int considered = 0; +/* During processing we might drop some dependencies, which can't be freed + immediately because they are still in use. Remember them: this is mainly + to satisfy leak detectors. */ +static struct dep **dropped_list = NULL; +static size_t dropped_list_len = 0; +#define DROPPED_LIST_INCR 5 + static enum update_status update_file (struct file *file, unsigned int depth); static enum update_status update_file_1 (struct file *file, unsigned int depth); static enum update_status check_dep (struct file *file, unsigned int depth, @@ -608,15 +615,21 @@ update_file_1 (struct file *file, unsigned int depth) OSS (error, NILF, _("Circular %s <- %s dependency dropped."), file->name, d->file->name); - /* We cannot free D here because our the caller will still have - a reference to it when we were called recursively via - check_dep below. */ if (lastd == 0) file->deps = du->next; else lastd->next = du->next; du = du->next; + + /* We cannot free D here because our the caller will still have + a reference to it when we were called recursively via + check_dep below. */ + if (dropped_list_len % DROPPED_LIST_INCR == 0) + dropped_list = xrealloc (dropped_list, + sizeof (struct dep *) * (dropped_list_len + DROPPED_LIST_INCR)); + dropped_list[dropped_list_len++] = d; + continue; } diff --git a/tests/scripts/features/exec b/tests/scripts/features/exec index f139cf8c..d96d31c1 100644 --- a/tests/scripts/features/exec +++ b/tests/scripts/features/exec @@ -15,7 +15,11 @@ my $details = "The various shells that this test uses are the default" $port_type eq 'UNIX' or return -1; $^O =~ /cygwin/ and return -1; -my @shbangs = ('', '#!/bin/sh', "#!$perl_name"); +my @shbangs = ('#!/bin/sh', "#!$perl_name"); + +# The exec in Valgrind's VM doesn't allow starting commands without any shbang +$valgrind or push @shbangs, ''; + my @shells = ('', 'SHELL=/bin/sh'); # Try whatever shell the user has, as long as it's not a C shell. diff --git a/tests/scripts/features/jobserver b/tests/scripts/features/jobserver index a2f06ee8..ad5f9e7f 100644 --- a/tests/scripts/features/jobserver +++ b/tests/scripts/features/jobserver @@ -168,7 +168,8 @@ if ($port_type ne 'W32') { "all 1\nall 2\nrecurse"); } -if (exists $FEATURES{'jobserver-fifo'}) { +# We can't reset TMPDIR to something invalid when using valgrind +if (exists $FEATURES{'jobserver-fifo'} && !$valgrind) { # sv 62908. # Test that when mkfifo fails, make switches to pipe and succeeds. # Force mkfifo to fail by attempting to create a fifo in a non existent diff --git a/tests/scripts/features/output-sync b/tests/scripts/features/output-sync index 3353f1c6..779e24c8 100644 --- a/tests/scripts/features/output-sync +++ b/tests/scripts/features/output-sync @@ -380,17 +380,23 @@ unlink($fout); # Create a non-writable temporary directory. # Run the test twice, because run_make_test cannot match a regex against a # multiline input. -my $tdir = 'test_tmp_dir'; -mkdir($tdir, 0500); -$ENV{'TMPDIR'} = $tdir; -run_make_test(q! +# If we do this Valgrind fails because it cannot write temp files... the docs +# don't describe any way to tell valgrind to use a directory other than TMPDIR. + +if (!$valgrind) { + my $tdir = 'test_tmp_dir'; + mkdir($tdir, 0500); + $ENV{'TMPDIR'} = $tdir; + + run_make_test(q! all:; $(info hello, world) !, '-Orecurse', "/suppressing output-sync/"); -run_make_test(undef, '-Orecurse', "/#MAKE#: 'all' is up to date./"); + run_make_test(undef, '-Orecurse', "/#MAKE#: 'all' is up to date./"); -rmdir($tdir); + rmdir($tdir); +} } # This tells the test driver that the perl test script executed properly. diff --git a/tests/scripts/features/temp_stdin b/tests/scripts/features/temp_stdin index fee32a90..201dcb94 100644 --- a/tests/scripts/features/temp_stdin +++ b/tests/scripts/features/temp_stdin @@ -115,16 +115,21 @@ rmdir($tmakedir); # makefile from stdin to a temporary file. # Create a non-writable temporary directory. -my $tdir = 'test_tmp_dir'; -mkdir($tdir, 0500); -$ENV{'TMPDIR'} = $tdir; -close(STDIN); -open(STDIN, "<", 'input.mk') || die "$0: cannot open input.mk for reading: $!"; +# If we do this Valgrind fails because it cannot write temp files... the docs +# don't describe any way to tell valgrind to use a directory other than TMPDIR. -run_make_test(q! +if (!$valgrind) { + my $tdir = 'test_tmp_dir'; + mkdir($tdir, 0500); + $ENV{'TMPDIR'} = $tdir; + close(STDIN); + open(STDIN, "<", 'input.mk') || die "$0: cannot open input.mk for reading: $!"; + + run_make_test(q! all:; $(info hello, world) !, '-f-', '/cannot store makefile from stdin to a temporary file. Stop./', 512); -rmdir($tdir); + rmdir($tdir); +} } # This close MUST come at the end of the test!! diff --git a/tests/scripts/functions/guile b/tests/scripts/functions/guile index 120aaf3b..8c0012d9 100644 --- a/tests/scripts/functions/guile +++ b/tests/scripts/functions/guile @@ -21,7 +21,8 @@ $details = 'This only works on systems that support it.'; # If we don't have Guile support, never mind. exists $FEATURES{guile} or return -1; -# Guile and Valgrind don't play together at all. +# Guile and Valgrind/ASAN don't play together at all. +exists $FEATURES{sanitize} and return -1; $valgrind and return -1; # Verify simple data type conversions diff --git a/tests/scripts/functions/wildcard b/tests/scripts/functions/wildcard index f01f574a..1a1addda 100644 --- a/tests/scripts/functions/wildcard +++ b/tests/scripts/functions/wildcard @@ -150,8 +150,9 @@ if ($port_type ne 'W32' && eval { symlink("",""); 1 }) { # Test for dangling symlinks # This doesn't work with the built-in glob... needs to be updated! + # It also for some obscure reason, will break if we use ASAN!! - if (get_config('USE_SYSTEM_GLOB') eq 'yes') { + if (get_config('USE_SYSTEM_GLOB') eq 'yes' && !exists($FEATURES{sanitize})) { symlink($dir, $lnk); run_make_test(qq!all: ; \@echo \$(wildcard $lnk)!, '', "$lnk"); -- cgit v1.2.1