[PATCH 0/6] pull: almost working AddressSanitizer support

[PATCH 0/6] pull: almost working AddressSanitizer support

[Sami Kerola]

Hello,

As said some time ago

http://markmail.org/message/xfd6u2s53jlsjoc6

the more static analysis the better.  So I to heard about AddressSanitizer
I thought why not to give it a go, to see if making it work would benefit
this project.  This patch set fixes one real issue '4/6: libmount: fix
memory overflow' and makes some noise to go away.  I am hoping to get the
AddressSanitizer to run without issues so that it could be part of travis
clang root build.

But it seems nothing is never that easy.  For some reason the mkswap(8)
refuses to work when it is build with

export CC=clang
export CFLAGS='-O1 -g -fsanitize=address -fno-omit-frame-pointer'
export ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer
export LD=ld.gold

The short description is: when the code is at very last line of mkswap at
'return EXIT_SUCCESS;' somehow the EXIT_SUCCESS is 1.  If I change the
'return EXIT_SUCCESS;' to 'exit(EXIT_SUCCESS);' the return value stays
expected.  Possible explanations include:

1. I'm doing something silly and/or wrong.

2. This happens only on my laptop (or imagination).  Please, let it not
   be this.

3. This is a bug in clang, llvm, and/or AddressSanitizer.
   clang / llvm version 3.5.0 from Archlinux packages 3.5.0-2.1

4. With gcc 4.9.2 and the same CFLAGS the mkswap works.

Adding a AddressSanitizer travis build rules while having this sort of
problem going on does not sound right thing to do.  Getting a hint what
might be going on is highly appreciated.


----------------------------------------------------------------
The following changes since commit 9a83b03c7307715b45b143f203f9bf8b0bcf7a5d:
  mkswap: make final report more human readable (2014-11-07 14:03:35 +0100)
are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git asan
for you to fetch changes up to d07a5b79ec55bffceb6db994fc5a0ddadd8a1ca5:
  tests: mark python libmount tests known to fail with AddressSanitizer (2014-11-10 20:02:53 +0000)
----------------------------------------------------------------

Sami Kerola (6):
  build-sys: make scanf_cv_alloc_modifier to work [AddressSanitizer]
  tests: fix memory leak [AddressSanitizer]
  tests: add mkswap size printout to expected output
  libmount: fix memory overflow [AddressSanitizer]
  tests: skip kill -SEGV test when running AddressSanitizer
  tests: mark python libmount tests known to fail with AddressSanitizer

 configure.ac                       | 1 +
 lib/cpuset.c                       | 1 +
 libmount/src/utils.c               | 2 +-
 tests/expected/misc/swaplabel      | 2 +-
 tests/ts/kill/name_to_number       | 3 +++
 tests/ts/libmount/tabfiles-py      | 4 ++++
 tests/ts/libmount/tabfiles-tags    | 4 ++++
 tests/ts/libmount/tabfiles-tags-py | 4 ++++
 tests/ts/libmount/update-py        | 6 +++++-
 9 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.1.3

[PATCH 1/6] build-sys: make scanf_cv_alloc_modifier to work [AddressSanitizer]

[Sami Kerola]

The tests failed with following message in config.log

ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2 byte(s) in 1 object(s) allocated from:
    #0 0x49a40e in realloc (/home/src/util-linux/conftest+0x49a40e)
    #1 0x7fbe48633e69 in __GI__IO_vfscanf (/usr/lib/libc.so.6+0x56e69)
    #2 0x7fbe48649786 in _IO_vsscanf (/usr/lib/libc.so.6+0x6c786)

which knocked out libmount from build, and commands depending on it.

The reason this change makes sense is that AddressSanitizer seems like a
good addition to set of tools that util-linux package can use, when and
if needed.

Reference: https://code.google.com/p/address-sanitizer/wiki/AddressSanitizer
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 configure.ac | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure.ac b/configure.ac
index 1336f8d..b8a6627 100644
--- a/configure.ac
+++ b/configure.ac
@@ -461,6 +461,7 @@ int main()
 	int i;
 	char *s;
 	i = sscanf("x", $1, &s);
+	free(s);
 	if (i == 1)
 		return 0;
 	return 1;
-- 
2.1.3

