11:28 p.m.
This patch set adds valgrind support for the crash's custom memory
allocator. The motivation comes from investigation at
https://github.com/crash-utility/crash-extensions/issues/1.
The 1st patch implements the valgrind support, while 2nd and 3rd fixes
the actual issues on the crash's custom memory allocator detected by
valgrind.
HATAYAMA Daisuke (3):
Add valgrind support for the crash's custom memory allocator
symbols: Fix potential read to already freed object
tools: Fix potential write to object of 0 size
Makefile | 4 ++++
configure.c | 15 ++++++++++++-
symbols.c | 10 +++------
tools.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
4 files changed, 91 insertions(+), 10 deletions(-)
--
1.8.3.1
11:28 p.m.
New subject: [PATCH v1 1/3] Add valgrind support for the crash's custom memory allocator
This adds valgrind support for the crash's custom memory allocator
using the way described in the following valgrind's Memcheck manual:
https://www.valgrind.org/docs/manual/mc-manual.html#mc-manual.mempools
This helps detecting various memory errors on the crash's custom
memory allocator.
To enable this feature, build crash command as:
# make valgrind
Then, run crash commnad using valgrind as:
# valgrind ./crash vmlinux vmcore
Signed-off-by: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
---
Makefile | 4 ++++
configure.c | 15 ++++++++++++-
tools.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 87 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index f66eba7..e0c922f 100644
--- a/Makefile
+++ b/Makefile
@@ -332,6 +332,10 @@ snappy: make_configure
@./configure -x snappy ${CONF_TARGET_FLAG} -w -b
@make --no-print-directory gdb_merge
+valgrind: make_configure
+ @./configure -x valgrind ${CONF_TARGET_FLAG} -w -b
+ @make --no-print-directory gdb_merge
+
main.o: ${GENERIC_HFILES} main.c
${CC} -c ${CRASH_CFLAGS} main.c ${WARNING_OPTIONS} ${WARNING_ERROR}
diff --git a/configure.c b/configure.c
index 7f6d19e..3748a80 100644
--- a/configure.c
+++ b/configure.c
@@ -1710,12 +1710,16 @@ get_extra_flags(char *filename, char *initial)
* For snappy:
* - enter -DLZO in the CFLAGS.extra file
* - enter -llzo2 in the LDFLAGS.extra file.
+ *
+ * For valgrind:
+ * - enter -DVALGRIND in the CFLAGS.extra file
*/
void
add_extra_lib(char *option)
{
int lzo, add_DLZO, add_llzo2;
int snappy, add_DSNAPPY, add_lsnappy;
+ int valgrind, add_DVALGRIND;
char *cflags, *ldflags;
FILE *fp_cflags, *fp_ldflags;
char *mode;
@@ -1723,6 +1727,7 @@ add_extra_lib(char *option)
lzo = add_DLZO = add_llzo2 = 0;
snappy = add_DSNAPPY = add_lsnappy = 0;
+ valgrind = add_DVALGRIND = 0;
ldflags = get_extra_flags("LDFLAGS.extra", NULL);
cflags = get_extra_flags("CFLAGS.extra", NULL);
@@ -1743,6 +1748,12 @@ add_extra_lib(char *option)
add_lsnappy++;
}
+ if (strcmp(option, "valgrind") == 0) {
+ valgrind++;
+ if (!cflags || !strstr(cflags, "-DVALGRIND"))
+ add_DVALGRIND++;
+ }
+
if ((lzo || snappy) &&
file_exists("diskdump.o") && (unlink("diskdump.o") <
0)) {
perror("diskdump.o");
@@ -1762,13 +1773,15 @@ add_extra_lib(char *option)
return;
}
- if (add_DLZO || add_DSNAPPY) {
+ if (add_DLZO || add_DSNAPPY || add_DVALGRIND) {
while (fgets(inbuf, 512, fp_cflags))
;
if (add_DLZO)
fputs("-DLZO\n", fp_cflags);
if (add_DSNAPPY)
fputs("-DSNAPPY\n", fp_cflags);
+ if (add_DVALGRIND)
+ fputs("-DVALGRIND\n", fp_cflags);
}
if (add_llzo2 || add_lsnappy) {
diff --git a/tools.c b/tools.c
index 89352b1..91d5baf 100644
--- a/tools.c
+++ b/tools.c
@@ -18,6 +18,11 @@
#include "defs.h"
#include <ctype.h>
+#ifdef VALGRIND
+#include <valgrind/valgrind.h>
+#include <valgrind/memcheck.h>
+#endif
+
static void print_number(struct number_option *, int, int);
static long alloc_hq_entry(void);
struct hq_entry;
@@ -5679,8 +5684,21 @@ buf_init(void)
bp->smallest = 0x7fffffff;
bp->total = 0.0;
-}
+#ifdef VALGRIND
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_1K, sizeof(bp->buf_1K));
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_2K, sizeof(bp->buf_2K));
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_4K, sizeof(bp->buf_4K));
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_8K, sizeof(bp->buf_8K));
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_32K, sizeof(bp->buf_32K));
+
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_1K, 0, 1);
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_2K, 0, 1);
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_4K, 0, 1);
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_8K, 0, 1);
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_32K, 0, 1);
+#endif
+}
/*
* Free up all buffers used by the last command.
@@ -5707,6 +5725,26 @@ void free_all_bufs(void)
if (bp->mallocs != bp->frees)
error(WARNING, "malloc/free mismatch (%ld/%ld)\n",
bp->mallocs, bp->frees);
+
+#ifdef VALGRIND
+ VALGRIND_DESTROY_MEMPOOL(&bp->buf_1K);
+ VALGRIND_DESTROY_MEMPOOL(&bp->buf_2K);
+ VALGRIND_DESTROY_MEMPOOL(&bp->buf_4K);
+ VALGRIND_DESTROY_MEMPOOL(&bp->buf_8K);
+ VALGRIND_DESTROY_MEMPOOL(&bp->buf_32K);
+
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_1K, sizeof(bp->buf_1K));
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_2K, sizeof(bp->buf_2K));
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_4K, sizeof(bp->buf_4K));
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_8K, sizeof(bp->buf_8K));
+ VALGRIND_MAKE_MEM_NOACCESS(&bp->buf_32K, sizeof(bp->buf_32K));
+
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_1K, 0, 1);
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_2K, 0, 1);
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_4K, 0, 1);
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_8K, 0, 1);
+ VALGRIND_CREATE_MEMPOOL(&bp->buf_32K, 0, 1);
+#endif
}
/*
@@ -5731,6 +5769,9 @@ freebuf(char *addr)
for (i = 0; i < NUMBER_1K_BUFS; i++) {
if (addr == (char *)&bp->buf_1K[i]) {
bp->buf_inuse[B1K] &= ~(1 << i);
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_FREE(&bp->buf_1K, addr);
+#endif
return;
}
}
@@ -5738,6 +5779,9 @@ freebuf(char *addr)
for (i = 0; i < NUMBER_2K_BUFS; i++) {
if (addr == (char *)&bp->buf_2K[i]) {
bp->buf_inuse[B2K] &= ~(1 << i);
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_FREE(&bp->buf_2K, addr);
+#endif
return;
}
}
@@ -5745,6 +5789,9 @@ freebuf(char *addr)
for (i = 0; i < NUMBER_4K_BUFS; i++) {
if (addr == (char *)&bp->buf_4K[i]) {
bp->buf_inuse[B4K] &= ~(1 << i);
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_FREE(&bp->buf_4K, addr);
+#endif
return;
}
}
@@ -5752,6 +5799,9 @@ freebuf(char *addr)
for (i = 0; i < NUMBER_8K_BUFS; i++) {
if (addr == (char *)&bp->buf_8K[i]) {
bp->buf_inuse[B8K] &= ~(1 << i);
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_FREE(&bp->buf_8K, addr);
+#endif
return;
}
}
@@ -5759,6 +5809,9 @@ freebuf(char *addr)
for (i = 0; i < NUMBER_32K_BUFS; i++) {
if (addr == (char *)&bp->buf_32K[i]) {
bp->buf_inuse[B32K] &= ~(1 << i);
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_FREE(&bp->buf_32K, addr);
+#endif
return;
}
}
@@ -5924,6 +5977,9 @@ getbuf(long reqsize)
bp->buf_inuse[B1K] |= (1 << bdx);
bp->buf_1K_maxuse = MAX(bp->buf_1K_maxuse,
count_bits_int(bp->buf_inuse[B1K]));
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_ALLOC(&bp->buf_1K, bufp, 1024);
+#endif
BZERO(bufp, 1024);
return(bufp);
}
@@ -5938,6 +5994,9 @@ getbuf(long reqsize)
bp->buf_inuse[B2K] |= (1 << bdx);
bp->buf_2K_maxuse = MAX(bp->buf_2K_maxuse,
count_bits_int(bp->buf_inuse[B2K]));
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_ALLOC(&bp->buf_2K, bufp, 2048);
+#endif
BZERO(bufp, 2048);
return(bufp);
}
@@ -5952,6 +6011,9 @@ getbuf(long reqsize)
bp->buf_inuse[B4K] |= (1 << bdx);
bp->buf_4K_maxuse = MAX(bp->buf_4K_maxuse,
count_bits_int(bp->buf_inuse[B4K]));
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_ALLOC(&bp->buf_4K, bufp, 4096);
+#endif
BZERO(bufp, 4096);
return(bufp);
}
@@ -5966,6 +6028,9 @@ getbuf(long reqsize)
bp->buf_inuse[B8K] |= (1 << bdx);
bp->buf_8K_maxuse = MAX(bp->buf_8K_maxuse,
count_bits_int(bp->buf_inuse[B8K]));
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_ALLOC(&bp->buf_8K, bufp, 8192);
+#endif
BZERO(bufp, 8192);
return(bufp);
}
@@ -5980,6 +6045,9 @@ getbuf(long reqsize)
bp->buf_inuse[B32K] |= (1 << bdx);
bp->buf_32K_maxuse = MAX(bp->buf_32K_maxuse,
count_bits_int(bp->buf_inuse[B32K]));
+#ifdef VALGRIND
+ VALGRIND_MEMPOOL_ALLOC(&bp->buf_32K, bufp, 32768);
+#endif
BZERO(bufp, 32768);
return(bufp);
}
--
1.8.3.1
11:28 p.m.
New subject: [PATCH v1 2/3] symbols: Fix potential read to already freed object
valgrind detects the following potential invalid read to some already
freed object:
==22220== Invalid read of size 4
==22220== at 0x539641: datatype_info (symbols.c:5791)
==22220== by 0x4EC8B1: dump_variable_length_record_log (kernel.c:5313)
==22220== by 0x4EC8B1: dump_log (kernel.c:5042)
==22220== by 0x4C5A25: get_panicmsg (task.c:6275)
==22220== by 0x4F3E71: display_sys_stats (kernel.c:5645)
==22220== by 0x464BC7: main_loop (main.c:797)
==22220== by 0x6BF262: captured_command_loop (main.c:258)
==22220== by 0x6BD869: catch_errors (exceptions.c:557)
==22220== by 0x6C02E5: captured_main (main.c:1064)
==22220== by 0x6BD869: catch_errors (exceptions.c:557)
==22220== by 0x6C0596: gdb_main (main.c:1079)
==22220== by 0x6C0596: gdb_main_entry (main.c:1099)
==22220== by 0x46316F: main (main.c:708)
==22220== Address 0xb498c8 is 72 bytes inside a block of size 1,024 free'd
==22220== at 0x471261: freebuf (tools.c:5766)
==22220== by 0x53946B: datatype_info (symbols.c:5747)
==22220== by 0x4FEA2A: net_init (net.c:173)
==22220== by 0x464A55: main_loop (main.c:777)
==22220== by 0x6BF262: captured_command_loop (main.c:258)
==22220== by 0x6BD869: catch_errors (exceptions.c:557)
==22220== by 0x6C02E5: captured_main (main.c:1064)
==22220== by 0x6BD869: catch_errors (exceptions.c:557)
==22220== by 0x6C0596: gdb_main (main.c:1079)
==22220== by 0x6C0596: gdb_main_entry (main.c:1099)
==22220== by 0x46316F: main (main.c:708)
==22220== Block was alloc'd at
==22220== at 0x471C80: getbuf (tools.c:5965)
==22220== by 0x5392B7: datatype_info (symbols.c:5624)
==22220== by 0x4FEA2A: net_init (net.c:173)
==22220== by 0x464A55: main_loop (main.c:777)
==22220== by 0x6BF262: captured_command_loop (main.c:258)
==22220== by 0x6BD869: catch_errors (exceptions.c:557)
==22220== by 0x6C02E5: captured_main (main.c:1064)
==22220== by 0x6BD869: catch_errors (exceptions.c:557)
==22220== by 0x6C0596: gdb_main (main.c:1079)
==22220== by 0x6C0596: gdb_main_entry (main.c:1099)
==22220== by 0x46316F: main (main.c:708)
==22220==
This was caused by the fact that in datatype_info(), the object
associated with the variable req is freed too early although it's
still be referred to after the freeing.
Fix this by changing the way allocating the object from by GETBUF() to
by allocation on stack, which simplifies the code because explicit
free() operations are unnecessary.
Signed-off-by: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
---
symbols.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/symbols.c b/symbols.c
index a51078d..c0f68af 100644
--- a/symbols.c
+++ b/symbols.c
@@ -5611,7 +5611,7 @@ datatype_init(void)
long
datatype_info(char *name, char *member, struct datatype_member *dm)
{
- struct gnu_request *req;
+ struct gnu_request request, *req = &request;
long offset, size, member_size;
int member_typecode;
ulong type_found;
@@ -5625,7 +5625,7 @@ datatype_info(char *name, char *member, struct datatype_member *dm)
strcpy(buf, name);
- req = (struct gnu_request *)GETBUF(sizeof(struct gnu_request));
+ BZERO(req, sizeof(*req));
req->command = GNU_GET_DATATYPE;
req->flags |= GNU_RETURN_ON_ERROR;
req->name = buf;
@@ -5633,10 +5633,8 @@ datatype_info(char *name, char *member, struct datatype_member
*dm)
req->fp = pc->nullfp;
gdb_interface(req);
- if (req->flags & GNU_COMMAND_FAILED) {
- FREEBUF(req);
+ if (req->flags & GNU_COMMAND_FAILED)
return (dm == MEMBER_TYPE_NAME_REQUEST) ? 0 : -1;
- }
if (!req->typecode) {
sprintf(buf, "struct %s", name);
@@ -5748,8 +5746,6 @@ datatype_info(char *name, char *member, struct datatype_member *dm)
break;
}
- FREEBUF(req);
-
if (dm && (dm != MEMBER_SIZE_REQUEST) && (dm !=
MEMBER_TYPE_REQUEST) &&
(dm != STRUCT_SIZE_REQUEST) && (dm != MEMBER_TYPE_NAME_REQUEST)) {
dm->type = type_found;
--
1.8.3.1
11:28 p.m.
New subject: [PATCH v1 3/3] tools: Fix potential write to object of 0 size
valgrind detects the following invalid write on the call of strcpy():
==22674== Invalid write of size 1
==22674== at 0x483CCFE: strcpy (vg_replace_strmem.c:511)
==22674== by 0x47202B: extract_hex (tools.c:1152)
==22674== by 0x5104ED: search_for_switch_to (x86_64.c:6342)
==22674== by 0x51D6EE: x86_64_thread_return_init (x86_64.c:6368)
==22674== by 0x51D6EE: x86_64_init (x86_64.c:721)
==22674== by 0x464A2D: main_loop (main.c:770)
==22674== by 0x6BF1B2: captured_command_loop (main.c:258)
==22674== by 0x6BD7B9: catch_errors (exceptions.c:557)
==22674== by 0x6C0235: captured_main (main.c:1064)
==22674== by 0x6BD7B9: catch_errors (exceptions.c:557)
==22674== by 0x6C04E6: gdb_main (main.c:1079)
==22674== by 0x6C04E6: gdb_main_entry (main.c:1099)
==22674== by 0x46316F: main (main.c:708)
==22674== Address 0x2b439eb8 is 0 bytes after a block of size 40 alloc'd
==22674== at 0x483BAE9: calloc (vg_replace_malloc.c:760)
==22674== by 0x471794: getbuf (tools.c:6036)
==22674== by 0x47201D: extract_hex (tools.c:1151)
==22674== by 0x5104ED: search_for_switch_to (x86_64.c:6342)
==22674== by 0x51D6EE: x86_64_thread_return_init (x86_64.c:6368)
==22674== by 0x51D6EE: x86_64_init (x86_64.c:721)
==22674== by 0x464A2D: main_loop (main.c:770)
==22674== by 0x6BF1B2: captured_command_loop (main.c:258)
==22674== by 0x6BD7B9: catch_errors (exceptions.c:557)
==22674== by 0x6C0235: captured_main (main.c:1064)
==22674== by 0x6BD7B9: catch_errors (exceptions.c:557)
==22674== by 0x6C04E6: gdb_main (main.c:1079)
==22674== by 0x6C04E6: gdb_main_entry (main.c:1099)
==22674== by 0x46316F: main (main.c:708)
This is due to strcpy() receives empty string in its 1st argument
because the size of the buffer associated with buf variable then is of
size 0 due to lack of consideration of the terminal '\0' byte.
Fix this by +1 to the buffer size for the terminal '\0' byte.
Signed-off-by: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
---
tools.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools.c b/tools.c
index 91d5baf..8050d30 100644
--- a/tools.c
+++ b/tools.c
@@ -1150,7 +1150,7 @@ extract_hex(char *s, ulong *result, char stripchar, ulong
first_instance)
ulong value;
char *buf;
- buf = GETBUF(strlen(s));
+ buf = GETBUF(strlen(s) + 1);
strcpy(buf, s);
argc = parse_line(buf, arglist);
--
1.8.3.1
10:46 p.m.
Any comments?
Thanks.
HATAYAMA, Daisuke
________________________________________
From: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
Sent: Monday, January 4, 2021 14:28
To: crash-utility(a)redhat.com
Cc: Hatayama, Daisuke/畑山 大輔
Subject: [PATCH v1 0/3] Add valgrind support for the crash's custom memory
This patch set adds valgrind support for the crash's custom memory
allocator. The motivation comes from investigation at
https://github.com/crash-utility/crash-extensions/issues/1.
The 1st patch implements the valgrind support, while 2nd and 3rd fixes
the actual issues on the crash's custom memory allocator detected by
valgrind.
HATAYAMA Daisuke (3):
Add valgrind support for the crash's custom memory allocator
symbols: Fix potential read to already freed object
tools: Fix potential write to object of 0 size
Makefile | 4 ++++
configure.c | 15 ++++++++++++-
symbols.c | 10 +++------
tools.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
4 files changed, 91 insertions(+), 10 deletions(-)
--
1.8.3.1
7:03 p.m.
New subject: [PATCH v1 0/3] Add valgrind support for the crash's custom memory
Hi Hatayama-san,
-----Original Message-----
This patch set adds valgrind support for the crash's custom
memory
allocator. The motivation comes from investigation at
https://github.com/crash-utility/crash-extensions/issues/1.
The 1st patch implements the valgrind support, while 2nd and 3rd fixes
the actual issues on the crash's custom memory allocator detected by
valgrind.
This series looks good to me.
Acked-by: Kazuhito Hagio <k-hagio-ab(a)nec.com>
I hope this helps us develop/debug crash and extension modules.
Thanks,
Kazu
HATAYAMA Daisuke (3):
Add valgrind support for the crash's custom memory allocator
symbols: Fix potential read to already freed object
tools: Fix potential write to object of 0 size
Makefile | 4 ++++
configure.c | 15 ++++++++++++-
symbols.c | 10 +++------
tools.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
4 files changed, 91 insertions(+), 10 deletions(-)
--
1.8.3.1
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
1424
days inactive
1453
days old
devel@lists.crash-utility.osci.io
5 comments
3 participants
participants (3)
-
d.hatayama＠fujitsu.com -
HAGIO KAZUHITO(萩尾 一仁) -
HATAYAMA Daisuke