On 03/05/2021 15:13, TU Haoxin wrote:
Dear all.We are a team from Singapore Management University and we wrote a symbolic execution tool based on KLEE recently. After running the Coreutils packages(the newest released version, 8.32), we found some interesting issues(most are reported as "detected memory leaks" and one is reported "stack-overflow" by AddressSanitizer). Due to this is our first time reporting a potential bug in this community, we are not sure these bugs found by our tool are useful. Therefore, we don't want to waste your precious time, and here we just want to report one bug to you. If you confirm this is a real bug and could be helpful to you, we will file other issues then. Thank you very much! The following are instructions to help reproduce the issue: $ginstall "/" "/" "--" ./ginstall: omitting directory '/' ================================================================= ==2394==ERROR: LeakSanitizer: detected memory leaks Direct leak of 80 byte(s) in 1 object(s) allocated from: #0 0x492bcf in __interceptor_malloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:145 #1 0x4ef365 in hash_initialize ../lib/hash.c:605 #2 0x4e1362 in dest_info_init ../src/copy.c:1740 #3 0x4d98d7 in main ../src/install.c:1050 #4 0x7feef2e9abf6 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6) Indirect leak of 1264 byte(s) in 1 object(s) allocated from: #0 0x492d87 in __interceptor_calloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x50119b in rpl_calloc ../lib/calloc.c:65 #2 0x4ef45c in hash_initialize ../lib/hash.c:626 #3 0x4e1362 in dest_info_init ../src/copy.c:1740 #4 0x4d98d7 in main ../src/install.c:1050 #5 0x7feef2e9abf6 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6) SUMMARY: AddressSanitizer: 1344 byte(s) leaked in 2 allocation(s). $./ginstall --version install (GNU coreutils) 8.32 Copyright (C) 2020 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>. This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Written by David MacKenzie. $uname -a Linux jlx-super-server 5.4.0-72-generic #80~18.04.1-Ubuntu SMP Mon Apr 12 23:26:25 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux The configure option is "../configure --disable-nls CFLAGS="-fsanitize=address -static-libasan -g" CXXFLAGS="-fsanitize=address -static-libasan -g" By the way, may I ask if the issues reported by AddressSanitizer are really bugs worth reporting? I am not sure whether such a tool will report some false-positive cases. Thanks again for your time and help!
This isn't a real leak as the program terminates after return from main(). Now for asan/valgrind to be useful we should avoid these "definitely lost" cases. The attached does this for these hash structures at least, by consistently freeing them in dev mode (with "lint" defined). Note if we exit() from the main() functions, rather than using return, then valgrind recognizes the termination case, and does not flag this as definitely lost. The main() functions were changed from using exit() to return in cleanup associated with the support for the single binary build: https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=8defcee49 Perhaps we might consider adjusting that bacak, to something more general that valgrind may infer program termination from? Marking this as done, cheers, Pádraig
>From f4ad182a14f31537238542fc01fce59c26220f53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com> Date: Mon, 3 May 2021 18:11:04 +0100 Subject: [PATCH] maint: consistently free hash structures in dev mode Ensure we call hash_free() to avoid valgrind and leak_sanitizer "definitely lost" warnings. These were not real leaks as we terminate immediately after, but we should avoid these "definitely lost" warnings where possible. * src/copy.c: Add dest_info_free() and src_info_free(). * src/copy.h: Declare the above. * src/cp-hash.c: Don't define unless "lint" is defined. * src/install.c: Call dest_info_free() in dev mode. * src/mv.c: Likewise. * src/cp.c: Likewise. Also call src_info_free(). * src/ln.c: Call hash_free() in dev mode. * src/tail.c: Call hash_free() even if about to exit, in dev mode. Fixes https://bugs.gnu.org/48189 --- src/copy.c | 20 ++++++++++++++++++++ src/copy.h | 2 ++ src/cp-hash.c | 2 ++ src/cp.c | 5 +++++ src/install.c | 3 +++ src/ln.c | 6 ++++++ src/mv.c | 4 ++++ src/tail.c | 10 +++++++++- 8 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/copy.c b/src/copy.c index 3e1abee28..afd7974b1 100644 --- a/src/copy.c +++ b/src/copy.c @@ -1955,6 +1955,16 @@ dest_info_init (struct cp_options *x) triple_free); } +#ifdef lint +extern void +dest_info_free (struct cp_options *x) +{ + if (x->dest_info) + hash_free (x->dest_info); + x->dest_info = NULL; +} +#endif + /* Initialize the hash table implementing a set of F_triple entries corresponding to source files listed on the command line. */ extern void @@ -1977,6 +1987,16 @@ src_info_init (struct cp_options *x) triple_free); } +#ifdef lint +extern void +src_info_free (struct cp_options *x) +{ + if (x->src_info) + hash_free (x->src_info); + x->src_info = NULL; +} +#endif + /* When effecting a move (e.g., for mv(1)), and given the name DST_NAME of the destination and a corresponding stat buffer, DST_SB, return true if the logical 'move' operation should _not_ proceed. diff --git a/src/copy.h b/src/copy.h index 026530518..a97089137 100644 --- a/src/copy.h +++ b/src/copy.h @@ -300,7 +300,9 @@ extern bool set_file_security_ctx (char const *dst_name, bool recurse, const struct cp_options *x); void dest_info_init (struct cp_options *); +void dest_info_free (struct cp_options *); void src_info_init (struct cp_options *); +void src_info_free (struct cp_options *); void cp_options_default (struct cp_options *); bool chown_failure_ok (struct cp_options const *) _GL_ATTRIBUTE_PURE; diff --git a/src/cp-hash.c b/src/cp-hash.c index 423fcda23..1223c8aed 100644 --- a/src/cp-hash.c +++ b/src/cp-hash.c @@ -157,8 +157,10 @@ hash_init (void) /* Reset the hash structure in the global variable 'htab' to contain no entries. */ +#ifdef lint extern void forget_all (void) { hash_free (src_to_dest); } +#endif diff --git a/src/cp.c b/src/cp.c index a420c592b..c97a67563 100644 --- a/src/cp.c +++ b/src/cp.c @@ -730,6 +730,11 @@ do_copy (int n_files, char **file, char const *target_directory, free (dst_name); } + +#ifdef lint + dest_info_free (x); + src_info_free (x); +#endif } else /* !target_directory */ { diff --git a/src/install.c b/src/install.c index 8edf00f93..c9456fe5d 100644 --- a/src/install.c +++ b/src/install.c @@ -1017,6 +1017,9 @@ main (int argc, char **argv) if (! install_file_in_dir (file[i], target_directory, &x, i == 0 && mkdir_and_install)) exit_status = EXIT_FAILURE; +#ifdef lint + dest_info_free (&x); +#endif } } diff --git a/src/ln.c b/src/ln.c index 760bab25a..c7eb74026 100644 --- a/src/ln.c +++ b/src/ln.c @@ -680,6 +680,12 @@ main (int argc, char **argv) ok &= do_link (file[i], destdir_fd, dest_base, dest, -1); free (dest); } + +#ifdef lint + if (dest_set) + hash_free (dest_set); + dest_set = NULL; +#endif } else ok = do_link (file[0], AT_FDCWD, file[1], file[1], link_errno); diff --git a/src/mv.c b/src/mv.c index 3db5cc48d..7d81b59a7 100644 --- a/src/mv.c +++ b/src/mv.c @@ -503,6 +503,10 @@ main (int argc, char **argv) x.last_file = i + 1 == n_files; ok &= movefile (file[i], target_directory, true, &x); } + +#ifdef lint + dest_info_free (&x); +#endif } else { diff --git a/src/tail.c b/src/tail.c index 9095a18d8..ff567560d 100644 --- a/src/tail.c +++ b/src/tail.c @@ -1568,7 +1568,12 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files, return true; } if (follow_mode == Follow_descriptor && !found_watchable_file) - return false; + { +# ifdef lint + hash_free (wd_to_name); +# endif + return false; + } prev_fspec = &(f[n_files - 1]); @@ -1626,6 +1631,9 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files, && hash_get_n_entries (wd_to_name) == 0) { error (0, 0, _("no files remaining")); +# ifdef lint + hash_free (wd_to_name); +# endif return false; } -- 2.26.2