[PATCH 2/6] tests: fix memory leak [AddressSanitizer]

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 lib/cpuset.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/cpuset.c b/lib/cpuset.c
index e5b6b9d..d715720 100644
--- a/lib/cpuset.c
+++ b/lib/cpuset.c
@@ -388,6 +388,7 @@ int main(int argc, char *argv[])
 	printf("[%s]\n", cpulist_create(buf, buflen, set, setsize));
 
 	free(buf);
+	free(mask);
 	free(range);
 	cpuset_free(set);
 
-- 
2.1.3

[PATCH 3/6] tests: add mkswap size printout to expected output

[Sami Kerola]

The commit 9a83b03c changed mkswap final printout, that made test to
fail.  This change updates test expectation with mentioned commit.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/expected/misc/swaplabel | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/expected/misc/swaplabel b/tests/expected/misc/swaplabel
index fd442af..d54d559 100644
--- a/tests/expected/misc/swaplabel
+++ b/tests/expected/misc/swaplabel
@@ -1,6 +1,6 @@
 mkswap: error: swap area needs to be at least 10 pages
 mkswap: Label was truncated.
-Setting up swapspace version 1, size = 9 pages
+Setting up swapspace version 1, size = 9 pages (36864 bytes)
 LABEL=1234567890abcde, UUID=12345678-abcd-abcd-abcd-1234567890ab
 LABEL: 1234567890abcde
 UUID:  12345678-abcd-abcd-abcd-1234567890ab
-- 
2.1.3

[PATCH 4/6] libmount: fix memory overflow [AddressSanitizer]

[Sami Kerola]

