diff mbox series

[bpf-next,v4,2/6] libbpf: Fix memory leak in parse_usdt_arg()

Message ID 20221011120108.782373-3-xukuohai@huaweicloud.com (mailing list archive)
State Accepted
Commit 0dc9254e03704c75f2ebc9cbef2ce4de83fba603
Headers show
Series Fix bugs found by ASAN when running selftests | expand

Commit Message

Xu Kuohai Oct. 11, 2022, 12:01 p.m. UTC 
From: Xu Kuohai <xukuohai@huawei.com>

In the arm64 version of parse_usdt_arg(), when sscanf returns 2, reg_name
is allocated but not freed. Fix it.

Fixes: 0f8619929c57 ("libbpf: Usdt aarch64 arg parsing support")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 tools/lib/bpf/usdt.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Andrii Nakryiko Oct. 13, 2022, 3:47 p.m. UTC | #1 
On Tue, Oct 11, 2022 at 4:43 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>
> From: Xu Kuohai <xukuohai@huawei.com>
>
> In the arm64 version of parse_usdt_arg(), when sscanf returns 2, reg_name
> is allocated but not freed. Fix it.
>
> Fixes: 0f8619929c57 ("libbpf: Usdt aarch64 arg parsing support")
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  tools/lib/bpf/usdt.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
> index e83b497c2245..49f3c3b7f609 100644
> --- a/tools/lib/bpf/usdt.c
> +++ b/tools/lib/bpf/usdt.c
> @@ -1348,25 +1348,23 @@ static int calc_pt_regs_off(const char *reg_name)
>
>  static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg)
>  {
> -       char *reg_name = NULL;
> +       char reg_name[16];
>         int arg_sz, len, reg_off;
>         long off;
>
> -       if (sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, &reg_name, &off, &len) == 3) {
> +       if (sscanf(arg_str, " %d @ \[ %15[a-z0-9], %ld ] %n", &arg_sz, reg_name, &off, &len) == 3) {

It would be nice to do the same change for other architectures where
it makes sense and avoid having to deal with unnecessary memory
allocations. Please send follow up patches with similar changes for
other implementations of parse_usdt_arg. Thanks.


>                 /* Memory dereference case, e.g., -4@[sp, 96] */
>                 arg->arg_type = USDT_ARG_REG_DEREF;
>                 arg->val_off = off;
>                 reg_off = calc_pt_regs_off(reg_name);
> -               free(reg_name);
>                 if (reg_off < 0)
>                         return reg_off;
>                 arg->reg_off = reg_off;
> -       } else if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
> +       } else if (sscanf(arg_str, " %d @ \[ %15[a-z0-9] ] %n", &arg_sz, reg_name, &len) == 2) {
>                 /* Memory dereference case, e.g., -4@[sp] */
>                 arg->arg_type = USDT_ARG_REG_DEREF;
>                 arg->val_off = 0;
>                 reg_off = calc_pt_regs_off(reg_name);
> -               free(reg_name);
>                 if (reg_off < 0)
>                         return reg_off;
>                 arg->reg_off = reg_off;
> @@ -1375,12 +1373,11 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>                 arg->arg_type = USDT_ARG_CONST;
>                 arg->val_off = off;
>                 arg->reg_off = 0;
> -       } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, &reg_name, &len) == 2) {
> +       } else if (sscanf(arg_str, " %d @ %15[a-z0-9] %n", &arg_sz, reg_name, &len) == 2) {
>                 /* Register read case, e.g., -8@x4 */
>                 arg->arg_type = USDT_ARG_REG;
>                 arg->val_off = 0;
>                 reg_off = calc_pt_regs_off(reg_name);
> -               free(reg_name);
>                 if (reg_off < 0)
>                         return reg_off;
>                 arg->reg_off = reg_off;
> --
> 2.30.2
>
Xu Kuohai Oct. 14, 2022, 1:53 a.m. UTC | #2 
On 10/13/2022 11:47 PM, Andrii Nakryiko wrote:
> On Tue, Oct 11, 2022 at 4:43 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> From: Xu Kuohai <xukuohai@huawei.com>
>>
>> In the arm64 version of parse_usdt_arg(), when sscanf returns 2, reg_name
>> is allocated but not freed. Fix it.
>>
>> Fixes: 0f8619929c57 ("libbpf: Usdt aarch64 arg parsing support")
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   tools/lib/bpf/usdt.c | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
>> index e83b497c2245..49f3c3b7f609 100644
>> --- a/tools/lib/bpf/usdt.c
>> +++ b/tools/lib/bpf/usdt.c
>> @@ -1348,25 +1348,23 @@ static int calc_pt_regs_off(const char *reg_name)
>>
>>   static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg)
>>   {
>> -       char *reg_name = NULL;
>> +       char reg_name[16];
>>          int arg_sz, len, reg_off;
>>          long off;
>>
>> -       if (sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, &reg_name, &off, &len) == 3) {
>> +       if (sscanf(arg_str, " %d @ \[ %15[a-z0-9], %ld ] %n", &arg_sz, reg_name, &off, &len) == 3) {
> 
> It would be nice to do the same change for other architectures where
> it makes sense and avoid having to deal with unnecessary memory
> allocations. Please send follow up patches with similar changes for
> other implementations of parse_usdt_arg. Thanks.
>

ok, will do

> 
>>                  /* Memory dereference case, e.g., -4@[sp, 96] */
>>                  arg->arg_type = USDT_ARG_REG_DEREF;
>>                  arg->val_off = off;
>>                  reg_off = calc_pt_regs_off(reg_name);
>> -               free(reg_name);
>>                  if (reg_off < 0)
>>                          return reg_off;
>>                  arg->reg_off = reg_off;
>> -       } else if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
>> +       } else if (sscanf(arg_str, " %d @ \[ %15[a-z0-9] ] %n", &arg_sz, reg_name, &len) == 2) {
>>                  /* Memory dereference case, e.g., -4@[sp] */
>>                  arg->arg_type = USDT_ARG_REG_DEREF;
>>                  arg->val_off = 0;
>>                  reg_off = calc_pt_regs_off(reg_name);
>> -               free(reg_name);
>>                  if (reg_off < 0)
>>                          return reg_off;
>>                  arg->reg_off = reg_off;
>> @@ -1375,12 +1373,11 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>>                  arg->arg_type = USDT_ARG_CONST;
>>                  arg->val_off = off;
>>                  arg->reg_off = 0;
>> -       } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, &reg_name, &len) == 2) {
>> +       } else if (sscanf(arg_str, " %d @ %15[a-z0-9] %n", &arg_sz, reg_name, &len) == 2) {
>>                  /* Register read case, e.g., -8@x4 */
>>                  arg->arg_type = USDT_ARG_REG;
>>                  arg->val_off = 0;
>>                  reg_off = calc_pt_regs_off(reg_name);
>> -               free(reg_name);
>>                  if (reg_off < 0)
>>                          return reg_off;
>>                  arg->reg_off = reg_off;
>> --
>> 2.30.2
>>
> .
diff mbox series

Patch

diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
index e83b497c2245..49f3c3b7f609 100644
--- a/tools/lib/bpf/usdt.c
+++ b/tools/lib/bpf/usdt.c
@@ -1348,25 +1348,23 @@  static int calc_pt_regs_off(const char *reg_name)
 
 static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg)
 {
-	char *reg_name = NULL;
+	char reg_name[16];
 	int arg_sz, len, reg_off;
 	long off;
 
-	if (sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, &reg_name, &off, &len) == 3) {
+	if (sscanf(arg_str, " %d @ \[ %15[a-z0-9], %ld ] %n", &arg_sz, reg_name, &off, &len) == 3) {
 		/* Memory dereference case, e.g., -4@[sp, 96] */
 		arg->arg_type = USDT_ARG_REG_DEREF;
 		arg->val_off = off;
 		reg_off = calc_pt_regs_off(reg_name);
-		free(reg_name);
 		if (reg_off < 0)
 			return reg_off;
 		arg->reg_off = reg_off;
-	} else if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
+	} else if (sscanf(arg_str, " %d @ \[ %15[a-z0-9] ] %n", &arg_sz, reg_name, &len) == 2) {
 		/* Memory dereference case, e.g., -4@[sp] */
 		arg->arg_type = USDT_ARG_REG_DEREF;
 		arg->val_off = 0;
 		reg_off = calc_pt_regs_off(reg_name);
-		free(reg_name);
 		if (reg_off < 0)
 			return reg_off;
 		arg->reg_off = reg_off;
@@ -1375,12 +1373,11 @@  static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
 		arg->arg_type = USDT_ARG_CONST;
 		arg->val_off = off;
 		arg->reg_off = 0;
-	} else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, &reg_name, &len) == 2) {
+	} else if (sscanf(arg_str, " %d @ %15[a-z0-9] %n", &arg_sz, reg_name, &len) == 2) {
 		/* Register read case, e.g., -8@x4 */
 		arg->arg_type = USDT_ARG_REG;
 		arg->val_off = 0;
 		reg_off = calc_pt_regs_off(reg_name);
-		free(reg_name);
 		if (reg_off < 0)
 			return reg_off;
 		arg->reg_off = reg_off;