| Message ID | 20240626033949.1017381-1-namhyung@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libtraceevent: Fix a double free in process_op() | expand |
On Tue, 25 Jun 2024 20:39:49 -0700 Namhyung Kim <namhyung@gmail.com> wrote: > When process_cond() failed, it freed the token but didn't reset the > arg->op.op to NULL. So it tried to free the arg->op.op again from > free_arg() from the caller and resulted in a double free. > > Signed-off-by: Namhyung Kim <namhyung@google.com> > --- > src/event-parse.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/event-parse.c b/src/event-parse.c > index 9f0522c..c327917 100644 > --- a/src/event-parse.c > +++ b/src/event-parse.c > @@ -2375,8 +2375,11 @@ 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); > + if (type == TEP_EVENT_ERROR) { > + /* arg->op.op (= token) will be freed at out_free */ > + arg->op.op = NULL; > + goto out_free; > + } > > } else if (strcmp(token, ">>") == 0 || > strcmp(token, "<<") == 0 || Hmm, so this fixes: 76a0eb8d5a20c ("libtraceevent: Fix event-parse memory leak in process_cond") Which added: + if (type == TEP_EVENT_ERROR) + free(token); because of a memory leak. But if token is arg->op, and we should be freeing that arg, how did it end up being a leak? Because args->op.left is allocated. Whatever frees that, should also have freed args->op.op. I'm getting a feeling that we are missing the real bug. Perhaps the real fix was: diff --git a/src/event-parse.c b/src/event-parse.c index 9f0522c8d9e4..c7ffad116c3f 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); } else if (strcmp(token, ">>") == 0 || strcmp(token, "<<") == 0 || @@ -2587,6 +2585,9 @@ static int alloc_and_process_delim(struct tep_event *event, char *next_token, if (type == TEP_EVENT_OP) { type = process_op(event, field, &token); + if (type == TEP_EVENT_ERROR) + goto out_error; + if (consolidate_op_arg(field) < 0) type = TEP_EVENT_ERROR; -- Steve
On Fri, 19 Jul 2024 16:32:41 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> Perhaps the real fix was:
Or better yet, something a bit cleaner, as I found another location that
needs an update.
diff --git a/src/event-parse.c b/src/event-parse.c
index 9f0522c8d9e4..d44da8baf087 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -2197,21 +2197,24 @@ static int set_op_prio(struct tep_print_arg *arg)
return arg->op.prio;
}
-static int consolidate_op_arg(struct tep_print_arg *arg)
+static int consolidate_op_arg( enum tep_event_type type, struct tep_print_arg *arg)
{
unsigned long long val, left, right;
int ret = 0;
+ if (type == TEP_EVENT_ERROR)
+ return -1;
+
if (arg->type != TEP_PRINT_OP)
return 0;
if (arg->op.left)
- ret = consolidate_op_arg(arg->op.left);
+ ret = consolidate_op_arg(type, arg->op.left);
if (ret < 0)
return ret;
if (arg->op.right)
- ret = consolidate_op_arg(arg->op.right);
+ ret = consolidate_op_arg(type, arg->op.right);
if (ret < 0)
return ret;
@@ -2375,8 +2378,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);
} else if (strcmp(token, ">>") == 0 ||
strcmp(token, "<<") == 0 ||
@@ -2587,7 +2588,7 @@ static int alloc_and_process_delim(struct tep_event *event, char *next_token,
if (type == TEP_EVENT_OP) {
type = process_op(event, field, &token);
- if (consolidate_op_arg(field) < 0)
+ if (consolidate_op_arg(type, field) < 0)
type = TEP_EVENT_ERROR;
if (type == TEP_EVENT_ERROR)
@@ -3818,7 +3819,7 @@ static int event_read_print_args(struct tep_event *event, struct tep_print_arg *
type = process_op(event, arg, &token);
free_token(token);
- if (consolidate_op_arg(arg) < 0)
+ if (consolidate_op_arg(type, arg) < 0)
type = TEP_EVENT_ERROR;
if (type == TEP_EVENT_ERROR) {
-- Steve
Hi Steve, On Fri, Jul 19, 2024 at 1:39 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 19 Jul 2024 16:32:41 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > Perhaps the real fix was: > > Or better yet, something a bit cleaner, as I found another location that > needs an update. Looks good, I'll send v2 with this. Thanks, Namhyung > > diff --git a/src/event-parse.c b/src/event-parse.c > index 9f0522c8d9e4..d44da8baf087 100644 > --- a/src/event-parse.c > +++ b/src/event-parse.c > @@ -2197,21 +2197,24 @@ static int set_op_prio(struct tep_print_arg *arg) > return arg->op.prio; > } > > -static int consolidate_op_arg(struct tep_print_arg *arg) > +static int consolidate_op_arg( enum tep_event_type type, struct tep_print_arg *arg) > { > unsigned long long val, left, right; > int ret = 0; > > + if (type == TEP_EVENT_ERROR) > + return -1; > + > if (arg->type != TEP_PRINT_OP) > return 0; > > if (arg->op.left) > - ret = consolidate_op_arg(arg->op.left); > + ret = consolidate_op_arg(type, arg->op.left); > if (ret < 0) > return ret; > > if (arg->op.right) > - ret = consolidate_op_arg(arg->op.right); > + ret = consolidate_op_arg(type, arg->op.right); > if (ret < 0) > return ret; > > @@ -2375,8 +2378,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); > > } else if (strcmp(token, ">>") == 0 || > strcmp(token, "<<") == 0 || > @@ -2587,7 +2588,7 @@ static int alloc_and_process_delim(struct tep_event *event, char *next_token, > if (type == TEP_EVENT_OP) { > type = process_op(event, field, &token); > > - if (consolidate_op_arg(field) < 0) > + if (consolidate_op_arg(type, field) < 0) > type = TEP_EVENT_ERROR; > > if (type == TEP_EVENT_ERROR) > @@ -3818,7 +3819,7 @@ static int event_read_print_args(struct tep_event *event, struct tep_print_arg * > type = process_op(event, arg, &token); > free_token(token); > > - if (consolidate_op_arg(arg) < 0) > + if (consolidate_op_arg(type, arg) < 0) > type = TEP_EVENT_ERROR; > > if (type == TEP_EVENT_ERROR) { > > > -- Steve
diff --git a/src/event-parse.c b/src/event-parse.c index 9f0522c..c327917 100644 --- a/src/event-parse.c +++ b/src/event-parse.c @@ -2375,8 +2375,11 @@ 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); + if (type == TEP_EVENT_ERROR) { + /* arg->op.op (= token) will be freed at out_free */ + arg->op.op = NULL; + goto out_free; + } } else if (strcmp(token, ">>") == 0 || strcmp(token, "<<") == 0 ||
When process_cond() failed, it freed the token but didn't reset the arg->op.op to NULL. So it tried to free the arg->op.op again from free_arg() from the caller and resulted in a double free. Signed-off-by: Namhyung Kim <namhyung@google.com> --- src/event-parse.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)