==10918==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffd795b680 at pc 0x0000004447c6 bp 0x7fffd795b3e0 sp 0x7fffd795ab78
WRITE of size 129 at 0x7fffd795b680 thread T0
    #0 0x4447c5 in scanf_common(void*, int, bool, char const*, __va_list_tag*) (/home/src/util-linux/.libs/lt-mount+0x4447c5)
    #1 0x445892 in sscanf (/home/src/util-linux/.libs/lt-mount+0x445892)
    #2 0x7fe78709a3d3 in get_filesystems /home/src/util-linux/libmount/src/utils.c:581:7
    #3 0x7fe78709a1ba in mnt_get_filesystems /home/src/util-linux/libmount/src/utils.c:622:7
    #4 0x7fe7870aa78f in do_mount_by_pattern /home/src/util-linux/libmount/src/context_mount.c:833:7
    #5 0x7fe7870a9534 in mnt_context_do_mount /home/src/util-linux/libmount/src/context_mount.c:951:9
    #6 0x7fe7870aab2b in mnt_context_mount /home/src/util-linux/libmount/src/context_mount.c:1051:8
    #7 0x4ba9f5 in main /home/src/util-linux/sys-utils/mount.c:1107:7
    #8 0x7fe785caa03f in __libc_start_main (/usr/lib/libc.so.6+0x2003f)
    #9 0x4b9f9c in _start (/home/src/util-linux/.libs/lt-mount+0x4b9f9c)

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 libmount/src/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmount/src/utils.c b/libmount/src/utils.c
index 456a898..33e2c5b 100644
--- a/libmount/src/utils.c
+++ b/libmount/src/utils.c
@@ -578,7 +578,7 @@ static int get_filesystems(const char *filename, char ***filesystems, const char
 
 		if (*line == '#' || strncmp(line, "nodev", 5) == 0)
 			continue;
-		if (sscanf(line, " %128[^\n ]\n", name) != 1)
+		if (sscanf(line, " %" stringify(sizeof(line) - 1)  "[^\n ]\n", name) != 1)
 			continue;
 		if (strcmp(name, "*") == 0) {
 			rc = 1;
-- 
2.1.3

[PATCH 5/6] tests: skip kill -SEGV test when running AddressSanitizer

[Sami Kerola]

Sending signal indicating invalid memory reference makes AddressSanitizer
to report false positive test failure.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/ts/kill/name_to_number | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/ts/kill/name_to_number b/tests/ts/kill/name_to_number
index 9a370bb..cde55c9 100755
--- a/tests/ts/kill/name_to_number
+++ b/tests/ts/kill/name_to_number
@@ -33,6 +33,9 @@ for SIG in $($TS_CMD_KILL -L); do
 		EXPECTED=$SIG
 		continue
 	fi
+	if [ -f "$ASAN_SYMBOLIZER_PATH" ] && [ "x$SIG" = "xSEGV" ]; then
+		continue
+	fi
 	if [ "x$SIG" = "xSTOP" ] || [ "x$SIG" = "xKILL" ]; then
 		continue
 	fi
-- 
2.1.3

[PATCH 6/6] tests: mark python libmount tests known to fail with AddressSanitizer

[Sami Kerola]

The tests fail with an error similar to this.

Traceback (most recent call last):
  File "/home/src/util-linux/libmount/python/test_mount_tab_update.py", line 7, in <module>
    import pylibmount as mnt
ImportError: /home/src/util-linux/.libs/libuuid.so.1: undefined symbol: __asan_option_detect_stack_use_after_return

It might be possible to build ASAN-DSO and set LD_PRELOAD, but this
solution is not officially supported.  See the reference for details.

Reference: https://code.google.com/p/address-sanitizer/wiki/AsanAsDso
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 tests/ts/libmount/tabfiles-py      | 4 ++++
 tests/ts/libmount/tabfiles-tags    | 4 ++++
 tests/ts/libmount/tabfiles-tags-py | 4 ++++
 tests/ts/libmount/update-py        | 6 +++++-
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/tests/ts/libmount/tabfiles-py b/tests/ts/libmount/tabfiles-py
index f0c4836..b157460 100755
--- a/tests/ts/libmount/tabfiles-py
+++ b/tests/ts/libmount/tabfiles-py
@@ -9,6 +9,10 @@ TS_DESC="tab files-py"
 ts_init "$*"
 ts_init_py libmount
 
+if [ -f "$ASAN_SYMBOLIZER_PATH" ]; then
+	TS_KNOWN_FAIL="yes"
+fi
+
 PYDBG="$PYTHON -m pdb"
 TESTPROG="$TS_HELPER_PYLIBMOUNT_TAB"
 [ -x $TESTPROG ] || ts_die "test script missing"
diff --git a/tests/ts/libmount/tabfiles-tags b/tests/ts/libmount/tabfiles-tags
index 1d9534b..15696b0 100755
--- a/tests/ts/libmount/tabfiles-tags
+++ b/tests/ts/libmount/tabfiles-tags
@@ -7,6 +7,10 @@ TS_DESC="tags"
 ts_init "$*"
 ts_skip_nonroot
 
+if [ -f "$ASAN_SYMBOLIZER_PATH" ]; then
+	TS_KNOWN_FAIL="yes"
+fi
+
 TESTPROG="$TS_HELPER_LIBMOUNT_TAB"
 
 [ -x $TESTPROG ] || ts_skip "test not compiled"
diff --git a/tests/ts/libmount/tabfiles-tags-py b/tests/ts/libmount/tabfiles-tags-py
index 2f462a0..d6ca470 100755
--- a/tests/ts/libmount/tabfiles-tags-py
+++ b/tests/ts/libmount/tabfiles-tags-py
@@ -8,6 +8,10 @@ ts_init "$*"
 ts_init_py libmount
 ts_skip_nonroot
 
+if [ -f "$ASAN_SYMBOLIZER_PATH" ]; then
+	TS_KNOWN_FAIL="yes"
+fi
+
 TESTPROG="$TS_HELPER_PYLIBMOUNT_TAB"
 [ -x $TESTPROG ] || ts_die "test script missing"
 
diff --git a/tests/ts/libmount/update-py b/tests/ts/libmount/update-py
index 6493224..66f1754 100755
--- a/tests/ts/libmount/update-py
+++ b/tests/ts/libmount/update-py
@@ -10,6 +10,10 @@ ts_init "$*"
 ts_init_py libmount
 ts_skip_nonroot
 
+if [ -f "$ASAN_SYMBOLIZER_PATH" ]; then
+	TS_KNOWN_FAIL="yes"
+fi
+
 TESTPROG="$TS_HELPER_PYLIBMOUNT_UPDATE"
 [ -x $TESTPROG ] || ts_die "test script missing"
 
@@ -21,7 +25,7 @@ rm -f $LIBMOUNT_FSTAB
 cp "$TS_SELF/files/fstab.comment" $LIBMOUNT_FSTAB
 
 ts_init_subtest "fstab-replace"
-$PYTHON $TESTPROG --replace "LABEL=foo" "/mnt/foo"
+$PYTHON $TESTPROG --replace "LABEL=foo" "/mnt/foo" >/dev/null 2>&1
 cp $LIBMOUNT_FSTAB $TS_OUTPUT	# save the fstab aside
 ts_finalize_subtest		#checks the fstab
 
-- 
2.1.3

Re: [PATCH 1/6] build-sys: make scanf_cv_alloc_modifier to work [AddressSanitizer]

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 221 bytes --]

On 10 Nov 2014 21:49, Sami Kerola wrote:
> The tests failed with following message in config.log
> 
> ERROR: LeakSanitizer: detected memory leaks

nit: the subject says ASAN but this is fixing an LSAN bug ;)
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/6] tests: mark python libmount tests known to fail with AddressSanitizer

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

