5:20 a.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.
v2:
- Unlink tools.o when make valgrind
- Add description about make valgrind in README
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 ++++
README | 5 +++++
configure.c | 21 +++++++++++++++++-
help.c | 5 +++++
symbols.c | 10 +++------
tools.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
6 files changed, 107 insertions(+), 10 deletions(-)
--
1.8.3.1
5:20 a.m.
New subject: [PATCH v2 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 ++++
README | 5 +++++
configure.c | 21 ++++++++++++++++++-
help.c | 5 +++++
tools.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
5 files changed, 103 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/README b/README
index e2af924..9b9c6e6 100644
--- a/README
+++ b/README
@@ -105,6 +105,11 @@
use either the LZO or snappy compression libraries. To build crash with
either or both of those libraries, type "make lzo" or "make
snappy".
+ crash supports valgrind Memcheck tool on the crash's custom memory
+ allocator. To build crash with this feature enabled, type "make
+ valrind" and then run crash with valgrind as "valgrind crash
+ vmlinux vmcore".
+
All of the alternate build commands above are "sticky" in that the
special "make" targets only have to be entered one time; all subsequent
builds will follow suit.
diff --git a/configure.c b/configure.c
index 7f6d19e..52d3c24 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,12 +1748,24 @@ 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");
return;
}
+ if (valgrind &&
+ file_exists("tools.o") && (unlink("tools.o") < 0)) {
+ perror("tools.o");
+ return;
+ }
+
mode = file_exists("CFLAGS.extra") ? "r+" : "w+";
if ((fp_cflags = fopen("CFLAGS.extra", mode)) == NULL) {
perror("CFLAGS.extra");
@@ -1762,13 +1779,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/help.c b/help.c
index d3427a3..f2b1007 100644
--- a/help.c
+++ b/help.c
@@ -9428,6 +9428,11 @@ README_ENTER_DIRECTORY,
" use either the LZO or snappy compression libraries. To build crash with",
" either or both of those libraries, type \"make lzo\" or \"make
snappy\".",
"",
+" crash supports valgrind Memcheck tool on the crash's custom memory",
+" allocator. To build crash with this feature enabled, type \"make",
+" valrind\" and then run crash with valgrind as \"valgrind crash",
+" vmlinux vmcore\".",
+"",
" All of the alternate build commands above are \"sticky\" in that
the",
" special \"make\" targets only have to be entered one time; all
subsequent",
" builds will follow suit.",
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
5:20 a.m.
New subject: [PATCH v2 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
5:20 a.m.
New subject: [PATCH v2 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
2:52 a.m.
New subject: [PATCH v2 0/3] Add valgrind support for the crash's custom memory
-----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.
v2:
- Unlink tools.o when make valgrind
- Add description about make valgrind in README
Thanks for the update, the v2 patchset looks good.
Applied with fixing the typos Lianbo pointed and a few adjustments:
https://github.com/crash-utility/crash/compare/720279f...9c0c6c1
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 ++++
README | 5 +++++
configure.c | 21 +++++++++++++++++-
help.c | 5 +++++
symbols.c | 10 +++------
tools.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
6 files changed, 107 insertions(+), 10 deletions(-)
--
1.8.3.1
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://listman.redhat.com/mailman/listinfo/crash-utility
1214
days inactive
1215
days old
devel@lists.crash-utility.osci.io
4 comments
2 participants
participants (2)
-
HAGIO KAZUHITO(萩尾 一仁) -
HATAYAMA Daisuke