* [PATCH 2/4] libsmartcols: fix heap-buffer-overflow [asan]
2017-09-11 19:57 [PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan] Sami Kerola
@ 2017-09-11 19:57 ` Sami Kerola
2017-09-15 11:47 ` Karel Zak
2017-09-11 19:57 ` [PATCH 3/4] logger: fix memory leaks [asan] Sami Kerola
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Sami Kerola @ 2017-09-11 19:57 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/4] libsmartcols: fix heap-buffer-overflow [asan]
2017-09-11 19:57 ` [PATCH 2/4] libsmartcols: fix heap-buffer-overflow [asan] Sami Kerola
@ 2017-09-15 11:47 ` Karel Zak
0 siblings, 0 replies; 11+ messages in thread
From: Karel Zak @ 2017-09-15 11:47 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] logger: fix memory leaks [asan]
2017-09-11 19:57 [PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan] Sami Kerola
2017-09-11 19:57 ` [PATCH 2/4] libsmartcols: fix heap-buffer-overflow [asan] Sami Kerola
@ 2017-09-11 19:57 ` Sami Kerola
2017-09-15 11:05 ` Karel Zak
2017-09-11 19:57 ` [PATCH 4/4] partx: fix memory leak [asan] Sami Kerola
2017-09-15 11:33 ` [PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan] Karel Zak
3 siblings, 1 reply; 11+ messages in thread
From: Sami Kerola @ 2017-09-11 19:57 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/4] logger: fix memory leaks [asan]
2017-09-11 19:57 ` [PATCH 3/4] logger: fix memory leaks [asan] Sami Kerola
@ 2017-09-15 11:05 ` Karel Zak
2017-09-16 0:54 ` J William Piggott
0 siblings, 1 reply; 11+ messages in thread
From: Karel Zak @ 2017-09-15 11:05 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/4] logger: fix memory leaks [asan]
2017-09-15 11:05 ` Karel Zak
@ 2017-09-16 0:54 ` J William Piggott
2017-09-16 7:31 ` Sami Kerola
0 siblings, 1 reply; 11+ messages in thread
From: J William Piggott @ 2017-09-16 0:54 UTC (permalink / raw)
To: Karel Zak, Sami Kerola; +Cc: util-linux
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] logger: fix memory leaks [asan]
2017-09-16 0:54 ` J William Piggott
@ 2017-09-16 7:31 ` Sami Kerola
2017-09-18 8:55 ` Karel Zak
2017-09-18 9:40 ` Karel Zak
0 siblings, 2 replies; 11+ messages in thread
From: Sami Kerola @ 2017-09-16 7:31 UTC (permalink / raw)
To: J William Piggott; +Cc: Karel Zak, util-linux
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] logger: fix memory leaks [asan]
2017-09-16 7:31 ` Sami Kerola
@ 2017-09-18 8:55 ` Karel Zak
2017-09-18 9:40 ` Karel Zak
1 sibling, 0 replies; 11+ messages in thread
From: Karel Zak @ 2017-09-18 8:55 UTC (permalink / raw)
To: kerolasa; +Cc: J William Piggott, util-linux
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] logger: fix memory leaks [asan]
2017-09-16 7:31 ` Sami Kerola
2017-09-18 8:55 ` Karel Zak
@ 2017-09-18 9:40 ` Karel Zak
1 sibling, 0 replies; 11+ messages in thread
From: Karel Zak @ 2017-09-18 9:40 UTC (permalink / raw)
To: kerolasa; +Cc: J William Piggott, util-linux
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] partx: fix memory leak [asan]
2017-09-11 19:57 [PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan] Sami Kerola
2017-09-11 19:57 ` [PATCH 2/4] libsmartcols: fix heap-buffer-overflow [asan] Sami Kerola
2017-09-11 19:57 ` [PATCH 3/4] logger: fix memory leaks [asan] Sami Kerola
@ 2017-09-11 19:57 ` Sami Kerola
2017-09-15 11:33 ` [PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan] Karel Zak
3 siblings, 0 replies; 11+ messages in thread
From: Sami Kerola @ 2017-09-11 19:57 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan]
2017-09-11 19:57 [PATCH 1/4] uuidparse: fix stack-buffer-overflow [asan] Sami Kerola
` (2 preceding siblings ...)
2017-09-11 19:57 ` [PATCH 4/4] partx: fix memory leak [asan] Sami Kerola
@ 2017-09-15 11:33 ` Karel Zak
3 siblings, 0 replies; 11+ messages in thread
From: Karel Zak @ 2017-09-15 11:33 UTC (permalink / raw)
To: Sami Kerola; +Cc: util-linux
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
^ permalink raw reply [flat|nested] 11+ messages in thread