2012-10-16 01:38:47

by Hyunchul Lee

Subject: [PATCH] perf probe: convert_name_to_addr() allocated the wrong size buffer for a function name

convert_name_to_addr() allocated sizeof(char *) * MAX_PROBE_ARGS
bytes for a function name

Cc: Masami Hiramatsu <[email protected]>
Cc: Srikar Dronamraju <[email protected]>
Signed-off-by: Hyeoncheol Lee <[email protected]>
---
tools/perf/util/probe-event.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 49a256e..bb40ed4 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2352,13 +2352,14 @@ static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec)
free(exec_copy);
}
free(pp->function);
- pp->function = zalloc(sizeof(char *) * MAX_PROBE_ARGS);
+ pp->function = zalloc(sizeof(char) *
+ (3 + sizeof(unsigned long long) * 2));
if (!pp->function) {
ret = -ENOMEM;
pr_warning("Failed to allocate memory by zalloc.\n");
goto out;
}
- e_snprintf(pp->function, MAX_PROBE_ARGS, "0x%llx", vaddr);
+ sprintf(pp->function, "0x%llx", vaddr);
ret = 0;

out:
--
1.7.10.4

2012-10-16 04:20:03

by 平松雅巳 / HIRAMATU，MASAMI

Subject: Re: [PATCH] perf probe: convert_name_to_addr() allocated the wrong size buffer for a function name

(2012/10/16 10:37), Hyeoncheol Lee wrote:
> convert_name_to_addr() allocated sizeof(char *) * MAX_PROBE_ARGS
> bytes for a function name

Yeah, that one was from my laziness...

>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Srikar Dronamraju <[email protected]>
> Signed-off-by: Hyeoncheol Lee <[email protected]>
> ---
> tools/perf/util/probe-event.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 49a256e..bb40ed4 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2352,13 +2352,14 @@ static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec)
> free(exec_copy);
> }
> free(pp->function);
> - pp->function = zalloc(sizeof(char *) * MAX_PROBE_ARGS);
> + pp->function = zalloc(sizeof(char) *
> + (3 + sizeof(unsigned long long) * 2));

Could you comment that this is enough long here?

> if (!pp->function) {
> ret = -ENOMEM;
> pr_warning("Failed to allocate memory by zalloc.\n");
> goto out;
> }
> - e_snprintf(pp->function, MAX_PROBE_ARGS, "0x%llx", vaddr);
> + sprintf(pp->function, "0x%llx", vaddr);

And at least we should use snprintf instead of sprintf...
(I think ret = e_snprintf(...) is better)

> ret = 0;
>
> out:
>

Thank you,

--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2012-10-16 04:37:37

by Srikar Dronamraju

Subject: Re: [PATCH] perf probe: convert_name_to_addr() allocated the wrong size buffer for a function name

* Masami Hiramatsu <[email protected]> [2012-10-16 13:19:57]:

> (2012/10/16 10:37), Hyeoncheol Lee wrote:
> > convert_name_to_addr() allocated sizeof(char *) * MAX_PROBE_ARGS
> > bytes for a function name
>
> Yeah, that one was from my laziness...
>

Guess not your fault, but mine.

> >
> > Cc: Masami Hiramatsu <[email protected]>
> > Cc: Srikar Dronamraju <[email protected]>
> > Signed-off-by: Hyeoncheol Lee <[email protected]>
> > ---
> > tools/perf/util/probe-event.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 49a256e..bb40ed4 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -2352,13 +2352,14 @@ static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec)
> > free(exec_copy);
> > }
> > free(pp->function);
> > - pp->function = zalloc(sizeof(char *) * MAX_PROBE_ARGS);
> > + pp->function = zalloc(sizeof(char) *
> > + (3 + sizeof(unsigned long long) * 2));
>
> Could you comment that this is enough long here?

Also can we move the arith into a macro?

>
> > if (!pp->function) {
> > ret = -ENOMEM;
> > pr_warning("Failed to allocate memory by zalloc.\n");
> > goto out;
> > }
> > - e_snprintf(pp->function, MAX_PROBE_ARGS, "0x%llx", vaddr);
> > + sprintf(pp->function, "0x%llx", vaddr);
>
> And at least we should use snprintf instead of sprintf...
> (I think ret = e_snprintf(...) is better)
>

Agree.

> > ret = 0;
> >
> > out:
> >
>