On 10 Nov 2014 21:49, Sami Kerola wrote:
> The tests fail with an error similar to this.
> 
> Traceback (most recent call last):
>   File "/home/src/util-linux/libmount/python/test_mount_tab_update.py", line 7, in <module>
>     import pylibmount as mnt
> ImportError: /home/src/util-linux/.libs/libuuid.so.1: undefined symbol: __asan_option_detect_stack_use_after_return

if libuuid.so is built with ASAN, it should be linked with it too.  that means 
passing the ASAN flags at link time.  or maybe libtool is filtering them out 
when it goes to link the library ?
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Boris Egorov]

Hello,

On 11/11/2014 03:49 AM, Sami Kerola wrote:

> The short description is: when the code is at very last line of mkswap at
> 'return EXIT_SUCCESS;' somehow the EXIT_SUCCESS is 1.  If I change the
> 'return EXIT_SUCCESS;' to 'exit(EXIT_SUCCESS);' the return value stays
> expected.  Possible explanations include:
That's interesting.

> 1. I'm doing something silly and/or wrong.
I doubt it.

> 2. This happens only on my laptop (or imagination).  Please, let it not
>     be this.
No, I can confirm: It works as you describe with clang-3.5.0 from Debian 
Jessie. Returns 1 with ld.bfd too.

> 3. This is a bug in clang, llvm, and/or AddressSanitizer.
>     clang / llvm version 3.5.0 from Archlinux packages 3.5.0-2.1
Probably it is.

> 4. With gcc 4.9.2 and the same CFLAGS the mkswap works.
Confirmed with gcc 4.9.1.

