[PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan]

[PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan]

[Sami Kerola]

ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffee13b8f05 at
pc 0x55debcc9fda4 bp 0x7ffee13b8d70 sp 0x7ffee13b8520
WRITE of size 38 at 0x7ffee13b8f05 thread T0
    #0 0x55debcc9fda3 in scanf_common(void*, int, bool, char const*, __va_list_tag*) (/home/src/util-linux/.libs/lt-uuidparse+0x63da3)
    #1 0x55debcca077b in vscanf (/home/src/util-linux/.libs/lt-uuidparse+0x6477b)
    #2 0x55debcca085f in scanf (/home/src/util-linux/.libs/lt-uuidparse+0x6485f)
    #3 0x55debcd50fad in print_output /home/src/util-linux/misc-utils/uuidparse.c:284:10
    #4 0x55debcd4fcc5 in main /home/src/util-linux/misc-utils/uuidparse.c:355:2

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

diff --git a/misc-utils/uuidparse.c b/misc-utils/uuidparse.c
index 5fa34b23d..3347dc0af 100644
--- a/misc-utils/uuidparse.c
+++ b/misc-utils/uuidparse.c
@@ -279,7 +279,7 @@ static void print_output(struct control const *const ctrl, int argc,
 		fill_table_row(tb, argv[i]);
 
 	if (i == 0) {
-		char uuid[UUID_STR_LEN];
+		char uuid[UUID_STR_LEN + 1];
 
 		while (scanf(" %" stringify_value(UUID_STR_LEN)
 			     "[^ \t\n]%*c", uuid) && !feof(stdin))
-- 
2.14.1

[PATCH 2/4] libsmartcols: fix heap-buffer-overflow [asan]

[Sami Kerola]

This gives room for element 0 in array of column cells.

ERROR: AddressSanitizer: heap-buffer-overflow on address 0x608000000080 at
pc 0x5596bfaaca26 bp 0x7ffd8352a550 sp 0x7ffd83529d00
READ of size 64 at 0x608000000080 thread T0
    #0 0x5596bfaaca25 in __asan_memmove (/home/src/util-linux/.libs/lt-column+0xc2a25)
    #1 0x7f8d53660d1b in scols_line_move_cells /home/src/util-linux/libsmartcols/src/line.c:164:2
    #2 0x7f8d5366502c in scols_table_move_column /home/src/util-linux/libsmartcols/src/table.c:307:3
    #3 0x5596bfb0214b in reorder_table /home/src/util-linux/text-utils/column.c:303:3
    #4 0x5596bfaff7ec in modify_table /home/src/util-linux/text-utils/column.c:393:3
    #5 0x5596bfafdbb6 in main /home/src/util-linux/text-utils/column.c:770:4
    #6 0x7f8d52718f69 in __libc_start_main (/usr/lib/libc.so.6+0x20f69)
    #7 0x5596bfa089b9 in _start (/home/src/util-linux/.libs/lt-column+0x1e9b9)

0x608000000080 is located 0 bytes to the right of 96-byte region [0x608000000020,0x608000000080)

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

diff --git a/libsmartcols/src/line.c b/libsmartcols/src/line.c
index aa339ce38..a041eeb60 100644
--- a/libsmartcols/src/line.c
+++ b/libsmartcols/src/line.c
@@ -133,7 +133,7 @@ int scols_line_alloc_cells(struct libscols_line *ln, size_t n)
 
 	DBG(LINE, ul_debugobj(ln, "alloc %zu cells", n));
 
-	ce = realloc(ln->cells, n * sizeof(struct libscols_cell));
+	ce = realloc(ln->cells, (n + 1) * sizeof(struct libscols_cell));
 	if (!ce)
 		return -errno;
 
-- 
2.14.1

[PATCH 3/4] logger: fix memory leaks [asan]

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/logger.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index f6fb350cc..4eba778b9 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -606,25 +606,30 @@ static char *strdup_structured_data(struct structured_data *sd)
 
 	xasprintf(&res, "[%s %s]", sd->id,
 			(tmp = strv_join(sd->params, " ")));
+	free(sd->id);
+	strv_free(sd->params);
 	free(tmp);
 	return res;
 }
 
 static char *strdup_structured_data_list(struct list_head *ls)
 {
-	struct list_head *p;
+	struct list_head *p, *pnext;
 	char *res = NULL;
 
-	list_for_each(p, ls) {
+	list_for_each_safe(p, pnext, ls) {
 		struct structured_data *sd = list_entry(p, struct structured_data, sds);
 		char *one = strdup_structured_data(sd);
 		char *tmp = res;
 
-		if (!one)
+		if (!one) {
+			free(tmp);
 			continue;
+		}
 		res = strappend(tmp, one);
 		free(tmp);
 		free(one);
+		free(sd);
 	}
 
 	return res;
@@ -979,6 +984,7 @@ static void logger_stdin(struct logger_ctl *ctl)
 		if (c == '\n')	/* discard line terminator */
 			c = getchar();
 	}
+	free(buf);
 }
 
 static void logger_close(const struct logger_ctl *ctl)
-- 
2.14.1

[PATCH 4/4] partx: fix memory leak [asan]

[Sami Kerola]

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

diff --git a/disk-utils/partx.c b/disk-utils/partx.c
index 43a6448db..0d66abadb 100644
--- a/disk-utils/partx.c
+++ b/disk-utils/partx.c
@@ -1054,6 +1054,8 @@ int main(int argc, char **argv)
 	if (loopdev)
 		loopcxt_deinit(&lc);
 
+	free(wholedisk);
+
 	if (close_fd(fd) != 0)
 		err(EXIT_FAILURE, _("write failed"));
 
-- 
2.14.1

Re: [PATCH 3/4] logger: fix memory leaks [asan]

[Karel Zak]

On Mon, Sep 11, 2017 at 08:57:16PM +0100, Sami Kerola wrote:
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  misc-utils/logger.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/misc-utils/logger.c b/misc-utils/logger.c
> index f6fb350cc..4eba778b9 100644
> --- a/misc-utils/logger.c
> +++ b/misc-utils/logger.c
> @@ -606,25 +606,30 @@ static char *strdup_structured_data(struct structured_data *sd)
>  
>  	xasprintf(&res, "[%s %s]", sd->id,
>  			(tmp = strv_join(sd->params, " ")));
> +	free(sd->id);
> +	strv_free(sd->params);
>  	free(tmp);
>  	return res;
>  }

