Re: [PATCH] perf ksymbol: fix memory leak: decrease refcount of map and dso

Em Thu, Jun 03, 2021 at 09:26:40PM -0700, Ian Rogers escreveu:

On Wed, Jun 2, 2021 at 4:15 PM Riccardo Mancini [off-list ref] wrote:

quoted

+++ b/tools/perf/util/machine.c

@@ -776,6 +776,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
                if (dso) {
                        dso->kernel = DSO_SPACE__KERNEL;
                        map = map__new2(0, dso);
+                       dso__put(dso);

Will this cause 2 puts if the map allocation fails? Perhaps this
should be "if (map) dso__put(dso);".

I think its just a matter of removing the put in the error path, i.e.
the patch becomes what is at the end of this message.

I.e. if map__new2() fails, we want to drop the dso reference, and if it
works, we already have a reference to it, obtained in map__new2().

But looking at this code now I realize that maps__find() should grab a
refcount for the map it returns, because in this
machine__process_ksymbol_register() function we use reference that 'map'
after the if block, i.e. we use it if it came from maps__find() or if we
created it machine__process_ksymbol_register, so there is a possible
race where other thread removes it from the list and map__put()s it
ending up in map__delete() while we still use it in
machine__process_ksymbol_register(), right?

- Arnaldo

quoted

                }

quoted

                if (!dso || !map) {

@@ -792,6 +793,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
                map->start = event->ksymbol.addr;
                map->end = map->start + event->ksymbol.len;
                maps__insert(&machine->kmaps, map);
+               map__put(map);
                dso__set_loaded(dso);

quoted

                if (is_bpf_image(event->ksymbol.name)) {

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 3ff4936a15a42f74..da19be7da284c250 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c

@@ -776,10 +776,10 @@ static int machine__process_ksymbol_register(struct machine *machine,
 		if (dso) {
 			dso->kernel = DSO_SPACE__KERNEL;
 			map = map__new2(0, dso);
+			dso__put(dso);
 		}
 
 		if (!dso || !map) {
-			dso__put(dso);
 			return -ENOMEM;
 		}
 

@@ -792,6 +792,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
 		map->start = event->ksymbol.addr;
 		map->end = map->start + event->ksymbol.len;
 		maps__insert(&machine->kmaps, map);
+		map__put(map);
 		dso__set_loaded(dso);
 
 		if (is_bpf_image(event->ksymbol.name)) {