I found more info: problem connected with atexit(close_stdout). If I 
comment this line, mkswap exits normally. Looks like close_stdout ends 
successfully (I've made it not inline and checked it with gdb), but then 
program exits with code 1.

Also, there is a problem building mkswap with clang on my machine. 
autoconf test for HAVE_SCANF_MS_MODIFIER is incomplete. clang/llvm have 
%m modifier (I've checked it), but this test fails to detect it. Test 
for HAVE_SCANF_AS_MODIFIER fails too, I suppose. So when I build mkswap 
I have an error:

     lib/colors.c:620:19: error: use of undeclared identifier 'UL_SCNsA'
                     rc = sscanf(p,  UL_SCNsA" "     /* name */

So, we need to detect %m modifier with clang on build somehow.

PS. Forgot to add mail list on reply, resending message. Sorry for the 
duplicate, Sami.

-- 
Best Regards,
Boris Egorov

Re: [PATCH 6/6] tests: mark python libmount tests known to fail with AddressSanitizer

[Sami Kerola]

On 11 November 2014 02:12, Mike Frysinger <vapier@gentoo.org> wrote:
> On 10 Nov 2014 21:49, Sami Kerola wrote:
>> The tests fail with an error similar to this.
>>
>> Traceback (most recent call last):
>>   File "/home/src/util-linux/libmount/python/test_mount_tab_update.py", line 7, in <module>
>>     import pylibmount as mnt
>> ImportError: /home/src/util-linux/.libs/libuuid.so.1: undefined symbol: __asan_option_detect_stack_use_after_return
>
> if libuuid.so is built with ASAN, it should be linked with it too.  that means
> passing the ASAN flags at link time.  or maybe libtool is filtering them out
> when it goes to link the library ?

Hi Mike,

The documentation

https://code.google.com/p/address-sanitizer/wiki/AsanAsDso

gives a hint

-- snip
=ASAN and LD_PRELOAD=

It is possible to use ASAN in this scenario (e.g. JVM+JNI, Python+SWIG):

* There is third-party executable binary which can not be recompiled
* It loads shared libraries that can be recompiled and we want to test
them with ASAN

A simple solution is to build ASAN-DSO and LD_PRELOAD it into the
process, however the devil is in the detail.

* If the process creates sub-processes, they will also have ASAN-DSO
preloaded, which may be undesirable.
* TODO (stay tuned)
-- snip

Making the python + libuuid should be possible, but I it seems to
require a bit trying to get work. I feel it is acceptable to get most
of the util-linux & travis to work with the asan first, and once the
easy stuff is done one can/should concentrate on making the less
trivial things to work.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Sami Kerola]

On 11 November 2014 07:17, Boris Egorov <egorov@linux.com> wrote:
> On 11/11/2014 03:49 AM, Sami Kerola wrote:
>
>> The short description is: when the code is at very last line of mkswap at
>> 'return EXIT_SUCCESS;' somehow the EXIT_SUCCESS is 1.  If I change the
>> 'return EXIT_SUCCESS;' to 'exit(EXIT_SUCCESS);' the return value stays
>> expected.  Possible explanations include:
>
> That's interesting.
>
>> 1. I'm doing something silly and/or wrong.
>
> I doubt it.
>
>> 2. This happens only on my laptop (or imagination).  Please, let it not
>>     be this.
>
> No, I can confirm: It works as you describe with clang-3.5.0 from Debian
> Jessie. Returns 1 with ld.bfd too.

Hi Boris,

Excellent to hear the issue is reproducible.

>> 3. This is a bug in clang, llvm, and/or AddressSanitizer.
>>     clang / llvm version 3.5.0 from Archlinux packages 3.5.0-2.1
>
> Probably it is.
>
>> 4. With gcc 4.9.2 and the same CFLAGS the mkswap works.
>
> Confirmed with gcc 4.9.1.
>
> I found more info: problem connected with atexit(close_stdout). If I comment
> this line, mkswap exits normally. Looks like close_stdout ends successfully
> (I've made it not inline and checked it with gdb), but then program exits
> with code 1.

Good idea. Removal of the atexit(close_stdout) made a leak to appear.
In wipe_device() the 'char *type' leaked. Unfortunately this
correction did nothing to exit value issue.

It's time to get a core.

$ ASAN_OPTIONS=abort_on_error=1 ./mkswap test ; echo $?
lt-mkswap: test: warning: wiping old swap signature.
Setting up swapspace version 1, size = 6.5 MiB (6819840 bytes)
no label, UUID=0cd79d6a-4abc-4ce8-951a-308506e5a09e
Aborted (core dumped)
134

Almost good news.

Nov 11 21:55:08 kerolasa-home systemd-coredump[4941]: Coredump of 4921
(lt-mkswap) is larger than configured processing limit, refusing.
Nov 11 21:55:09 kerolasa-home systemd-coredump[4941]: Process 4921
(lt-mkswap) of user 1000 dumped core.

If I'm reading code right the core has to be more than 2 gigabytes
before that happens.

src/journal/coredump.c:#define PROCESS_SIZE_MAX ((off_t)
(2LLU*1024LLU*1024LLU*1024LLU))

I'm running out of ideas.

> Also, there is a problem building mkswap with clang on my machine. autoconf
> test for HAVE_SCANF_MS_MODIFIER is incomplete. clang/llvm have %m modifier
> (I've checked it), but this test fails to detect it. Test for
> HAVE_SCANF_AS_MODIFIER fails too, I suppose. So when I build mkswap I have
> an error:
>
>     lib/colors.c:620:19: error: use of undeclared identifier 'UL_SCNsA'
>                     rc = sscanf(p,  UL_SCNsA" "     /* name */
>
> So, we need to detect %m modifier with clang on build somehow.

The scanf modifier detection fix is the first change in this  pull request.

http://marc.info/?l=util-linux-ng&m=141565620610774&w=2

> PS. Forgot to add mail list on reply, resending message. Sorry for the
> duplicate, Sami.

NP.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Sami Kerola]

On Tue, 11 Nov 2014, Sami Kerola wrote:
> I'm running out of ideas.

That was a bit hasty comment. Without '-fsanitize=address' compiler 
option, while keepign '-O0 -g -ggdb' valgrind was able to display another 
leak.  When both leaks were fixed the mkswap started to return zero 
expected way.

The second leak was caused by size_to_human_string() that is coming from 
libcommon.la. Did the ASAN + clang fall over because of libtool static 
library and atexit()? I don't know, and I guess this maillist is wrong 
forum to investigate. I'm just happy if someone who has similar issue 
finds this thread useful.

--->8----
From: Sami Kerola <kerolasa@iki.fi>
Date: Tue, 11 Nov 2014 21:34:30 +0000
Subject: [PATCH v2] mkswap: remove memory leaks [LeakSanitizer] [valgrind]

==18922==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8 byte(s) in 1 object(s) allocated from:
     #0 0x49d12b in __interceptor_malloc (/home/src/util-linux/.libs/lt-mkswap+0x49d12b)
     #1 0x7faf2a5069c9 in __GI___strdup (/usr/lib/libc.so.6+0x819c9)
     #2 0xffff96e7e33 (<unknown module>)

SUMMARY: AddressSanitizer: 8 byte(s) leaked in 1 allocation(s).

And another one that valgrind found.

==6316== 8 bytes in 1 blocks are definitely lost in loss record 1 of 1
==6316==    at 0x4C29F90: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6316==    by 0x5E3F9C9: strdup (in /usr/lib/libc-2.20.so)
==6316==    by 0x43A25F: size_to_human_string (strutils.c:495)
==6316==    by 0x42B35C: main (mkswap.c:488)

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
  disk-utils/mkswap.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/disk-utils/mkswap.c b/disk-utils/mkswap.c
index c4fe8c9..d34e2fd 100644
--- a/disk-utils/mkswap.c
+++ b/disk-utils/mkswap.c
@@ -323,6 +323,7 @@ static void wipe_device(struct mkswap_control *ctl)
  			fprintf(stderr, _("        (compiled without libblkid). "));
  		fprintf(stderr, _("Use -f to force.\n"));
  	}
+	free(type);
  #ifdef HAVE_LIBBLKID
  	blkid_free_probe(pr);
  #endif
@@ -488,6 +489,7 @@ int main(int argc, char **argv)

  	printf(_("Setting up swapspace version %d, size = %s (%ju bytes)\n"),
  		version, strsz, sz);
+	free(strsz);

  	set_signature(&ctl);
  	set_uuid_and_label(&ctl);
-- 
2.1.3

Re: [PATCH 1/6] build-sys: make scanf_cv_alloc_modifier to work [AddressSanitizer]

[Sami Kerola]

On 11 November 2014 02:10, Mike Frysinger <vapier@gentoo.org> wrote:
> On 10 Nov 2014 21:49, Sami Kerola wrote:
>> The tests failed with following message in config.log
>>
>> ERROR: LeakSanitizer: detected memory leaks
>
> nit: the subject says ASAN but this is fixing an LSAN bug ;)

Thanks Mike,

Message is reworded, and I added you as Reviewed-by.

https://github.com/kerolasa/lelux-utiliteetit/commit/b29fbfdb3a83d6dd1215280c24422150baa8b477

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Karel Zak]