--
Thanks and Regards
Srikar

2012-10-16 08:13:49

by Hyunchul Lee

Subject: Re: [PATCH] perf probe: convert_name_to_addr() allocated the wrong size buffer for a function name

Hi,

2012/10/16 Masami Hiramatsu <[email protected]>:
> (2012/10/16 10:37), Hyeoncheol Lee wrote:
>> convert_name_to_addr() allocated sizeof(char *) * MAX_PROBE_ARGS
>> bytes for a function name
>
> Yeah, that one was from my laziness...
>
>>
>> Cc: Masami Hiramatsu <[email protected]>
>> Cc: Srikar Dronamraju <[email protected]>
>> Signed-off-by: Hyeoncheol Lee <[email protected]>
>> ---
>> tools/perf/util/probe-event.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 49a256e..bb40ed4 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -2352,13 +2352,14 @@ static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec)
>> free(exec_copy);
>> }
>> free(pp->function);
>> - pp->function = zalloc(sizeof(char *) * MAX_PROBE_ARGS);
>> + pp->function = zalloc(sizeof(char) *
>> + (3 + sizeof(unsigned long long) * 2));
>
> Could you comment that this is enough long here?
>

Because a hexadecimal address that starts with "0x"
will be stored in pp->function. sizeof(unsigned long long) * 2 is
maximum length of hexadecimal number of variable "vaddr"
and 3 bytes are for "0x" and null character.

>> if (!pp->function) {
>> ret = -ENOMEM;
>> pr_warning("Failed to allocate memory by zalloc.\n");
>> goto out;
>> }
>> - e_snprintf(pp->function, MAX_PROBE_ARGS, "0x%llx", vaddr);
>> + sprintf(pp->function, "0x%llx", vaddr);
>
> And at least we should use snprintf instead of sprintf...
> (I think ret = e_snprintf(...) is better)
>

You are right, but I didn't want to write down the length of
"pp->function" again.

>> ret = 0;
>>
>> out:
>>
>
> Thank you,
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: [email protected]
>
>

Thank you very much!

2012-10-16 08:16:30

by Hyunchul Lee

Subject: Re: [PATCH] perf probe: convert_name_to_addr() allocated the wrong size buffer for a function name

Hi,

2012/10/16 Srikar Dronamraju <[email protected]>:
> * Masami Hiramatsu <[email protected]> [2012-10-16 13:19:57]:
>
>> (2012/10/16 10:37), Hyeoncheol Lee wrote:
>> > convert_name_to_addr() allocated sizeof(char *) * MAX_PROBE_ARGS
>> > bytes for a function name
>>
>> Yeah, that one was from my laziness...
>>
>
> Guess not your fault, but mine.
>
>> >
>> > Cc: Masami Hiramatsu <[email protected]>
>> > Cc: Srikar Dronamraju <[email protected]>
>> > Signed-off-by: Hyeoncheol Lee <[email protected]>
>> > ---
>> > tools/perf/util/probe-event.c | 5 +++--
>> > 1 file changed, 3 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> > index 49a256e..bb40ed4 100644
>> > --- a/tools/perf/util/probe-event.c
>> > +++ b/tools/perf/util/probe-event.c
>> > @@ -2352,13 +2352,14 @@ static int convert_name_to_addr(struct perf_probe_event *pev, const char *exec)
>> > free(exec_copy);
>> > }
>> > free(pp->function);
>> > - pp->function = zalloc(sizeof(char *) * MAX_PROBE_ARGS);
>> > + pp->function = zalloc(sizeof(char) *
>> > + (3 + sizeof(unsigned long long) * 2));
>>
>> Could you comment that this is enough long here?
>
> Also can we move the arith into a macro?
>

I will do.

>>
>> > if (!pp->function) {
>> > ret = -ENOMEM;
>> > pr_warning("Failed to allocate memory by zalloc.\n");
>> > goto out;
>> > }
>> > - e_snprintf(pp->function, MAX_PROBE_ARGS, "0x%llx", vaddr);
>> > + sprintf(pp->function, "0x%llx", vaddr);
>>
>> And at least we should use snprintf instead of sprintf...
>> (I think ret = e_snprintf(...) is better)
>>
>
> Agree.

Yes

>
>> > ret = 0;
>> >
>> > out:
>> >
>>
>
> --
> Thanks and Regards
> Srikar
>

Thank you for your comment.