Re: memory leaks in gnulib tests (for ASAN/LINT)
On 10/10/18 7:05 AM, Tim Rühsen wrote: Given Paul's statement "Besides, debugging tools come and go...": this means with 100 such tools we (or the people who care) would need to write and maintain 100 exception files. Exception files are not the only way to suppress false positives. It would be better to improve the debugging tools so that they don't have the false positives in the first place. That way, we don't need to maintain unnecessarily-tricky suppression files, and we don't need to maintain unnecessarily-tricky source code either (which is even worse). Long ago I became wary of "#ifdef lint" code because it means production code differs from the tested (or statically-analyzed) code. This wariness means that every time I look at such code there is mental overhead, which complicates and slows down maintenance. In some cases we really need to use such constructs despite their drawbacks, but this doesn't seem to be one of the cases. For example, it's reasonably safe for "#ifdef lint" code to substitute defined for undefined behavior to pacify a misguided static analyzer, but it's less safe for "#ifdef lint" to do the reverse (substitute undefined for defined behavior) It would be better to find out some way to write the tests so that they are idiomatic and yet don't cause this limited debugging tool to report a false alarm. For tests, always doing the 'free' may be the best we can do, as Bruno is suggesting. But really, it should be incumbent on the tool makers to have better tools, and not inflict their inadequacies on all the source code out there.
Re: memory leaks in gnulib tests (for ASAN/LINT)
Hi Assaf, > In this specific case, the fix is rather simple and costs very little. > would you consider it? Yes. But I don't like the '#ifdef lint' lines. Unit tests are not optimized for speed. Instead, I find it useful to not multiply the number of possible configurations of the tests (with and without -O2, in VPATH dir or not, now also with or without -Dlint, ...). May I remove these '#ifdef lint' and still commit it under your name? > With it, sed and sed's gnulib testsuite passes all tests under SASN, > which is a nice bonus Does sed's test suite also pass with NLS enabled, in a locale other than the "C" or an English locale, without harmless memory leaks? Bruno
Re: memory leaks in gnulib tests (for ASAN/LINT)
Hello all, You raise many good points, both for and against. In this specific case, the fix is rather simple and costs very little. would you consider it? With it, sed and sed's gnulib testsuite passes all tests under SASN, which is a nice bonus (and will also prevent false reports in the future). regards, - assaf >From 8769b66e1e05f5fdb46221f4a49cef58abaa0b0c Mon Sep 17 00:00:00 2001 From: Assaf Gordon Date: Wed, 10 Oct 2018 17:01:43 -0600 Subject: [PATCH] samacls tests: free memory under lint Avoids false-positives under ASAN and provides better usage examples. Initially reported by deltatau in https://bugs.gnu.org/32685 . Discussed in gnulib at https://lists.gnu.org/r/bug-gnulib/2018-10/msg00026.html . * tests/test-sameacls.c (main): Call free() and acl_free() as needed. --- tests/test-sameacls.c | 30 ++ 1 file changed, 30 insertions(+) diff --git a/tests/test-sameacls.c b/tests/test-sameacls.c index 51196e3a8..7f0ee6579 100644 --- a/tests/test-sameacls.c +++ b/tests/test-sameacls.c @@ -84,6 +84,10 @@ main (int argc, char *argv[]) fflush (stderr); abort (); } +#ifdef lint +free (contents1); +free (contents2); +#endif } /* Compare the access permissions of the two files, including ACLs. */ @@ -218,6 +222,12 @@ main (int argc, char *argv[]) return 1; } } +#ifdef lint +acl_free (acl1); +acl_free (acl2); +acl_free (text1); +acl_free (text2); +#endif } #elif HAVE_FACL && defined GETACL /* Solaris, Cygwin, not HP-UX */ int count1; @@ -287,6 +297,10 @@ main (int argc, char *argv[]) return 1; } } +#ifdef lint + free (entries1); + free (entries2); +#endif } # ifdef ACE_GETACL count1 = acl (file1, ACE_GETACLCNT, 0, NULL); @@ -366,6 +380,10 @@ main (int argc, char *argv[]) return 1; } } +#ifdef lint +free (entries1); +free (entries2); +#endif } # endif #elif HAVE_GETACL /* HP-UX */ @@ -438,6 +456,10 @@ main (int argc, char *argv[]) return 1; } } +#ifdef lint + free (entries1); + free (entries2); +#endif } # if HAVE_ACLV_H /* HP-UX >= 11.11 */ @@ -511,6 +533,10 @@ main (int argc, char *argv[]) return 1; } } +#ifdef lint + free (entries1); + free (entries2); +#endif } # endif #elif HAVE_ACLX_GET /* AIX */ @@ -687,6 +713,10 @@ main (int argc, char *argv[]) return 1; } } +#ifdef lint + free (entries1); + free (entries2); +#endif } #endif } -- 2.11.0
Re: memory leaks in gnulib tests (for ASAN/LINT)
Hi Tim, There is no way to avoid not-serious memory leaks in the long run. 1) There are API functions like bindtextdomain(), textdomain() which are supposed to keep information until the process terminates. 2) There are certain tasks like reading an initialization file (think of ~/.wgetrc), that are also keep the allocated settings in memory until the end of the process. 3) There are caches. Glibc has some caches (e.g. in gettext()). Gnulib has some caches (e.g. in localename). Are there APIs to undo these memory allocations? reset_textdomain? free_all_settings? clear_gettext_cache? No, because when you know that the OS cleans up the entire process space anyway, programmers don't want to spend time creating such APIs. (Programmers for embedded devices without an OS might have different habits. But that's not the target of Gnulib nor of GNU.) So, these not-serious memory leaks will stay. Do you think 'wget' testing with valgrind/ASAN should be limited to situations without i18n and without initialization file? I would think it's essential to test 'wget' also with i18n and with an initialization file, since many users will use it this way. > onto *manual* separating the serious from the harmless reports. I encourage you to find automated ways to separate the serious from the harmless memory leaks. One approach is repetition. See tests/test-fprintf-posix3.c for an example. The other approach I am aware of are suppression files. > with 100 such tools we (or the people who care) would need to > write and maintain 100 exception files. This is simply not reasonable. Then you (or the people who care) should make an effort to standardize the suppressions file format. So that a suppression created for valgrind can be reused with ASAN, and vice versa. > Even if no one here has time to do so, we should encourage people like > Assaf to at least report these issues I welcome and encourage all reports about valgrind/ASAN findings regarding invalid or unaligned pointer accesses, uninitialized memory or conditions, and serious memory leaks. Bruno
Re: memory leaks in gnulib tests (for ASAN/LINT)
On 10/10/18 11:10 AM, Bernhard Voelker wrote: > On 10/9/18 7:37 PM, Paul Eggert wrote: >> If we want to silence these diagnostics we can either complicate the source >> code or complicate the debugging tool. > > I'm fine with either. Myy only concern is that code from the gnulib tests > might > be taken as basis for application code. Still, it's the repsonsibility of the > author of such an application, yet a little "IF_LINT(free)" could be a helpful > hint. Right, test code is often taken as example code, especially from newbies. And even if these people find + fix those (bugs|flaws|issues), they will run around telling the world how bad gnulib's code base is. I know the code base is superb thanks to truly capable and engaged maintainers... but rumors are hard to stop. We already have enough jokes about the "old quirky GNU neckbeards". Given Paul's statement "Besides, debugging tools come and go...": this means with 100 such tools we (or the people who care) would need to write and maintain 100 exception files. This is simply not reasonable. Instead we should think about fixing those issues at the source - just because there is more than just one leak finder. Staying with the current state puts endless *manual* work onto either maintaining the exceptions/suppressions *or* onto *manual* separating the serious from the harmless reports. Given that people-power is becoming rarer and rarer the goal must be automated tests+reporting without the need for maintaining these exceptions/suppressions. IMO the most reasonable way is to squeeze all leaks out of the (test) source code. This is manual work just one time and makes tools, that might come and go, happy forever (ideally, of course). Even if no one here has time to do so, we should encourage people like Assaf to at least report these issues (maybe there is already a list of known issues !?). And we should encourage people to come up with patches/suggestions and not just say "we don't want to fix those". Regards, Tim
Re: memory leaks in gnulib tests (for ASAN/LINT)
On 10/9/18 7:37 PM, Paul Eggert wrote: > If we want to silence these diagnostics we can either complicate the source > code or complicate the debugging tool. I'm fine with either. Myy only concern is that code from the gnulib tests might be taken as basis for application code. Still, it's the repsonsibility of the author of such an application, yet a little "IF_LINT(free)" could be a helpful hint. Have a nice day, Berny
Re: memory leaks in gnulib tests (for ASAN/LINT)
Tim Rühsen wrote: The GNU standards [1] say so as well: "Memory analysis tools such as valgrind can be useful, but don’t complicate a program merely to avoid their false alarms. For example, if memory is used until just before a process exits, don’t free it simply to silence such a tool." This is wrong and ignorant in several ways. It really hurts. Sorry that it hurts, but any guideline or policy in this area will step on *somebody's* toes. And this guideline wasn't chosen out of ignorance. If we want to silence these diagnostics we can either complicate the source code or complicate the debugging tool. In cases like these I'd rather complicate the debugging tool, as that is easier to maintain than adding complexity to all the source-code programs that the tool is applied to. Besides, debugging tools come and go, but source code lasts forever.
Re: memory leaks in gnulib tests (for ASAN/LINT)
On 10/9/18 5:22 PM, Bruno Haible wrote: >> I don't intend to do anything about the not serious ones. > > The GNU standards [1] say so as well: > "Memory analysis tools such as valgrind can be useful, but don’t complicate >a program merely to avoid their false alarms. For example, if memory is > used >until just before a process exits, don’t free it simply to silence such a > tool." This is wrong and ignorant in several ways. It really hurts. Regards, Tim signature.asc Description: OpenPGP digital signature
Re: memory leaks in gnulib tests (for ASAN/LINT)
> I don't intend to do anything about the not serious ones. The GNU standards [1] say so as well: "Memory analysis tools such as valgrind can be useful, but don’t complicate a program merely to avoid their false alarms. For example, if memory is used until just before a process exits, don’t free it simply to silence such a tool." [1] https://www.gnu.org/prep/standards/html_node/Memory-Usage.html#index-memory-leak
Re: memory leaks in gnulib tests (for ASAN/LINT)
Hello Assaf > It is a long tradition not to bother freeing memory in such cases Yes. I remind the distinction between "not serious" and "serious" memory leaks (as defined in https://en.wikipedia.org/wiki/Memory_leak ), and I don't intend to do anything about the not serious ones. Why? 1) Because there are cases where one-time allocation of memory is normal. For example the functions textdomain() and bindtextdomain() do allocate memory that stays allocated until the application ends. This is not a bug, because it does not lead to a serious memory leak. 2) In Gnulib we want to have a large number of unit tests. To achieve this, we should not make it too annoying to create unit tests. Therefore some sloppiness is allowed in unit tests that is not allowed in library code. > to help more automated/ASAN testing? It is the duty of the reporters to report as a bug only serious memory leaks. If you want to have automated testing against serious memory leaks, you need to distinguish not serious and serious memory leaks. For example, by passing --suppression options to valgrind, or by running test cases in a loop that repeats them a 1000 times. Bruno
memory leaks in gnulib tests (for ASAN/LINT)
Hello gnulib, 'deltatau' recently reported 3 memory leaks detected in gnulib's test in sed [1]. This was found using our recent ASAN build feature [2]. [1] https://bugs.gnu.org/32685 [2] https://git.savannah.gnu.org/cgit/sed.git/commit/?id=0cc0ade4237b8db69f5253c3d933f9890b698f4d It is a long tradition not to bother freeing memory in such cases, but perhaps it's worth considering IF_LINT(free(...)) to help more automated/ASAN testing? Details below. regards, - assaf FAIL: test-copy-acl.sh == + test 1 = 0 + func_tmpdir + : /tmp + tmp=/tmp/glg11kSA + test -n /tmp/glg11kSA + test -d /tmp/glg11kSA ++ pwd + builddir=/home/ksk/delete/sed-build-asan/gnulib-tests + cd /home/ksk/delete/sed-build-asan/gnulib-tests + cd /tmp/glg11kSA + rm -f 'tmpfile[0-9]' 'tmpaclout[0-2]' + echo 'Simple contents' + chmod 600 tmpfile0 + acl_flavor=none + acl_flavor=linux + case $acl_flavor in + func_test_copy tmpfile0 tmpfile1 + echo 'Simple contents' + chmod 600 tmpfile1 + /home/ksk/delete/sed-build-asan/gnulib-tests/test-copy-acl tmpfile0 tmpfile1 + /home/ksk/delete/sed-build-asan/gnulib-tests/test-sameacls tmpfile0 tmpfile1 = ==26800==ERROR: LeakSanitizer: detected memory leaks Direct leak of 108 byte(s) in 2 object(s) allocated from: #0 0x7fa615dbe019 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:86 #1 0x7fa615ac851b (/usr/lib/libacl.so.1+0x451b) #2 0x7fa615924222 in __libc_start_main (/usr/lib/libc.so.6+0x24222) Direct leak of 17 byte(s) in 1 object(s) allocated from: #0 0x7fa615dbe019 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:86 #1 0x563664a51531 in fread_file /home/ksk/delete/sed-build-asan/gnulib-tests/read-file.c:73 #2 0x563664a51908 in internal_read_file /home/ksk/delete/sed-build-asan/gnulib-tests/read-file.c:147 #3 0x563664a51a36 in read_file /home/ksk/delete/sed-build-asan/gnulib-tests/read-file.c:174 #4 0x563664a50545 in main /home/ksk/delete/sed-build-asan/gnulib-tests/test-sameacls.c:58 #5 0x7fa615924222 in __libc_start_main (/usr/lib/libc.so.6+0x24222) [...]