On Tue, Nov 11, 2014 at 10:49:02PM +0000, Sami Kerola wrote:
>  	printf(_("Setting up swapspace version %d, size = %s (%ju bytes)\n"),
>  		version, strsz, sz);
> +	free(strsz);

 free-before-exit, you know what I think about it, right? :)

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Sami Kerola]

On 12 November 2014 14:25, Karel Zak <kzak@redhat.com> wrote:
> On Tue, Nov 11, 2014 at 10:49:02PM +0000, Sami Kerola wrote:
>>       printf(_("Setting up swapspace version %d, size = %s (%ju bytes)\n"),
>>               version, strsz, sz);
>> +     free(strsz);
>
>  free-before-exit, you know what I think about it, right? :)

Normally I would agree, but this case is bizarre. The mkswap will
return 1 when compiled with clang -fsanitize=address without any
message what is wrong. That's probably clang bug, which is a different
topic all together. Adding the free() calls seems to make the
immediate tests failure issue to go away, that is needed to automate
sanitize. In case travis && address sanitation is not needed at all
then these changes does not make sense.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

On 12 Nov 2014 15:02, Sami Kerola wrote:
> On 12 November 2014 14:25, Karel Zak <kzak@redhat.com> wrote:
> > On Tue, Nov 11, 2014 at 10:49:02PM +0000, Sami Kerola wrote:
> >>       printf(_("Setting up swapspace version %d, size = %s (%ju bytes)\n"),
> >>               version, strsz, sz);
> >> +     free(strsz);
> >
> >  free-before-exit, you know what I think about it, right? :)
> 
> Normally I would agree, but this case is bizarre. The mkswap will
> return 1 when compiled with clang -fsanitize=address without any
> message what is wrong. That's probably clang bug, which is a different
> topic all together. Adding the free() calls seems to make the
> immediate tests failure issue to go away, that is needed to automate
> sanitize. In case travis && address sanitation is not needed at all
> then these changes does not make sense.