This is strdup-like function, why do you touch the argument 'sd' ?
It's has to copy and nothing else.

>  static char *strdup_structured_data_list(struct list_head *ls)
>  {
> -	struct list_head *p;
> +	struct list_head *p, *pnext;
>  	char *res = NULL;
>  
> -	list_for_each(p, ls) {
> +	list_for_each_safe(p, pnext, ls) {
>  		struct structured_data *sd = list_entry(p, struct structured_data, sds);
>  		char *one = strdup_structured_data(sd);
>  		char *tmp = res;
>  
> -		if (!one)
> +		if (!one) {
> +			free(tmp);

 tmp contains result from the previous loop

>  			continue;
> +		}
>  		res = strappend(tmp, one);
>  		free(tmp);
>  		free(one);
> +		free(sd);
>  	}

 again, I don't see reason to deallocate 'sd' in strdup-like function.
 It just converts the list to string and nothing else. It's also crazy
 to use free() for linked list member.

 The list contains information specified by user on command line, if
 you deallocate the list then the next attempt to generate a header
 will produce incomplete information.

 All the story seems like free-before-exit. We don't care about it.

 The mem-leaks are bad thing in daemons, libs, libs tests, and
 allocation in loop is dangerous, otherwise for command line utils you
 waste time with this kind of free() calls, because kernel on exit()
 will be faster to remove all the program from memory. 

    Karel

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

Re: [PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan]

[Karel Zak]

On Mon, Sep 11, 2017 at 08:57:14PM +0100, Sami Kerola wrote:
> ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffee13b8f05 at
> pc 0x55debcc9fda4 bp 0x7ffee13b8d70 sp 0x7ffee13b8520
> WRITE of size 38 at 0x7ffee13b8f05 thread T0
>     #0 0x55debcc9fda3 in scanf_common(void*, int, bool, char const*, __va_list_tag*) (/home/src/util-linux/.libs/lt-uuidparse+0x63da3)
>     #1 0x55debcca077b in vscanf (/home/src/util-linux/.libs/lt-uuidparse+0x6477b)
>     #2 0x55debcca085f in scanf (/home/src/util-linux/.libs/lt-uuidparse+0x6485f)
>     #3 0x55debcd50fad in print_output /home/src/util-linux/misc-utils/uuidparse.c:284:10
>     #4 0x55debcd4fcc5 in main /home/src/util-linux/misc-utils/uuidparse.c:355:2
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  misc-utils/uuidparse.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/misc-utils/uuidparse.c b/misc-utils/uuidparse.c
> index 5fa34b23d..3347dc0af 100644
> --- a/misc-utils/uuidparse.c
> +++ b/misc-utils/uuidparse.c
> @@ -279,7 +279,7 @@ static void print_output(struct control const *const ctrl, int argc,
>  		fill_table_row(tb, argv[i]);
>  
>  	if (i == 0) {
> -		char uuid[UUID_STR_LEN];
> +		char uuid[UUID_STR_LEN + 1];
>  
>  		while (scanf(" %" stringify_value(UUID_STR_LEN)

 It seems better to use "%36[" because the uuid size is really
 36+\0.

 Fixed, good catch.

    Karel

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

Re: [PATCH 2/4] libsmartcols: fix heap-buffer-overflow [asan]

[Karel Zak]

On Mon, Sep 11, 2017 at 08:57:15PM +0100, Sami Kerola wrote:
> This gives room for element 0 in array of column cells.
> 
> ERROR: AddressSanitizer: heap-buffer-overflow on address 0x608000000080 at
> pc 0x5596bfaaca26 bp 0x7ffd8352a550 sp 0x7ffd83529d00
> READ of size 64 at 0x608000000080 thread T0
>     #0 0x5596bfaaca25 in __asan_memmove (/home/src/util-linux/.libs/lt-column+0xc2a25)
>     #1 0x7f8d53660d1b in scols_line_move_cells /home/src/util-linux/libsmartcols/src/line.c:164:2
>     #2 0x7f8d5366502c in scols_table_move_column /home/src/util-linux/libsmartcols/src/table.c:307:3
>     #3 0x5596bfb0214b in reorder_table /home/src/util-linux/text-utils/column.c:303:3
>     #4 0x5596bfaff7ec in modify_table /home/src/util-linux/text-utils/column.c:393:3
>     #5 0x5596bfafdbb6 in main /home/src/util-linux/text-utils/column.c:770:4
>     #6 0x7f8d52718f69 in __libc_start_main (/usr/lib/libc.so.6+0x20f69)
>     #7 0x5596bfa089b9 in _start (/home/src/util-linux/.libs/lt-column+0x1e9b9)
> 
> 0x608000000080 is located 0 bytes to the right of 96-byte region [0x608000000020,0x608000000080)
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  libsmartcols/src/line.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libsmartcols/src/line.c b/libsmartcols/src/line.c
> index aa339ce38..a041eeb60 100644
> --- a/libsmartcols/src/line.c
> +++ b/libsmartcols/src/line.c
> @@ -133,7 +133,7 @@ int scols_line_alloc_cells(struct libscols_line *ln, size_t n)
>  
>  	DBG(LINE, ul_debugobj(ln, "alloc %zu cells", n));
>  
> -	ce = realloc(ln->cells, n * sizeof(struct libscols_cell));
> +	ce = realloc(ln->cells, (n + 1) * sizeof(struct libscols_cell));

The issue is scols_line_move_cells(). Fixed. Thanks!

    Karel

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

Re: [PATCH 3/4] logger: fix memory leaks [asan]

[J William Piggott]

On 09/15/2017 07:05 AM, Karel Zak wrote:
> On Mon, Sep 11, 2017 at 08:57:16PM +0100, Sami Kerola wrote:

 8< ---

>  The mem-leaks are bad thing in daemons, libs, libs tests, and
>  allocation in loop is dangerous, otherwise for command line utils you
>  waste time with this kind of free() calls, because kernel on exit()
>  will be faster to remove all the program from memory. 

Does this rule apply to the free() calls in hwclock.c?

> 
>     Karel
> 

Re: [PATCH 3/4] logger: fix memory leaks [asan]

[Sami Kerola]

On 16 September 2017 at 01:54, J William Piggott <elseifthen@gmx.com> wrote:
> On 09/15/2017 07:05 AM, Karel Zak wrote:
>> On Mon, Sep 11, 2017 at 08:57:16PM +0100, Sami Kerola wrote:
>>  The mem-leaks are bad thing in daemons, libs, libs tests, and
>>  allocation in loop is dangerous, otherwise for command line utils you
>>  waste time with this kind of free() calls, because kernel on exit()
>>  will be faster to remove all the program from memory.
>
> Does this rule apply to the free() calls in hwclock.c?

What I was trying to to achieve was as few ASAN failures as possible.
That makes seeing memory corruptions, buffer overflows and other
things that do matter easier. Maybe adding supressions is good
enough way to deal the rest.

https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer

Thank you Karel making my patches better. Here's what I can still
see:

https://pastebin.com/raw/U4CSUrxS

Compiled with these:

export CC=clang
export CFLAGS='-fsanitize=address -ggdb'
export ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer

And disk-utils/partx.c needs atexit(close_stdout); commented out
to display errors.

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

Re: [PATCH 3/4] logger: fix memory leaks [asan]

[Karel Zak]

On Sat, Sep 16, 2017 at 08:31:27AM +0100, Sami Kerola wrote:
> On 16 September 2017 at 01:54, J William Piggott <elseifthen@gmx.com> wrote:
> > On 09/15/2017 07:05 AM, Karel Zak wrote:
> >> On Mon, Sep 11, 2017 at 08:57:16PM +0100, Sami Kerola wrote:
> >>  The mem-leaks are bad thing in daemons, libs, libs tests, and
> >>  allocation in loop is dangerous, otherwise for command line utils you
> >>  waste time with this kind of free() calls, because kernel on exit()
> >>  will be faster to remove all the program from memory.
> >
> > Does this rule apply to the free() calls in hwclock.c?
> 
> What I was trying to to achieve was as few ASAN failures as possible.
> That makes seeing memory corruptions, buffer overflows and other
> things that do matter easier. Maybe adding supressions is good
> enough way to deal the rest.
> 
> https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer
> 
> Thank you Karel making my patches better. Here's what I can still
> see:
> 
> https://pastebin.com/raw/U4CSUrxS

Fixed. Thanks!

 Karel

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

Re: [PATCH 3/4] logger: fix memory leaks [asan]

[Karel Zak]

On Sat, Sep 16, 2017 at 08:31:27AM +0100, Sami Kerola wrote:
> Compiled with these:
> 
> export CC=clang
> export CFLAGS='-fsanitize=address -ggdb'
> export ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer

An idea for the next release v2.32:

 * add a new --enable-clang-asan to ./configure
 * add a new --asan option to tests/run.sh
 * rename tests/run.sh --memcheck to --valgrind 
 * rename tests/functions.sh:ts_valgrind() to ts_run()
 * improve ts_run() to be sensitive to --asan or --valgrind (etc.)
 * use export ASAN_OPTIONS='detect_leaks=0' by default
 * add TS_ENABLE_MEMLEAK_CHECK=yes to tests where we want to check for
   memory leaks (library tests, daemons, etc.)


and then we can use

    $ ./configure --enable-clang-asan
    $ cd tests
    $ ./run.sh --asan

to be sure we're without memory corruptions or relevant memory leaks.

    Karel


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