On Mon, 1 Jul 2024 10:24:46 +0800
Tw <tw19881113@xxxxxxxxx> wrote:
> The corner case is that when we encounter a invalid right argument of a condition operation.
> Currently, we free token immediately, but it will also be freed when free `arg->op.op`.
>
> BTW, the crash calltrace as follows:
OK, so someone else sent a patch for the same bug, but the cause of the bug
ended up being something different.
https://lore.kernel.org/linux-trace-devel/20240719163242.690f4c0e@xxxxxxxxxxxxxxxxxx/
https://lore.kernel.org/linux-trace-devel/20240722161917.991077-1-namhyung@xxxxxxxxxx
>
> Program received signal SIGSEGV, Segmentation fault.
> get_meta (p=<optimized out>) at /home/tw/code/zig/build/stage3/lib/zig/libc/musl/src/malloc/mallocng/meta.h:141
> 141 /home/tw/code/zig/build/stage3/lib/zig/libc/musl/src/malloc/mallocng/meta.h: No such file or directory.
> (gdb) bt
> at /home/tw/code/zig/build/stage3/lib/zig/libc/musl/src/malloc/mallocng/meta.h:141
> at /home/tw/code/zig/build/stage3/lib/zig/libc/musl/src/malloc/mallocng/free.c:105
> at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:1128
> list@entry=0x7ff7b18768)
> at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:1417
> at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:3895
> sys=<optimized out>)
> at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:7824
> size=<optimized out>, sys=sys@entry=0x7ff7ff51c0 "kvm")
> at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:7882
> buf=0x7ff7b0c610 "kvm_sys_access", size=549616874800, sys=0x7fffffe0b2 "me", sys@entry=0x7ff7ff51c0 "kvm")
> at /tmp/.cache/zig/p/12207a2e4477bf4414e7df3eb2172c698ab916695a0d3eefbf16f65b0c969dd81184/src/event-parse.c:7945
> tracing_dir=tracing_dir@entry=0x7ff7ffc660 "/sys/kernel/tracing", system=system@entry=0x7ff7ff51c0 "kvm",
> check=false)
> at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1062
> system=0x7ff7ff51c0 "kvm")
> at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1084
> tep=tep@entry=0x7ff7ffc830, sys_names=sys_names@entry=0x0, parsing_failures=0x0,
> parsing_failures@entry=0x7fffffe7b0)
> at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1284
> sys_names@entry=0x7ffffff880)
> at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1355
> tracing_dir=0x6500006c6f6f62 <error: Cannot access memory at address 0x6500006c6f6f62>)
> at /tmp/.cache/zig/p/1220c1c006cbf05434d240b65f343c84f3d7f837fbef31f2cade733ec911cc3ed76b/src/tracefs-events.c:1377
>
> Signed-off-by: Tw <tw19881113@xxxxxxxxx>
> ---
> src/event-parse.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/src/event-parse.c b/src/event-parse.c
> index 9f0522c..1f51ee9 100644
> --- a/src/event-parse.c
> +++ b/src/event-parse.c
> @@ -2375,8 +2375,6 @@ process_op(struct tep_event *event, struct tep_print_arg *arg, char **tok)
>
> /* it will set arg->op.right */
> type = process_cond(event, arg, tok);
> - if (type == TEP_EVENT_ERROR)
> - free(token);
This should be removed, but how to fix the bug for the reason it was added
in the first place must be done too.
>
> } else if (strcmp(token, ">>") == 0 ||
> strcmp(token, "<<") == 0 ||
> @@ -3787,7 +3785,7 @@ static int event_read_print_args(struct tep_event *event, struct tep_print_arg *
> {
> enum tep_event_type type = TEP_EVENT_ERROR;
> struct tep_print_arg *arg;
> - char *token;
> + char *token = NULL;
This is unneeded because token is assigned later:
if (type == TEP_EVENT_NEWLINE) {
type = read_token_item(event->tep, &token);
continue;
}
where read_token_item() has:
static enum tep_event_type read_token_item(struct tep_handle *tep, char **tok)
{
enum tep_event_type type;
for (;;) {
type = __read_token(tep, tok);
And:
static enum tep_event_type __read_token(struct tep_handle *tep, char **tok)
{
char buf[BUFSIZ];
int ch, last_ch, quote_ch, next_ch;
int i = 0;
int tok_size = 0;
enum tep_event_type type;
*tok = NULL;
So that does the "token = NULL", and if that first if doesn't get hit in
event_read_print_args(), it then does:
type = process_arg(event, arg, &token);
Where process_arg() is:
static enum tep_event_type
process_arg(struct tep_event *event, struct tep_print_arg *arg, char **tok)
{
enum tep_event_type type;
char *token;
type = read_token(event->tep, &token);
Which assigns token.
Hence, initializing "token" is not needed.
> int args = 0;
>
> do {
> @@ -3817,6 +3815,7 @@ static int event_read_print_args(struct tep_event *event, struct tep_print_arg *
> if (type == TEP_EVENT_OP) {
> type = process_op(event, arg, &token);
> free_token(token);
> + token = NULL;
>
> if (consolidate_op_arg(arg) < 0)
> type = TEP_EVENT_ERROR;
Now this part does have the bug. But that's fixed in the links I showed.
The key thing was to look at the result of "type". It was not about freeing
token, because if the type returned was TEP_EVENT_ERROR, that
consolidate_op_arg() should never had been called.
Thanks,
-- Steve
|