i don't think adding a free to avoid a bug in clang is justified

at least gcc defines __SANITIZE_ADDRESS__ ... so you might be able to gate on 
that with a comment pointing to an open bug report on the clang side
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Sami Kerola]

On Wed, 12 Nov 2014, Mike Frysinger wrote:

> On 12 Nov 2014 15:02, Sami Kerola wrote:
>> On 12 November 2014 14:25, Karel Zak <kzak@redhat.com> wrote:
>>> On Tue, Nov 11, 2014 at 10:49:02PM +0000, Sami Kerola wrote:
>>>>       printf(_("Setting up swapspace version %d, size = %s (%ju bytes)\n"),
>>>>               version, strsz, sz);
>>>> +     free(strsz);
>>>
>>>  free-before-exit, you know what I think about it, right? :)
>>
>> Normally I would agree, but this case is bizarre. The mkswap will
>> return 1 when compiled with clang -fsanitize=address without any
>> message what is wrong. That's probably clang bug, which is a different
>> topic all together. Adding the free() calls seems to make the
>> immediate tests failure issue to go away, that is needed to automate
>> sanitize. In case travis && address sanitation is not needed at all
>> then these changes does not make sense.
>
> i don't think adding a free to avoid a bug in clang is justified
>
> at least gcc defines __SANITIZE_ADDRESS__ ... so you might be able to gate on
> that with a comment pointing to an open bug report on the clang side

Hi Mike,

I like the idea, let me give a try to it.

$ git diff
diff --git a/disk-utils/mkswap.c b/disk-utils/mkswap.c
index d34e2fd..895d1ec 100644
--- a/disk-utils/mkswap.c
+++ b/disk-utils/mkswap.c
@@ -347,7 +347,7 @@ static void write_header_to_device(struct mkswap_control *ctl)
                         ctl->devname);
  }

-int main(int argc, char **argv)
+int UL_ASAN_BLACKLIST main(int argc, char **argv)
  {
         struct mkswap_control ctl = { .fd = -1 };
         int c;
@@ -489,7 +489,6 @@ int main(int argc, char **argv)

         printf(_("Setting up swapspace version %d, size = %s (%ju bytes)\n"),
                 version, strsz, sz);
-       free(strsz);

         set_signature(&ctl);
         set_uuid_and_label(&ctl);
diff --git a/include/c.h b/include/c.h
index 0f6e5b2..c581d73 100644
--- a/include/c.h
+++ b/include/c.h
@@ -313,4 +313,21 @@ static inline int xusleep(useconds_t usec)
  #define stringify_value(s) stringify(s)
  #define stringify(s) #s

+/*
+ * UL_ASAN_BLACKLIST is a macro to tell AddressSanitizer (a compile-time
+ * instrumentation shipped with Clang and GCC) to not instrument the
+ * annotated function.  Furthermore, it will prevent the compiler from
+ * inlining the function because inlining currently breaks the
+ * blacklisting mechanism of AddressSanitizer.
+ */
+#if defined(__has_feature)
+# if __has_feature(address_sanitizer)
+#  define UL_ASAN_BLACKLIST __attribute__((no_sanitize_address))
+# else
+#  define UL_ASAN_BLACKLIST    /* nothing */
+# endif
+#else
+# define UL_ASAN_BLACKLIST     /* nothing */
+#endif
+
  #endif /* UTIL_LINUX_C_H */

I borrowed the above from mozilla source.
http://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h

There is just one catch.  After compiling with no_sanitize_address 
instrumentation the return value issue is back.

$ ./mkswap test ; echo $?
lt-mkswap: test: warning: wiping old swap signature.
Setting up swapspace version 1, size = 96 KiB (98304 bytes)
no label, UUID=1aeb88e8-6442-4e0f-93a5-118271b026c0
1	<--- is the issue

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Bernhard Voelker]

