Re: [PATCH] perf trace: Format fields with hex specifiers in print_fmt as hexadecimal

On Fri, Jul 31, 2026 at 10:12:03PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] field_has_hex_fmt() prematurely truncates the format string search when it encounters escaped quotes.
> --
> 
> --- Patch [1]: [PATCH] perf trace: Format fields with hex specifiers in print_fmt as hexadecimal ---
> commit 1d9edf5adf3cf666d7e9393e5db036cd73d6656a
> Author: Aaron Tomlin <[email protected]>
> 
> perf trace: Format fields with hex specifiers in print_fmt as hexadecimal
> 
> This commit introduces field_has_hex_fmt to inspect tracepoint print_fmt
> strings. If a field uses hexadecimal specifiers, it assigns SCA_PTR formatting
> to display the value in hexadecimal by default.
> 
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index ba0f8749fc7d7..b0ce9c2a24e57 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> [ ... ]
> > @@ -2098,6 +2098,37 @@ static bool is_internal_field(struct tep_format_field *field)
> >  	return !strcmp(field->type, "__data_loc char[]");
> >  }
> >  
> > +static bool field_has_hex_fmt(struct tep_format_field *field, int len)
> > +{
> > +	const char *fmt, *pos, *end;
> > +
> > +	if (!field || !field->event || !field->event->print_fmt.format)
> > +		return false;
> > +
> > +	fmt = field->event->print_fmt.format;
> > +
> > +	/* Limit scanning strictly to the quoted printf format string */
> > +	if (*fmt == '"') {
> > +		fmt++;
> > +		end = strchr(fmt, '"');
> 
> [Severity: Medium]
> Does this parsing correctly handle escaped quotes inside the format string?
> 
> If a tracepoint format string contains an escaped quote, this strchr call
> will incorrectly match it as the end of the format string, causing the end
> pointer to be prematurely truncated.
> 
> > +	} else {
> > +		end = strchr(fmt, ',');
> > +	}
> > +
> > +	for (pos = strstr(fmt, field->name); pos && (!end || pos < end); pos = strstr(pos + 1, field->name)) {
> 
> [Severity: Medium]
> Because the end pointer can be truncated by an escaped quote earlier, won't
> the pos < end loop condition evaluate to false for any fields positioned after
> that escaped quote?
> 
> This seems like it would skip hex format detection for those subsequent fields,
> causing them to fall back to default decimal formatting.
> 
> > +		if (pos == fmt || !(isalnum(pos[-1]) || pos[-1] == '_')) {
> > +			const char *after = pos + len;
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Hi Arnaldo, Namhyung, Jiri, Ian,

I have updated the field_has_hex_fmt() function to use an escape-aware
scanner that explicitly skips backslash-escaped characters when seeking the
closing quote. I shall send through a v2 containing this fix shortly.

Kind regards,
-- 
Aaron Tomlin