Skip to contentSkip to conversation

perf config: fix caching and memory leak in perf_home_perfconfig

PATCH In review Riccardo Mancini 2 participants 4 messages Aug 20, 2021 – Aug 24, 2021 perf events

lore
Share & tools
lore

Riccardo Mancini

author
#0 lore raw

PATCH 1/1 perf config: fix caching and memory leak in perf_home_perfconfig

Acaict, perf_home_perfconfig is supposed to cache the result of home_perfconfig, which returns the default location of perfconfig for the user, given the HOME environment variable.
However, the current implementation calls home_perfconfig every time perf_home_perfconfig is called (so no caching is actually performed), replacing the previous pointer, thus also causing a memory leak.

This patch adds a check of whether either config or failed is set and, in that case, directly returns config without calling home_perfconfig at each invocation.

Cc Jiri Olsa <[email protected]>
Fixes f5f03e19ce14fc31 ("perf config: Add perf_home_perfconfig function")
Signed-off-by Riccardo Mancini <[email protected]>
---
 tools/perf/util/config.c | 3 +++
 1 file changed, 3 insertions(+)
tools/perf/util/config.c +3 -0
index 63d472b336de21d4..6ab670cdf512507e 100644
@@ -581,6 +581,9 @@ const char *perf_home_perfconfig(void)
 	static const char *config;
 	static bool failed;
 
+	if (config || failed)
+		return config;
+
 	config = failed ? NULL : home_perfconfig();
 	if (!config)
 		failed = true;
Signature · 1 line
2.31.1
maintainer
#1 lore raw

Em Fri, Aug 20, 2021 at 03:08:17PM +0200, Riccardo Mancini escreveu:

Quoted 23 lines

Acaict, perf_home_perfconfig is supposed to cache the result of home_perfconfig, which returns the default location of perfconfig for the user, given the HOME environment variable.
However, the current implementation calls home_perfconfig every time perf_home_perfconfig is called (so no caching is actually performed), replacing the previous pointer, thus also causing a memory leak.

This patch adds a check of whether either config or failed is set and, in that case, directly returns config without calling home_perfconfig at each invocation.

Cc Jiri Olsa <[email protected]>
Fixes f5f03e19ce14fc31 ("perf config: Add perf_home_perfconfig function")
Signed-off-by Riccardo Mancini <[email protected]>
---
 tools/perf/util/config.c | 3 +++
 1 file changed, 3 insertions(+)
tools/perf/util/config.c +0 -0
index 63d472b336de21d4..6ab670cdf512507e 100644

tools/perf/util/config.c const char *perf_home_perfconfig(void) · line 587

Quoted 7 lines
tools/perf/util/config.c +3 -0
@@ -581,6 +581,9 @@ const char *perf_home_perfconfig(void)
 	static const char *config;
 	static bool failed;
 
+	if (config || failed)
+		return config;
+
 	config = failed ? NULL : home_perfconfig();

humm, why keep the above failed test then?

tools/perf/util/config.c const char *perf_home_perfconfig(void) · line 589

Quoted 2 lines
tools/perf/util/config.c +0 -0
 	if (!config)
 		failed = true;

I.e. please check this:

tools/perf/util/config.c +4 -1
index 63d472b336de21d4..4fb5e90d7a57ae48 100644
@@ -581,7 +581,10 @@ const char *perf_home_perfconfig(void)
 	static const char *config;
 	static bool failed;
 
-	config = failed ? NULL : home_perfconfig();
+	if (failed || config)
+		return config;
+
+	config = home_perfconfig();
 	if (!config)
 		failed = true;
author
#2 lore raw

Hi,

Quoted 19 lines · Arnaldo Carvalho de Melo

Em Fri, Aug 20, 2021 at 03:08:17PM +0200, Riccardo Mancini escreveu:

Quoted 18 lines

Acaict, perf_home_perfconfig is supposed to cache the result of home_perfconfig, which returns the default location of perfconfig for the user, given the HOME environment variable.
However, the current implementation calls home_perfconfig every time perf_home_perfconfig is called (so no caching is actually performed), replacing the previous pointer, thus also causing a memory leak.

This patch adds a check of whether either config or failed is set and, in that case, directly returns config without calling home_perfconfig at each invocation.

Cc Jiri Olsa <[email protected]>
Fixes f5f03e19ce14fc31 ("perf config: Add perf_home_perfconfig function")
Signed-off-by Riccardo Mancini <[email protected]>

---
 tools/perf/util/config.c | 3 +++

 1 file changed, 3 insertions(+)
Quoted 14 lines
Quoted 7 lines
tools/perf/util/config.c +0 -0
index 63d472b336de21d4..6ab670cdf512507e 100644
@@ -581,6 +581,9 @@ const char *perf_home_perfconfig(void)
         static const char *config;
         static bool failed;
Quoted 5 lines


+       if (config || failed)
+               return config;
+
        config = failed ? NULL : home_perfconfig();

humm, why keep the above failed test then?

Sorry, I forgot about that, it should be removed.

Quoted 15 lines
Quoted 3 lines

        if (!config)
                failed = true;

I.e. please check this:

tools/perf/util/config.c +0 -0
index 63d472b336de21d4..4fb5e90d7a57ae48 100644
@@ -581,7 +581,10 @@ const char *perf_home_perfconfig(void)
         static const char *config;
         static bool failed;

Quoted 8 lines

-       config = failed ? NULL : home_perfconfig(); +       if (failed || config)
+               return config;
+
+       config = home_perfconfig();
        if (!config)
                failed = true;

Looks good to me.
Shall I resend it fixed?

Thanks,
Riccardo

maintainer
#3 lore raw

Em Fri, Aug 20, 2021 at 09:58:16PM +0200, Riccardo Mancini escreveu:

Quoted 12 lines · Arnaldo Carvalho de Melo
Quoted 11 lines
tools/perf/util/config.c +0 -0
index 63d472b336de21d4..4fb5e90d7a57ae48 100644
@@ -581,7 +581,10 @@ const char *perf_home_perfconfig(void)
         static const char *config;
         static bool failed;


-       config = failed ? NULL : home_perfconfig(); +       if (failed || config)
+               return config;

Quoted 8 lines
Quoted 5 lines

+
+       config = home_perfconfig();
        if (!config)
                failed = true;

Looks good to me.
Shall I resend it fixed?

No need, I'll fix it and apply, thanks for checking!

- Arnaldo