On 11/12/2014 04:02 PM, Sami Kerola wrote:
> On 12 November 2014 14:25, Karel Zak <kzak@redhat.com> wrote:
>> On Tue, Nov 11, 2014 at 10:49:02PM +0000, Sami Kerola wrote:
>>>       printf(_("Setting up swapspace version %d, size = %s (%ju bytes)\n"),
>>>               version, strsz, sz);
>>> +     free(strsz);
>>
>>  free-before-exit, you know what I think about it, right? :)
> 
> Normally I would agree, but this case is bizarre. The mkswap will
> return 1 when compiled with clang -fsanitize=address without any
> message what is wrong. That's probably clang bug, which is a different
> topic all together. Adding the free() calls seems to make the
> immediate tests failure issue to go away, that is needed to automate
> sanitize. In case travis && address sanitation is not needed at all
> then these changes does not make sense.

In coreutils, we're using a macro IF_LINT as condition for such
free() calls, and for tools like Coverity, we're turning it on.

Have a nice day,
Berny

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Karel Zak]

On Wed, Nov 12, 2014 at 08:50:01PM +0000, Sami Kerola wrote:
> +/*
> + * UL_ASAN_BLACKLIST is a macro to tell AddressSanitizer (a compile-time
> + * instrumentation shipped with Clang and GCC) to not instrument the
> + * annotated function.  Furthermore, it will prevent the compiler from
> + * inlining the function because inlining currently breaks the
> + * blacklisting mechanism of AddressSanitizer.
> + */
> +#if defined(__has_feature)
> +# if __has_feature(address_sanitizer)
> +#  define UL_ASAN_BLACKLIST __attribute__((no_sanitize_address))
> +# else
> +#  define UL_ASAN_BLACKLIST    /* nothing */
> +# endif
> +#else
> +# define UL_ASAN_BLACKLIST     /* nothing */
> +#endif
> +

it would be better to follow our current manner and use directly
__attribute__ in code than introduce extra layer of macros. All
you need is define empty no_sanitize_address 
if !__has_feature(address_sanitizer).

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/6] pull: almost working AddressSanitizer support

[Karel Zak]

On Mon, Nov 10, 2014 at 09:49:49PM +0000, Sami Kerola wrote:
>  configure.ac                       | 1 +
>  lib/cpuset.c                       | 1 +
>  libmount/src/utils.c               | 2 +-
>  tests/expected/misc/swaplabel      | 2 +-
>  tests/ts/kill/name_to_number       | 3 +++
>  tests/ts/libmount/tabfiles-py      | 4 ++++
>  tests/ts/libmount/tabfiles-tags    | 4 ++++
>  tests/ts/libmount/tabfiles-tags-py | 4 ++++
>  tests/ts/libmount/update-py        | 6 +++++-
>  9 files changed, 24 insertions(+), 3 deletions(-)

Merged (with one change) from github.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 4/6] libmount: fix memory overflow [AddressSanitizer]

[Karel Zak]

On Mon, Nov 10, 2014 at 09:49:53PM +0000, Sami Kerola wrote:
> -		if (sscanf(line, " %128[^\n ]\n", name) != 1)
> +		if (sscanf(line, " %" stringify(sizeof(line) - 1)  "[^\n ]\n", name) != 1)
>  			continue;

 Are you really sure with this code? Did you test it?

 Your homework:

        #include <stdio.h>

        #define stringify(s) #s

        int main(int argc, char **argv)
        {
            char buf[123];
            printf("Size is:" stringify(sizeof(buf) - 1) ".\n");
            return 1;
        }

 result:

         $ make a 
         cc     a.c   -o a
         x2:~$ ./a
         Size is:sizeof(buf) - 1.


 Anyway, fixed and merged..

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com