[Crash-utility] Re: [PATCH 3/3] Enable resolving mangled Rust symbol in lockless ring buffer

On Mon, Sep 8, 2025 at 2:55 PM lijiang <lijiang(a)redhat.com&gt; wrote: > > Hi, Tao > Thank you for the review. > > On Thu, Sep 4, 2025 at 1:08 PM Tao Liu <ltao(a)redhat.com&gt; wrote: >> >> Hi lianbo, >> >> Just some nits. >> >> On Wed, Aug 27, 2025 at 4:10 PM Lianbo Jiang <lijiang(a)redhat.com&gt; wrote: >> > >> > Without the patch: >> > crash> log >> > ... >> > [ 2174.308966] Call Trace: >> > [ 2174.311693] <TASK> >> > [ 2174.314033] dump_stack_lvl+0x5d/0x80 >> > [ 2174.318125] panic+0x156/0x32a >> > [ 2174.321539] _RNvCscb18lrEyTSA_10rust_panic10area_in_hp+0xf7/0x120 [rust_panic] >> > [ 2174.329700] ? console_unlock+0x9c/0x140 >> > [ 2174.334080] ? irq_work_queue+0x2d/0x50 >> > [ 2174.338352] ? __pfx_init_module+0x10/0x10 [rust_panic] >> > [ 2174.344183] _RNvMCscb18lrEyTSA_10rust_panicNtB2_10HelloPanic8step_two+0x20/0xe0 [rust_panic] >> > [ 2174.353698] ? _printk+0x6b/0x90 >> > ... >> > >> > With the patch: >> > crash> log >> > ... >> > [ 2174.308966] Call Trace: >> > [ 2174.311693] <TASK> >> > [ 2174.314033] dump_stack_lvl+0x5d/0x80 >> > [ 2174.318125] panic+0x156/0x32a >> > [ 2174.321539] rust_panic::area_in_hp+0xf7/0x120 [rust_panic] >> > [ 2174.329700] ? console_unlock+0x9c/0x140 >> > [ 2174.334080] ? irq_work_queue+0x2d/0x50 >> > [ 2174.338352] ? __pfx_init_module+0x10/0x10 [rust_panic] >> > [ 2174.344183] <rust_panic::HelloPanic>::step_two+0x20/0xe0 [rust_panic] >> > [ 2174.353698] ? _printk+0x6b/0x90 >> > ... >> > >> > Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com&gt; >> > --- >> > Makefile | 2 +- >> > printk.c | 28 +++++++++++++++++++++++++++- >> > 2 files changed, 28 insertions(+), 2 deletions(-) >> > >> > diff --git a/Makefile b/Makefile >> > index fc1c9588dcfa..b277129f6df2 100644 >> > --- a/Makefile >> > +++ b/Makefile >> > @@ -421,7 +421,7 @@ kernel.o: ${GENERIC_HFILES} kernel.c >> > ${CC} -c ${CRASH_CFLAGS} kernel.c -I${BFD_DIRECTORY} -I${GDB_INCLUDE_DIRECTORY} ${WARNING_OPTIONS} ${WARNING_ERROR} >> > >> > printk.o: ${GENERIC_HFILES} printk.c >> > - ${CC} -c ${CRASH_CFLAGS} printk.c ${WARNING_OPTIONS} ${WARNING_ERROR} >> > + ${CC} -c ${CRASH_CFLAGS} printk.c -I${GDB_INCLUDE_DIRECTORY} ${WARNING_OPTIONS} ${WARNING_ERROR} >> > >> > gdb_interface.o: ${GENERIC_HFILES} gdb_interface.c >> > ${CC} -c ${CRASH_CFLAGS} gdb_interface.c ${WARNING_OPTIONS} ${WARNING_ERROR} >> > diff --git a/printk.c b/printk.c >> > index 95db7e607e4c..6a87a790b892 100644 >> > --- a/printk.c >> > +++ b/printk.c >> > @@ -1,5 +1,6 @@ >> > #include "defs.h" >> > #include <ctype.h> >> > +#include "demangle.h" >> > >> > /* convenience struct for passing many values to helper functions */ >> > struct prb_map { >> > @@ -205,10 +206,35 @@ dump_record(struct prb_map *m, unsigned long id, int msg_flags) >> > if (*p == '\n') >> > fprintf(fp, "\n%s", space(ilen)); >> > else if (isprint(*p) || isspace(*p)) >> > - fputc(*p, fp); >> > + sprintf(&buf[i], "%c", *p); >> > else >> > fputc('.', fp); >> > } >> > + /* >> > + * Try to demangle a mangled Rust symbol(calltrace) from log buffer >> > + */ >> > + char *p1 = strstr(buf, "_R"); >> > + if (!p1) >> > + p1 = strstr(buf, "_ZN"); >> > + char *p2 = strchrnul(buf, '+'); >> > + if (p1 && p2) { >> > + char mangled[BUFSIZE] = {0}; >> > + char demangled[BUFSIZE] = {0}; >> > + char *res; >> > + size_t slen = p1 - buf; >> > + >> > + if (slen) >> > + memcpy(demangled, buf, slen); >> > + >> > + memcpy(mangled, p1, p2-p1); >> > + res = rust_demangle(mangled, DMGL_RUST); >> > + if (res) { >> > + sprintf(demangled+slen, "%s%s", res,p2); >> >> The (de)mangled buf size is 1500. First copied slen chars into >> demangled, then sprintf res and p2 into demangled buf without length >> check.Though this might be a rare case, but I guess a snprintf with a > > > Theoretically yes, but actually I never saw a very long string in the printk buffer because it would have bad readability. > Anyway, how about the following changes? > > diff --git a/printk.c b/printk.c > index 6a87a790b892..c547ea8c3f9f 100644 > --- a/printk.c > +++ b/printk.c > @@ -202,6 +202,11 @@ dump_record(struct prb_map *m, unsigned long id, int msg_flags) > > text = m->text_data + begin; > > + if (text_len > BUFSIZE) { > + error(WARNING, "\nThe messages could be truncated!\n"); > + text_len = BUFSIZE; > + } > + OK. > for (i = 0, p = text; i < text_len; i++, p++) { > if (*p == '\n') > fprintf(fp, "\n%s", space(ilen)); > @@ -229,7 +234,7 @@ dump_record(struct prb_map *m, unsigned long id, int msg_flags) > memcpy(mangled, p1, p2-p1); > res = rust_demangle(mangled, DMGL_RUST); > if (res) { > - sprintf(demangled+slen, "%s%s", res,p2); > + snprintf(demangled+slen, BUFSIZE, "%s%s", res, p2); I think it should be: snprintf(demangled+slen, BUFSIZE - slen, "%s%s", res, p2); Because demangled[BUFSIZE], with slen offset from demangled, the left space is BUFSIZE - slen.

> fprintf(fp, "%s",demangled); > free(res); > } > > Thanks > Lianbo > >> length check is safer for preventing overflow, since we are within >> ringbuffer buf, I'm not sure how much slen it may be. >> >> The rest of the patch are good to me. >> >> Thanks, >> Tao Liu >> >> > + fprintf(fp, "%s",demangled); >> > + free(res); >> > + } >> > + } else >> > + fprintf(fp, "%s", buf); >> > >> > if (msg_flags & SHOW_LOG_DICT) { >> > text = info + OFFSET(printk_info_dev_info) + >> > -- >> > 2.50.1 >> > -- >> > Crash-utility mailing list -- devel(a)lists.crash-utility.osci.io >> > To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io >> > https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/ >> > Contribution Guidelines: https://github.com/crash-utility/crash/wiki >>