[PATCH v2 00/18] clang-tools support in tools

[PATCH v2 00/18] clang-tools support in tools

[Ian Rogers]

Allow the clang-tools scripts to work with builds in tools such as
tools/perf and tools/lib/perf. An example use looks like:

```
$ cd tools/perf
$ make CC=clang CXX=clang++
$ ../../scripts/clang-tools/gen_compile_commands.py
$ ../../scripts/clang-tools/run-clang-tools.py clang-tidy compile_commands.json -checks=-*,readability-named-parameter
Skipping non-C file: 'tools/perf/bench/mem-memcpy-x86-64-asm.S'
Skipping non-C file: 'tools/perf/bench/mem-memset-x86-64-asm.S'
Skipping non-C file: 'tools/perf/arch/x86/tests/regs_load.S'
8 warnings generated.
Suppressed 8 warnings (8 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings generated.
4 warnings generated.
Suppressed 4 warnings (4 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
2 warnings generated.
4 warnings generated.
Suppressed 4 warnings (4 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
3 warnings generated.
tools/perf/util/parse-events-flex.c:546:27: warning: all parameters should be named in a function [readability-named-parameter]
void *yyalloc ( yy_size_t , yyscan_t yyscanner );
                          ^
                           /*size*/
...
```

Fix a number of the more serious low-hanging issues in perf found by
clang-tidy.

This support isn't complete, in particular it doesn't support output
directories properly and so fails for tools/lib/bpf, tools/bpf/bpftool
and if an output directory is used.

v2: Address comments by Nick Desaulniers in patch 3, and add their
    Reviewed-by to patches 1 and 2.

Ian Rogers (18):
  gen_compile_commands: Allow the line prefix to still be cmd_
  gen_compile_commands: Sort output compile commands by file name
  run-clang-tools: Add pass through checks and and header-filter
    arguments
  perf hisi-ptt: Fix potential memory leak
  perf bench uprobe: Fix potential use of memory after free
  perf buildid-cache: Fix use of uninitialized value
  perf env: Remove unnecessary NULL tests
  perf jitdump: Avoid memory leak
  perf mem-events: Avoid uninitialized read
  perf dlfilter: Be defensive against potential NULL dereference
  perf hists browser: Reorder variables to reduce padding
  perf hists browser: Avoid potential NULL dereference
  perf svghelper: Avoid memory leak
  perf parse-events: Fix unlikely memory leak when cloning terms
  tools api: Avoid potential double free
  perf trace-event-info: Avoid passing NULL value to closedir
  perf header: Fix various error path memory leaks
  perf bpf_counter: Fix a few memory leaks

 scripts/clang-tools/gen_compile_commands.py |  8 +--
 scripts/clang-tools/run-clang-tools.py      | 32 ++++++++---
 tools/lib/api/io.h                          |  1 +
 tools/perf/bench/uprobe.c                   |  1 +
 tools/perf/builtin-buildid-cache.c          |  6 +-
 tools/perf/builtin-lock.c                   |  1 +
 tools/perf/ui/browsers/hists.c              |  6 +-
 tools/perf/util/bpf_counter.c               |  5 +-
 tools/perf/util/dlfilter.c                  |  4 +-
 tools/perf/util/env.c                       |  6 +-
 tools/perf/util/header.c                    | 63 +++++++++++++--------
 tools/perf/util/hisi-ptt.c                  | 12 ++--
 tools/perf/util/jitdump.c                   |  1 +
 tools/perf/util/mem-events.c                |  3 +-
 tools/perf/util/parse-events.c              |  4 +-
 tools/perf/util/svghelper.c                 |  5 +-
 tools/perf/util/trace-event-info.c          |  3 +-
 17 files changed, 104 insertions(+), 57 deletions(-)

-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 01/18] gen_compile_commands: Allow the line prefix to still be cmd_

[Ian Rogers]

Builds in tools still use the cmd_ prefix in .cmd files, so don't
require the saved part. Name the groups in the line pattern match so
that changing the regular expression is more robust and works with the
addition of a new match group.

Signed-off-by: Ian Rogers <irogers@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---
 scripts/clang-tools/gen_compile_commands.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
index a84cc5737c2c..b43f9149893c 100755
--- a/scripts/clang-tools/gen_compile_commands.py
+++ b/scripts/clang-tools/gen_compile_commands.py
@@ -19,7 +19,7 @@ _DEFAULT_OUTPUT = 'compile_commands.json'
 _DEFAULT_LOG_LEVEL = 'WARNING'
 
 _FILENAME_PATTERN = r'^\..*\.cmd$'
-_LINE_PATTERN = r'^savedcmd_[^ ]*\.o := (.* )([^ ]*\.[cS]) *(;|$)'
+_LINE_PATTERN = r'^(saved)?cmd_[^ ]*\.o := (?P<command_prefix>.* )(?P<file_path>[^ ]*\.[cS]) *(;|$)'
 _VALID_LOG_LEVELS = ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL']
 # The tools/ directory adopts a different build system, and produces .cmd
 # files in a different format. Do not support it.
@@ -213,8 +213,8 @@ def main():
                 result = line_matcher.match(f.readline())
                 if result:
                     try:
-                        entry = process_line(directory, result.group(1),
-                                             result.group(2))
+                        entry = process_line(directory, result.group('command_prefix'),
+                                             result.group('file_path'))
                         compile_commands.append(entry)
                     except ValueError as err:
                         logging.info('Could not add line from %s: %s',
-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 02/18] gen_compile_commands: Sort output compile commands by file name

[Ian Rogers]

Make the output more stable and deterministic.

Signed-off-by: Ian Rogers <irogers@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---
 scripts/clang-tools/gen_compile_commands.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py
index b43f9149893c..180952fb91c1 100755
--- a/scripts/clang-tools/gen_compile_commands.py
+++ b/scripts/clang-tools/gen_compile_commands.py
@@ -221,7 +221,7 @@ def main():
                                      cmdfile, err)
 
     with open(output, 'wt') as f:
-        json.dump(compile_commands, f, indent=2, sort_keys=True)
+        json.dump(sorted(compile_commands, key=lambda x: x["file"]), f, indent=2, sort_keys=True)
 
 
 if __name__ == '__main__':
-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 03/18] run-clang-tools: Add pass through checks and and header-filter arguments

[Ian Rogers]

Add a -checks argument to allow the checks passed to the clang-tool to
be set on the command line.

Add a pass through -header-filter option.

Don't run analysis on non-C or CPP files.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 scripts/clang-tools/run-clang-tools.py | 32 ++++++++++++++++++++------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
index 3266708a8658..f31ffd09e1ea 100755
--- a/scripts/clang-tools/run-clang-tools.py
+++ b/scripts/clang-tools/run-clang-tools.py
@@ -33,6 +33,11 @@ def parse_arguments():
     path_help = "Path to the compilation database to parse"
     parser.add_argument("path", type=str, help=path_help)
 
+    checks_help = "Checks to pass to the analysis"
+    parser.add_argument("-checks", type=str, default=None, help=checks_help)
+    header_filter_help = "Pass the -header-filter value to the tool"
+    parser.add_argument("-header-filter", type=str, default=None, help=header_filter_help)
+
     return parser.parse_args()
 
 
@@ -45,14 +50,27 @@ def init(l, a):
 
 def run_analysis(entry):
     # Disable all checks, then re-enable the ones we want
-    checks = []
-    checks.append("-checks=-*")
-    if args.type == "clang-tidy":
-        checks.append("linuxkernel-*")
+    global args
+    checks = None
+    if args.checks:
+        checks = args.checks.split(',')
     else:
-        checks.append("clang-analyzer-*")
-        checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
-    p = subprocess.run(["clang-tidy", "-p", args.path, ",".join(checks), entry["file"]],
+        checks = ["-*"]
+        if args.type == "clang-tidy":
+            checks.append("linuxkernel-*")
+        else:
+            checks.append("clang-analyzer-*")
+            checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
+    file = entry["file"]
+    if not file.endswith(".c") and not file.endswith(".cpp"):
+        with lock:
+            print(f"Skipping non-C file: '{file}'", file=sys.stderr)
+        return
+    pargs = ["clang-tidy", "-p", args.path, "-checks=" + ",".join(checks)]
+    if args.header_filter:
+        pargs.append("-header-filter=" + args.header_filter)
+    pargs.append(file)
+    p = subprocess.run(pargs,
                        stdout=subprocess.PIPE,
                        stderr=subprocess.STDOUT,
                        cwd=entry["directory"])
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH v2 03/18] run-clang-tools: Add pass through checks and and header-filter arguments

[Nick Desaulniers]

On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
>
> Add a -checks argument to allow the checks passed to the clang-tool to
> be set on the command line.
>
> Add a pass through -header-filter option.
>
> Don't run analysis on non-C or CPP files.

Three distinct changes; I wouldn't have minded that as three distinct patches.

>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  scripts/clang-tools/run-clang-tools.py | 32 ++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
> index 3266708a8658..f31ffd09e1ea 100755
> --- a/scripts/clang-tools/run-clang-tools.py
> +++ b/scripts/clang-tools/run-clang-tools.py
> @@ -33,6 +33,11 @@ def parse_arguments():
>      path_help = "Path to the compilation database to parse"
>      parser.add_argument("path", type=str, help=path_help)
>
> +    checks_help = "Checks to pass to the analysis"
> +    parser.add_argument("-checks", type=str, default=None, help=checks_help)
> +    header_filter_help = "Pass the -header-filter value to the tool"
> +    parser.add_argument("-header-filter", type=str, default=None, help=header_filter_help)
> +
>      return parser.parse_args()
>
>
> @@ -45,14 +50,27 @@ def init(l, a):
>
>  def run_analysis(entry):
>      # Disable all checks, then re-enable the ones we want
> -    checks = []
> -    checks.append("-checks=-*")
> -    if args.type == "clang-tidy":
> -        checks.append("linuxkernel-*")
> +    global args
> +    checks = None
> +    if args.checks:
> +        checks = args.checks.split(',')
>      else:
> -        checks.append("clang-analyzer-*")
> -        checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
> -    p = subprocess.run(["clang-tidy", "-p", args.path, ",".join(checks), entry["file"]],
> +        checks = ["-*"]
> +        if args.type == "clang-tidy":
> +            checks.append("linuxkernel-*")
> +        else:
> +            checks.append("clang-analyzer-*")
> +            checks.append("-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling")
> +    file = entry["file"]
> +    if not file.endswith(".c") and not file.endswith(".cpp"):
> +        with lock:
> +            print(f"Skipping non-C file: '{file}'", file=sys.stderr)
> +        return

^ perhaps worth returning earlier if this guard fails? i.e.

rather than do a bunch of work, then do a guard that may return early
that doesn't depend on the earlier work, instead guard first then do
all work.

I don't mind that as a follow up rather than a whole v3 for the series.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> +    pargs = ["clang-tidy", "-p", args.path, "-checks=" + ",".join(checks)]
> +    if args.header_filter:
> +        pargs.append("-header-filter=" + args.header_filter)
> +    pargs.append(file)
> +    p = subprocess.run(pargs,
>                         stdout=subprocess.PIPE,
>                         stderr=subprocess.STDOUT,
>                         cwd=entry["directory"])
> --
> 2.42.0.609.gbb76f46606-goog
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v2 04/18] perf hisi-ptt: Fix potential memory leak

[Ian Rogers]

Fix clang-tidy found potential memory leak and unread value:
```
tools/perf/util/hisi-ptt.c:108:3: warning: Value stored to 'data_offset' is never read [clang-analyzer-deadcode.DeadStores]
                data_offset = 0;
                ^             ~
tools/perf/util/hisi-ptt.c:108:3: note: Value stored to 'data_offset' is never read
                data_offset = 0;
                ^             ~
tools/perf/util/hisi-ptt.c:112:12: warning: Potential leak of memory pointed to by 'data' [clang-analyzer-unix.Malloc]
                        return -errno;
                                ^
/usr/include/errno.h:38:18: note: expanded from macro 'errno'
                 ^
tools/perf/util/hisi-ptt.c:100:15: note: Memory is allocated
        void *data = malloc(size);
                     ^~~~~~~~~~~~
tools/perf/util/hisi-ptt.c:104:6: note: Assuming 'data' is non-null
        if (!data)
            ^~~~~
tools/perf/util/hisi-ptt.c:104:2: note: Taking false branch
        if (!data)
        ^
tools/perf/util/hisi-ptt.c:107:6: note: Assuming the condition is false
        if (perf_data__is_pipe(session->data)) {
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/hisi-ptt.c:107:2: note: Taking false branch
        if (perf_data__is_pipe(session->data)) {
        ^
tools/perf/util/hisi-ptt.c:111:7: note: Assuming the condition is true
                if (data_offset == -1)
                    ^~~~~~~~~~~~~~~~~
tools/perf/util/hisi-ptt.c:111:3: note: Taking true branch
                if (data_offset == -1)
                ^
tools/perf/util/hisi-ptt.c:112:12: note: Potential leak of memory pointed to by 'data'
                        return -errno;
                                ^
/usr/include/errno.h:38:18: note: expanded from macro 'errno'
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/hisi-ptt.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/hisi-ptt.c b/tools/perf/util/hisi-ptt.c
index 45b614bb73bf..ea297329c526 100644
--- a/tools/perf/util/hisi-ptt.c
+++ b/tools/perf/util/hisi-ptt.c
@@ -98,18 +98,18 @@ static int hisi_ptt_process_auxtrace_event(struct perf_session *session,
 	int fd = perf_data__fd(session->data);
 	int size = event->auxtrace.size;
 	void *data = malloc(size);
-	off_t data_offset;
 	int err;
 
 	if (!data)
 		return -errno;
 
-	if (perf_data__is_pipe(session->data)) {
-		data_offset = 0;
-	} else {
-		data_offset = lseek(fd, 0, SEEK_CUR);
-		if (data_offset == -1)
+	if (!perf_data__is_pipe(session->data)) {
+		off_t data_offset = lseek(fd, 0, SEEK_CUR);
+
+		if (data_offset == -1) {
+			free(data);
 			return -errno;
+		}
 	}
 
 	err = readn(fd, data, size);
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH v2 04/18] perf hisi-ptt: Fix potential memory leak

[Namhyung Kim]

On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
>
> Fix clang-tidy found potential memory leak and unread value:
> ```
> tools/perf/util/hisi-ptt.c:108:3: warning: Value stored to 'data_offset' is never read [clang-analyzer-deadcode.DeadStores]
>                 data_offset = 0;
>                 ^             ~
> tools/perf/util/hisi-ptt.c:108:3: note: Value stored to 'data_offset' is never read
>                 data_offset = 0;
>                 ^             ~
> tools/perf/util/hisi-ptt.c:112:12: warning: Potential leak of memory pointed to by 'data' [clang-analyzer-unix.Malloc]
>                         return -errno;
>                                 ^
> /usr/include/errno.h:38:18: note: expanded from macro 'errno'
>                  ^
> tools/perf/util/hisi-ptt.c:100:15: note: Memory is allocated
>         void *data = malloc(size);
>                      ^~~~~~~~~~~~
> tools/perf/util/hisi-ptt.c:104:6: note: Assuming 'data' is non-null
>         if (!data)
>             ^~~~~
> tools/perf/util/hisi-ptt.c:104:2: note: Taking false branch
>         if (!data)
>         ^
> tools/perf/util/hisi-ptt.c:107:6: note: Assuming the condition is false
>         if (perf_data__is_pipe(session->data)) {
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> tools/perf/util/hisi-ptt.c:107:2: note: Taking false branch
>         if (perf_data__is_pipe(session->data)) {
>         ^
> tools/perf/util/hisi-ptt.c:111:7: note: Assuming the condition is true
>                 if (data_offset == -1)
>                     ^~~~~~~~~~~~~~~~~
> tools/perf/util/hisi-ptt.c:111:3: note: Taking true branch
>                 if (data_offset == -1)
>                 ^
> tools/perf/util/hisi-ptt.c:112:12: note: Potential leak of memory pointed to by 'data'
>                         return -errno;
>                                 ^
> /usr/include/errno.h:38:18: note: expanded from macro 'errno'
> ```

We already have

  https://lore.kernel.org/r/20230930072719.1267784-1-visitorckw@gmail.com

Thanks,
Namhyung


>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/hisi-ptt.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/hisi-ptt.c b/tools/perf/util/hisi-ptt.c
> index 45b614bb73bf..ea297329c526 100644
> --- a/tools/perf/util/hisi-ptt.c
> +++ b/tools/perf/util/hisi-ptt.c
> @@ -98,18 +98,18 @@ static int hisi_ptt_process_auxtrace_event(struct perf_session *session,
>         int fd = perf_data__fd(session->data);
>         int size = event->auxtrace.size;
>         void *data = malloc(size);
> -       off_t data_offset;
>         int err;
>
>         if (!data)
>                 return -errno;
>
> -       if (perf_data__is_pipe(session->data)) {
> -               data_offset = 0;
> -       } else {
> -               data_offset = lseek(fd, 0, SEEK_CUR);
> -               if (data_offset == -1)
> +       if (!perf_data__is_pipe(session->data)) {
> +               off_t data_offset = lseek(fd, 0, SEEK_CUR);
> +
> +               if (data_offset == -1) {
> +                       free(data);
>                         return -errno;
> +               }
>         }
>
>         err = readn(fd, data, size);
> --
> 2.42.0.609.gbb76f46606-goog
>

Re: [PATCH v2 04/18] perf hisi-ptt: Fix potential memory leak

[Ian Rogers]

On Sun, Oct 8, 2023 at 10:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Fix clang-tidy found potential memory leak and unread value:
> > ```
> > tools/perf/util/hisi-ptt.c:108:3: warning: Value stored to 'data_offset' is never read [clang-analyzer-deadcode.DeadStores]
> >                 data_offset = 0;
> >                 ^             ~
> > tools/perf/util/hisi-ptt.c:108:3: note: Value stored to 'data_offset' is never read
> >                 data_offset = 0;
> >                 ^             ~
> > tools/perf/util/hisi-ptt.c:112:12: warning: Potential leak of memory pointed to by 'data' [clang-analyzer-unix.Malloc]
> >                         return -errno;
> >                                 ^
> > /usr/include/errno.h:38:18: note: expanded from macro 'errno'
> >                  ^
> > tools/perf/util/hisi-ptt.c:100:15: note: Memory is allocated
> >         void *data = malloc(size);
> >                      ^~~~~~~~~~~~
> > tools/perf/util/hisi-ptt.c:104:6: note: Assuming 'data' is non-null
> >         if (!data)
> >             ^~~~~
> > tools/perf/util/hisi-ptt.c:104:2: note: Taking false branch
> >         if (!data)
> >         ^
> > tools/perf/util/hisi-ptt.c:107:6: note: Assuming the condition is false
> >         if (perf_data__is_pipe(session->data)) {
> >             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > tools/perf/util/hisi-ptt.c:107:2: note: Taking false branch
> >         if (perf_data__is_pipe(session->data)) {
> >         ^
> > tools/perf/util/hisi-ptt.c:111:7: note: Assuming the condition is true
> >                 if (data_offset == -1)
> >                     ^~~~~~~~~~~~~~~~~
> > tools/perf/util/hisi-ptt.c:111:3: note: Taking true branch
> >                 if (data_offset == -1)
> >                 ^
> > tools/perf/util/hisi-ptt.c:112:12: note: Potential leak of memory pointed to by 'data'
> >                         return -errno;
> >                                 ^
> > /usr/include/errno.h:38:18: note: expanded from macro 'errno'
> > ```
>
> We already have
>
>   https://lore.kernel.org/r/20230930072719.1267784-1-visitorckw@gmail.com

Agreed. There is a written to but not read addressed in this one, but
I think that is okay to clean up later and to drop this patch.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/hisi-ptt.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/perf/util/hisi-ptt.c b/tools/perf/util/hisi-ptt.c
> > index 45b614bb73bf..ea297329c526 100644
> > --- a/tools/perf/util/hisi-ptt.c
> > +++ b/tools/perf/util/hisi-ptt.c
> > @@ -98,18 +98,18 @@ static int hisi_ptt_process_auxtrace_event(struct perf_session *session,
> >         int fd = perf_data__fd(session->data);
> >         int size = event->auxtrace.size;
> >         void *data = malloc(size);
> > -       off_t data_offset;
> >         int err;
> >
> >         if (!data)
> >                 return -errno;
> >
> > -       if (perf_data__is_pipe(session->data)) {
> > -               data_offset = 0;
> > -       } else {
> > -               data_offset = lseek(fd, 0, SEEK_CUR);
> > -               if (data_offset == -1)
> > +       if (!perf_data__is_pipe(session->data)) {
> > +               off_t data_offset = lseek(fd, 0, SEEK_CUR);
> > +
> > +               if (data_offset == -1) {
> > +                       free(data);
> >                         return -errno;
> > +               }
> >         }
> >
> >         err = readn(fd, data, size);
> > --
> > 2.42.0.609.gbb76f46606-goog
> >

[PATCH v2 05/18] perf bench uprobe: Fix potential use of memory after free

[Ian Rogers]

Found by clang-tidy:
```
bench/uprobe.c:98:3: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc]
                bench_uprobe_bpf__destroy(skel);
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/bench/uprobe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/bench/uprobe.c b/tools/perf/bench/uprobe.c
index 914c0817fe8a..5c71fdc419dd 100644
--- a/tools/perf/bench/uprobe.c
+++ b/tools/perf/bench/uprobe.c
@@ -89,6 +89,7 @@ static int bench_uprobe__setup_bpf_skel(enum bench_uprobe bench)
 	return err;
 cleanup:
 	bench_uprobe_bpf__destroy(skel);
+	skel = NULL;
 	return err;
 }
 
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH v2 05/18] perf bench uprobe: Fix potential use of memory after free

[Namhyung Kim]

On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
>
> Found by clang-tidy:
> ```
> bench/uprobe.c:98:3: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc]
>                 bench_uprobe_bpf__destroy(skel);
> ```
>
> Signed-off-by: Ian Rogers <irogers@google.com>

I'm ok with the change but I think it won't call
bench_uprobe__teardown_bpf_skel() if the setup function
returns a negative value.  Maybe we also need to set the
err in the default case of `switch (bench)` statement.

Thanks,
Namhyung


> ---
>  tools/perf/bench/uprobe.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/bench/uprobe.c b/tools/perf/bench/uprobe.c
> index 914c0817fe8a..5c71fdc419dd 100644
> --- a/tools/perf/bench/uprobe.c
> +++ b/tools/perf/bench/uprobe.c
> @@ -89,6 +89,7 @@ static int bench_uprobe__setup_bpf_skel(enum bench_uprobe bench)
>         return err;
>  cleanup:
>         bench_uprobe_bpf__destroy(skel);
> +       skel = NULL;
>         return err;
>  }
>
> --
> 2.42.0.609.gbb76f46606-goog
>

Re: [PATCH v2 05/18] perf bench uprobe: Fix potential use of memory after free

[Ian Rogers]

On Sun, Oct 8, 2023 at 10:51 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Found by clang-tidy:
> > ```
> > bench/uprobe.c:98:3: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc]
> >                 bench_uprobe_bpf__destroy(skel);
> > ```
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> I'm ok with the change but I think it won't call
> bench_uprobe__teardown_bpf_skel() if the setup function
> returns a negative value.  Maybe we also need to set the
> err in the default case of `switch (bench)` statement.

Yes, the analysis (I'll put it below) assumes that the err can be
positive yielding destroy being called twice, the second creating a
use-after-free. I think it is worth cleaning the code up and making
the analyzer's job easier.

Thanks,
Ian

```
bench/uprobe.c:98:3: warning: Use of memory after it is freed
[clang-analyzer-unix.Malloc]
               bench_uprobe_bpf__destroy(skel);
               ^
tools/perf/bench/uprobe.c:197:9: note: Calling 'bench_uprobe'
       return bench_uprobe(argc, argv, BENCH_UPROBE__TRACE_PRINTK);
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/bench/uprobe.c:150:6: note: 'bench' is not equal to
BENCH_UPROBE__BASELINE
       if (bench != BENCH_UPROBE__BASELINE &&
bench_uprobe__setup_bpf_skel(bench) < 0)
           ^~~~~
tools/perf/bench/uprobe.c:150:6: note: Left side of '&&' is true
tools/perf/bench/uprobe.c:150:41: note: Calling 'bench_uprobe__setup_bpf_skel'
       if (bench != BENCH_UPROBE__BASELINE &&
bench_uprobe__setup_bpf_skel(bench) < 0)

^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/bench/uprobe.c:68:7: note: 'skel' is non-null
       if (!skel) {
            ^~~~
tools/perf/bench/uprobe.c:68:2: note: Taking false branch
       if (!skel) {
       ^
tools/perf/bench/uprobe.c:74:6: note: Assuming 'err' is not equal to 0
       if (err) {
           ^~~
tools/perf/bench/uprobe.c:74:2: note: Taking true branch
       if (err) {
       ^
tools/perf/bench/uprobe.c:76:3: note: Control jumps to line 91
               goto cleanup;
               ^
tools/perf/bench/uprobe.c:91:2: note: Calling 'bench_uprobe_bpf__destroy'
       bench_uprobe_bpf__destroy(skel);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/bpf_skel/bench_uprobe.skel.h:44:6: note: Assuming
'obj' is non-null
       if (!obj)
           ^~~~
tools/perf/util/bpf_skel/bench_uprobe.skel.h:44:2: note: Taking false branch
       if (!obj)
       ^
tools/perf/util/bpf_skel/bench_uprobe.skel.h:46:6: note: Assuming
field 'skeleton' is null
       if (obj->skeleton)
           ^~~~~~~~~~~~~
tools/perf/util/bpf_skel/bench_uprobe.skel.h:46:2: note: Taking false branch
       if (obj->skeleton)
       ^
tools/perf/util/bpf_skel/bench_uprobe.skel.h:48:2: note: Memory is released
       free(obj);
       ^~~~~~~~~
tools/perf/bench/uprobe.c:91:2: note: Returning; memory was released
via 1st parameter
       bench_uprobe_bpf__destroy(skel);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/bench/uprobe.c:150:41: note: Returning; memory was released
       if (bench != BENCH_UPROBE__BASELINE &&
bench_uprobe__setup_bpf_skel(bench) < 0)

^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/bench/uprobe.c:150:41: note: Assuming the condition is false
       if (bench != BENCH_UPROBE__BASELINE &&
bench_uprobe__setup_bpf_skel(bench) < 0)

^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/bench/uprobe.c:150:2: note: Taking false branch
       if (bench != BENCH_UPROBE__BASELINE &&
bench_uprobe__setup_bpf_skel(bench) < 0)
       ^
tools/perf/bench/uprobe.c:155:14: note: Assuming 'i' is >= 'loops'
       for (i = 0; i < loops; i++) {
                   ^~~~~~~~~
tools/perf/bench/uprobe.c:155:2: note: Loop condition is false.
Execution continues on line 159
       for (i = 0; i < loops; i++) {
       ^
tools/perf/bench/uprobe.c:164:2: note: Control jumps to 'case 1:'  at line 169
       switch (bench_format) {
       ^
tools/perf/bench/uprobe.c:171:3: note:  Execution continues on line 179
               break;
               ^
tools/perf/bench/uprobe.c:179:6: note: 'bench' is not equal to
BENCH_UPROBE__BASELINE
       if (bench != BENCH_UPROBE__BASELINE)
           ^~~~~
tools/perf/bench/uprobe.c:179:2: note: Taking true branch
       if (bench != BENCH_UPROBE__BASELINE)
       ^
tools/perf/bench/uprobe.c:180:3: note: Calling 'bench_uprobe__teardown_bpf_skel'
               bench_uprobe__teardown_bpf_skel();
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/bench/uprobe.c:97:6: note: 'skel' is non-null
       if (skel) {
           ^~~~
tools/perf/bench/uprobe.c:97:2: note: Taking true branch
       if (skel) {
       ^
tools/perf/bench/uprobe.c:98:3: note: Use of memory after it is freed
               bench_uprobe_bpf__destroy(skel);
               ^                         ~~~~
1 warning generated.
```

> Thanks,
> Namhyung
>
>
> > ---
> >  tools/perf/bench/uprobe.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/perf/bench/uprobe.c b/tools/perf/bench/uprobe.c
> > index 914c0817fe8a..5c71fdc419dd 100644
> > --- a/tools/perf/bench/uprobe.c
> > +++ b/tools/perf/bench/uprobe.c
> > @@ -89,6 +89,7 @@ static int bench_uprobe__setup_bpf_skel(enum bench_uprobe bench)
> >         return err;
> >  cleanup:
> >         bench_uprobe_bpf__destroy(skel);
> > +       skel = NULL;
> >         return err;
> >  }
> >
> > --
> > 2.42.0.609.gbb76f46606-goog
> >

[PATCH v2 06/18] perf buildid-cache: Fix use of uninitialized value

[Ian Rogers]

The buildid filename is first determined and then from this the
buildid read. If getting the filename fails then the buildid will be
used for a later memcmp uninitialized. Detected by clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-buildid-cache.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index cd381693658b..e2a40f1d9225 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -277,8 +277,10 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
 	char filename[PATH_MAX];
 	struct build_id bid;
 
-	if (dso__build_id_filename(dso, filename, sizeof(filename), false) &&
-	    filename__read_build_id(filename, &bid) == -1) {
+	if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
+		return true;
+
+	if (filename__read_build_id(filename, &bid) == -1) {
 		if (errno == ENOENT)
 			return false;
 
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH v2 06/18] perf buildid-cache: Fix use of uninitialized value

[Namhyung Kim]

On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
>
> The buildid filename is first determined and then from this the
> buildid read. If getting the filename fails then the buildid will be
> used for a later memcmp uninitialized. Detected by clang-tidy.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-buildid-cache.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> index cd381693658b..e2a40f1d9225 100644
> --- a/tools/perf/builtin-buildid-cache.c
> +++ b/tools/perf/builtin-buildid-cache.c
> @@ -277,8 +277,10 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
>         char filename[PATH_MAX];
>         struct build_id bid;
>
> -       if (dso__build_id_filename(dso, filename, sizeof(filename), false) &&
> -           filename__read_build_id(filename, &bid) == -1) {
> +       if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
> +               return true;

This won't print anything and ignore the file which changes
the existing behavior.  But if it fails to read the build-id, I
don't think there is not much we can do with it.  IIUC the
original intention of -M/--missing option is to list files that
have a build-id but it's not in the build-id cache.  So maybe
it's ok to silently ignore it.

Thanks,
Namhyung


> +
> +       if (filename__read_build_id(filename, &bid) == -1) {
>                 if (errno == ENOENT)
>                         return false;
>
> --
> 2.42.0.609.gbb76f46606-goog
>

Re: [PATCH v2 06/18] perf buildid-cache: Fix use of uninitialized value

[Ian Rogers]

On Sun, Oct 8, 2023 at 11:06 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
> >
> > The buildid filename is first determined and then from this the
> > buildid read. If getting the filename fails then the buildid will be
> > used for a later memcmp uninitialized. Detected by clang-tidy.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/builtin-buildid-cache.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> > index cd381693658b..e2a40f1d9225 100644
> > --- a/tools/perf/builtin-buildid-cache.c
> > +++ b/tools/perf/builtin-buildid-cache.c
> > @@ -277,8 +277,10 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
> >         char filename[PATH_MAX];
> >         struct build_id bid;
> >
> > -       if (dso__build_id_filename(dso, filename, sizeof(filename), false) &&
> > -           filename__read_build_id(filename, &bid) == -1) {
> > +       if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
> > +               return true;
>
> This won't print anything and ignore the file which changes
> the existing behavior.  But if it fails to read the build-id, I
> don't think there is not much we can do with it.  IIUC the
> original intention of -M/--missing option is to list files that
> have a build-id but it's not in the build-id cache.  So maybe
> it's ok to silently ignore it.

If getting the build id filename fails then 'bid' is uninitialized and
I don't think there is an expected behavior for what a memcmp on
uninitialized memory should do - we may hope that it fails and get the
pr_warning in the existing code, but that warning depends on reading
the filename too. This was the smallest change to not change behavior
but to avoid the undefined behavior (bugs) in the code. It could be a
signal the code needs to be worked on more.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> > +
> > +       if (filename__read_build_id(filename, &bid) == -1) {
> >                 if (errno == ENOENT)
> >                         return false;
> >
> > --
> > 2.42.0.609.gbb76f46606-goog
> >

[PATCH v2 07/18] perf env: Remove unnecessary NULL tests

[Ian Rogers]

clang-tidy was warning:
```
util/env.c:334:23: warning: Access to field 'nr_pmu_mappings' results in a dereference of a null pointer (loaded from variable 'env') [clang-analyzer-core.NullDereference]
        env->nr_pmu_mappings = pmu_num;
```

As functions are called potentially when !env was true. This condition
could never be true as it would produce a segv, so remove the
unnecessary NULL tests and silence clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/env.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index a164164001fb..44140b7f596a 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -457,7 +457,7 @@ const char *perf_env__cpuid(struct perf_env *env)
 {
 	int status;
 
-	if (!env || !env->cpuid) { /* Assume local operation */
+	if (!env->cpuid) { /* Assume local operation */
 		status = perf_env__read_cpuid(env);
 		if (status)
 			return NULL;
@@ -470,7 +470,7 @@ int perf_env__nr_pmu_mappings(struct perf_env *env)
 {
 	int status;
 
-	if (!env || !env->nr_pmu_mappings) { /* Assume local operation */
+	if (!env->nr_pmu_mappings) { /* Assume local operation */
 		status = perf_env__read_pmu_mappings(env);
 		if (status)
 			return 0;
@@ -483,7 +483,7 @@ const char *perf_env__pmu_mappings(struct perf_env *env)
 {
 	int status;
 
-	if (!env || !env->pmu_mappings) { /* Assume local operation */
+	if (!env->pmu_mappings) { /* Assume local operation */
 		status = perf_env__read_pmu_mappings(env);
 		if (status)
 			return NULL;
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH v2 07/18] perf env: Remove unnecessary NULL tests

[Namhyung Kim]

On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
>
> clang-tidy was warning:
> ```
> util/env.c:334:23: warning: Access to field 'nr_pmu_mappings' results in a dereference of a null pointer (loaded from variable 'env') [clang-analyzer-core.NullDereference]
>         env->nr_pmu_mappings = pmu_num;
> ```
>
> As functions are called potentially when !env was true. This condition
> could never be true as it would produce a segv, so remove the
> unnecessary NULL tests and silence clang-tidy.

IIRC !env was to handle live sessions like perf top
or when it doesn't have a perf data file.  Anyway,
it doesn't seem to work anymore.

Thanks,
Namhyung

>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/env.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index a164164001fb..44140b7f596a 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
> @@ -457,7 +457,7 @@ const char *perf_env__cpuid(struct perf_env *env)
>  {
>         int status;
>
> -       if (!env || !env->cpuid) { /* Assume local operation */
> +       if (!env->cpuid) { /* Assume local operation */
>                 status = perf_env__read_cpuid(env);
>                 if (status)
>                         return NULL;
> @@ -470,7 +470,7 @@ int perf_env__nr_pmu_mappings(struct perf_env *env)
>  {
>         int status;
>
> -       if (!env || !env->nr_pmu_mappings) { /* Assume local operation */
> +       if (!env->nr_pmu_mappings) { /* Assume local operation */
>                 status = perf_env__read_pmu_mappings(env);
>                 if (status)
>                         return 0;
> @@ -483,7 +483,7 @@ const char *perf_env__pmu_mappings(struct perf_env *env)
>  {
>         int status;
>
> -       if (!env || !env->pmu_mappings) { /* Assume local operation */
> +       if (!env->pmu_mappings) { /* Assume local operation */
>                 status = perf_env__read_pmu_mappings(env);
>                 if (status)
>                         return NULL;
> --
> 2.42.0.609.gbb76f46606-goog
>

Re: [PATCH v2 07/18] perf env: Remove unnecessary NULL tests

[Ian Rogers]

On Sun, Oct 8, 2023 at 11:14 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
> >
> > clang-tidy was warning:
> > ```
> > util/env.c:334:23: warning: Access to field 'nr_pmu_mappings' results in a dereference of a null pointer (loaded from variable 'env') [clang-analyzer-core.NullDereference]
> >         env->nr_pmu_mappings = pmu_num;
> > ```
> >
> > As functions are called potentially when !env was true. This condition
> > could never be true as it would produce a segv, so remove the
> > unnecessary NULL tests and silence clang-tidy.
>
> IIRC !env was to handle live sessions like perf top
> or when it doesn't have a perf data file.  Anyway,
> it doesn't seem to work anymore.

I trust the analyzer here and it would be better to crash earlier,
from env being NULL, than in a particular conditional branch. Like
you, I wasn't able to find a failure from removing the checks.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/env.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> > index a164164001fb..44140b7f596a 100644
> > --- a/tools/perf/util/env.c
> > +++ b/tools/perf/util/env.c
> > @@ -457,7 +457,7 @@ const char *perf_env__cpuid(struct perf_env *env)
> >  {
> >         int status;
> >
> > -       if (!env || !env->cpuid) { /* Assume local operation */
> > +       if (!env->cpuid) { /* Assume local operation */
> >                 status = perf_env__read_cpuid(env);
> >                 if (status)
> >                         return NULL;
> > @@ -470,7 +470,7 @@ int perf_env__nr_pmu_mappings(struct perf_env *env)
> >  {
> >         int status;
> >
> > -       if (!env || !env->nr_pmu_mappings) { /* Assume local operation */
> > +       if (!env->nr_pmu_mappings) { /* Assume local operation */
> >                 status = perf_env__read_pmu_mappings(env);
> >                 if (status)
> >                         return 0;
> > @@ -483,7 +483,7 @@ const char *perf_env__pmu_mappings(struct perf_env *env)
> >  {
> >         int status;
> >
> > -       if (!env || !env->pmu_mappings) { /* Assume local operation */
> > +       if (!env->pmu_mappings) { /* Assume local operation */
> >                 status = perf_env__read_pmu_mappings(env);
> >                 if (status)
> >                         return NULL;
> > --
> > 2.42.0.609.gbb76f46606-goog
> >

[PATCH v2 08/18] perf jitdump: Avoid memory leak

[Ian Rogers]

jit_repipe_unwinding_info is called in a loop by jit_process_dump,
avoid leaking unwinding_data by free-ing before overwriting. Error
detected by clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/jitdump.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index 6b2b96c16ccd..1f657ef8975f 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -675,6 +675,7 @@ jit_repipe_unwinding_info(struct jit_buf_desc *jd, union jr_entry *jr)
 	jd->eh_frame_hdr_size = jr->unwinding.eh_frame_hdr_size;
 	jd->unwinding_size = jr->unwinding.unwinding_size;
 	jd->unwinding_mapped_size = jr->unwinding.mapped_size;
+	free(jd->unwinding_data);
 	jd->unwinding_data = unwinding_data;
 
 	return 0;
-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 09/18] perf mem-events: Avoid uninitialized read

[Ian Rogers]

pmu should be initialized to NULL before perf_pmus__scan loop. Fix and
shrink the scope of pmu at the same time. Issue detected by clang-tidy.

Fixes: 5752c20f3787 ("perf mem: Scan all PMUs instead of just core ones")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/mem-events.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index 39ffe8ceb380..954b235e12e5 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -185,7 +185,6 @@ int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
 {
 	int i = *argv_nr, k = 0;
 	struct perf_mem_event *e;
-	struct perf_pmu *pmu;
 
 	for (int j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
 		e = perf_mem_events__ptr(j);
@@ -202,6 +201,8 @@ int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
 			rec_argv[i++] = "-e";
 			rec_argv[i++] = perf_mem_events__name(j, NULL);
 		} else {
+			struct perf_pmu *pmu = NULL;
+
 			if (!e->supported) {
 				perf_mem_events__print_unsupport_hybrid(e, j);
 				return -1;
-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 10/18] perf dlfilter: Be defensive against potential NULL dereference

[Ian Rogers]

In the unlikely case of having a symbol without a mapping, avoid a
NULL dereference that clang-tidy warns about.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/dlfilter.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dlfilter.c b/tools/perf/util/dlfilter.c
index 1dbf27822ee2..5e54832137a9 100644
--- a/tools/perf/util/dlfilter.c
+++ b/tools/perf/util/dlfilter.c
@@ -52,8 +52,10 @@ static void al_to_d_al(struct addr_location *al, struct perf_dlfilter_al *d_al)
 		d_al->sym_end = sym->end;
 		if (al->addr < sym->end)
 			d_al->symoff = al->addr - sym->start;
-		else
+		else if (al->map)
 			d_al->symoff = al->addr - map__start(al->map) - sym->start;
+		else
+			d_al->symoff = 0;
 		d_al->sym_binding = sym->binding;
 	} else {
 		d_al->sym = NULL;
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH v2 10/18] perf dlfilter: Be defensive against potential NULL dereference

[Namhyung Kim]

On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
>
> In the unlikely case of having a symbol without a mapping, avoid a
> NULL dereference that clang-tidy warns about.

I'm not sure if it's possible to have a symbol without a map,
but I'm also fine with being conservative.

Thanks,
Namhyung

>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/dlfilter.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/dlfilter.c b/tools/perf/util/dlfilter.c
> index 1dbf27822ee2..5e54832137a9 100644
> --- a/tools/perf/util/dlfilter.c
> +++ b/tools/perf/util/dlfilter.c
> @@ -52,8 +52,10 @@ static void al_to_d_al(struct addr_location *al, struct perf_dlfilter_al *d_al)
>                 d_al->sym_end = sym->end;
>                 if (al->addr < sym->end)
>                         d_al->symoff = al->addr - sym->start;
> -               else
> +               else if (al->map)
>                         d_al->symoff = al->addr - map__start(al->map) - sym->start;
> +               else
> +                       d_al->symoff = 0;
>                 d_al->sym_binding = sym->binding;
>         } else {
>                 d_al->sym = NULL;
> --
> 2.42.0.609.gbb76f46606-goog
>

[PATCH v2 11/18] perf hists browser: Reorder variables to reduce padding

[Ian Rogers]

Address clang-tidy warning:
```
tools/perf/ui/browsers/hists.c:2416:8: warning: Excessive padding in 'struct popup_action' (8 padding bytes, where 0 is optimal).
Optimal fields order:
time,
thread,
evsel,
fn,
ms,
socket,
rstype,
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 70db5a717905..f02ee605bbce 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2416,12 +2416,12 @@ static int switch_data_file(void)
 struct popup_action {
 	unsigned long		time;
 	struct thread 		*thread;
+	struct evsel	*evsel;
+	int (*fn)(struct hist_browser *browser, struct popup_action *act);
 	struct map_symbol 	ms;
 	int			socket;
-	struct evsel	*evsel;
 	enum rstype		rstype;
 
-	int (*fn)(struct hist_browser *browser, struct popup_action *act);
 };
 
 static int
-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 12/18] perf hists browser: Avoid potential NULL dereference

[Ian Rogers]

On other code paths browser->he_selection is NULL checked, add a
missing case reported by clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f02ee605bbce..f4812b226818 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3302,7 +3302,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 							&options[nr_options],
 							&bi->to.ms,
 							bi->to.al_addr);
-		} else {
+		} else if (browser->he_selection) {
 			nr_options += add_annotate_opt(browser,
 						       &actions[nr_options],
 						       &options[nr_options],
-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 13/18] perf svghelper: Avoid memory leak

[Ian Rogers]

On success path the sib_core and sib_thr values weren't being
freed. Detected by clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-lock.c   | 1 +
 tools/perf/util/svghelper.c | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index d4b22313e5fc..1b40b00c9563 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -2463,6 +2463,7 @@ static int parse_call_stack(const struct option *opt __maybe_unused, const char
 		entry = malloc(sizeof(*entry) + strlen(tok) + 1);
 		if (entry == NULL) {
 			pr_err("Memory allocation failure\n");
+			free(s);
 			return -1;
 		}
 
diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
index 0e4dc31c6c9c..1892e9b6aa7f 100644
--- a/tools/perf/util/svghelper.c
+++ b/tools/perf/util/svghelper.c
@@ -754,6 +754,7 @@ int svg_build_topology_map(struct perf_env *env)
 	int i, nr_cpus;
 	struct topology t;
 	char *sib_core, *sib_thr;
+	int ret = -1;
 
 	nr_cpus = min(env->nr_cpus_online, MAX_NR_CPUS);
 
@@ -799,11 +800,11 @@ int svg_build_topology_map(struct perf_env *env)
 
 	scan_core_topology(topology_map, &t, nr_cpus);
 
-	return 0;
+	ret = 0;
 
 exit:
 	zfree(&t.sib_core);
 	zfree(&t.sib_thr);
 
-	return -1;
+	return ret;
 }
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH v2 13/18] perf svghelper: Avoid memory leak

[Namhyung Kim]

On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
>
> On success path the sib_core and sib_thr values weren't being
> freed. Detected by clang-tidy.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-lock.c   | 1 +
>  tools/perf/util/svghelper.c | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index d4b22313e5fc..1b40b00c9563 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -2463,6 +2463,7 @@ static int parse_call_stack(const struct option *opt __maybe_unused, const char
>                 entry = malloc(sizeof(*entry) + strlen(tok) + 1);
>                 if (entry == NULL) {
>                         pr_err("Memory allocation failure\n");
> +                       free(s);
>                         return -1;
>                 }
>

This is unrelated.  Please put it in a separate patch.

Thanks,
Namhyung


> diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
> index 0e4dc31c6c9c..1892e9b6aa7f 100644
> --- a/tools/perf/util/svghelper.c
> +++ b/tools/perf/util/svghelper.c
> @@ -754,6 +754,7 @@ int svg_build_topology_map(struct perf_env *env)
>         int i, nr_cpus;
>         struct topology t;
>         char *sib_core, *sib_thr;
> +       int ret = -1;
>
>         nr_cpus = min(env->nr_cpus_online, MAX_NR_CPUS);
>
> @@ -799,11 +800,11 @@ int svg_build_topology_map(struct perf_env *env)
>
>         scan_core_topology(topology_map, &t, nr_cpus);
>
> -       return 0;
> +       ret = 0;
>
>  exit:
>         zfree(&t.sib_core);
>         zfree(&t.sib_thr);
>
> -       return -1;
> +       return ret;
>  }
> --
> 2.42.0.609.gbb76f46606-goog
>

Re: [PATCH v2 13/18] perf svghelper: Avoid memory leak

[Ian Rogers]

On Sun, Oct 8, 2023 at 11:31 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On success path the sib_core and sib_thr values weren't being
> > freed. Detected by clang-tidy.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/builtin-lock.c   | 1 +
> >  tools/perf/util/svghelper.c | 5 +++--
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > index d4b22313e5fc..1b40b00c9563 100644
> > --- a/tools/perf/builtin-lock.c
> > +++ b/tools/perf/builtin-lock.c
> > @@ -2463,6 +2463,7 @@ static int parse_call_stack(const struct option *opt __maybe_unused, const char
> >                 entry = malloc(sizeof(*entry) + strlen(tok) + 1);
> >                 if (entry == NULL) {
> >                         pr_err("Memory allocation failure\n");
> > +                       free(s);
> >                         return -1;
> >                 }
> >
>
> This is unrelated.  Please put it in a separate patch.

Sorry, will fix in v3.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> > diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
> > index 0e4dc31c6c9c..1892e9b6aa7f 100644
> > --- a/tools/perf/util/svghelper.c
> > +++ b/tools/perf/util/svghelper.c
> > @@ -754,6 +754,7 @@ int svg_build_topology_map(struct perf_env *env)
> >         int i, nr_cpus;
> >         struct topology t;
> >         char *sib_core, *sib_thr;
> > +       int ret = -1;
> >
> >         nr_cpus = min(env->nr_cpus_online, MAX_NR_CPUS);
> >
> > @@ -799,11 +800,11 @@ int svg_build_topology_map(struct perf_env *env)
> >
> >         scan_core_topology(topology_map, &t, nr_cpus);
> >
> > -       return 0;
> > +       ret = 0;
> >
> >  exit:
> >         zfree(&t.sib_core);
> >         zfree(&t.sib_thr);
> >
> > -       return -1;
> > +       return ret;
> >  }
> > --
> > 2.42.0.609.gbb76f46606-goog
> >

[PATCH v2 14/18] perf parse-events: Fix unlikely memory leak when cloning terms

[Ian Rogers]

Add missing free on an error path as detected by clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index c56e07bd7dd6..23c027cf20ae 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2549,8 +2549,10 @@ int parse_events_term__clone(struct parse_events_term **new,
 		return new_term(new, &temp, /*str=*/NULL, term->val.num);
 
 	str = strdup(term->val.str);
-	if (!str)
+	if (!str) {
+		zfree(&temp.config);
 		return -ENOMEM;
+	}
 	return new_term(new, &temp, str, /*num=*/0);
 }
 
-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 15/18] tools api: Avoid potential double free

[Ian Rogers]

io__getline will free the line on error but it doesn't clear the out
argument. This may lead to the line being freed twice, like in
tools/perf/util/srcline.c as detected by clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/api/io.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
index 9fc429d2852d..a77b74c5fb65 100644
--- a/tools/lib/api/io.h
+++ b/tools/lib/api/io.h
@@ -180,6 +180,7 @@ static inline ssize_t io__getline(struct io *io, char **line_out, size_t *line_l
 	return line_len;
 err_out:
 	free(line);
+	*line_out = NULL;
 	return -ENOMEM;
 }
 
-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 16/18] perf trace-event-info: Avoid passing NULL value to closedir

[Ian Rogers]

If opendir failed then closedir was passed NULL which is
erroneous. Caught by clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/trace-event-info.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 319ccf09a435..c8755679281e 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -313,7 +313,8 @@ static int record_event_files(struct tracepoint_path *tps)
 	}
 	err = 0;
 out:
-	closedir(dir);
+	if (dir)
+		closedir(dir);
 	put_tracing_file(path);
 
 	return err;
-- 
2.42.0.609.gbb76f46606-goog

[PATCH v2 17/18] perf header: Fix various error path memory leaks

[Ian Rogers]

Memory leaks were detected by clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/header.c | 63 ++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 25 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index d812e1e371a7..41b78e40b22b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2598,8 +2598,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
 			goto error;
 
 		/* include a NULL character at the end */
-		if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+		if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
+			free(str);
 			goto error;
+		}
 		size += string_size(str);
 		free(str);
 	}
@@ -2617,8 +2619,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
 			goto error;
 
 		/* include a NULL character at the end */
-		if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+		if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
+			free(str);
 			goto error;
+		}
 		size += string_size(str);
 		free(str);
 	}
@@ -2681,8 +2685,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
 			goto error;
 
 		/* include a NULL character at the end */
-		if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+		if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
+			free(str);
 			goto error;
+		}
 		size += string_size(str);
 		free(str);
 	}
@@ -2736,10 +2742,9 @@ static int process_numa_topology(struct feat_fd *ff, void *data __maybe_unused)
 			goto error;
 
 		n->map = perf_cpu_map__new(str);
+		free(str);
 		if (!n->map)
 			goto error;
-
-		free(str);
 	}
 	ff->ph->env.nr_numa_nodes = nr;
 	ff->ph->env.numa_nodes = nodes;
@@ -2913,10 +2918,10 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
 		return -1;
 
 	for (i = 0; i < cnt; i++) {
-		struct cpu_cache_level c;
+		struct cpu_cache_level *c = &caches[i];
 
 		#define _R(v)						\
-			if (do_read_u32(ff, &c.v))\
+			if (do_read_u32(ff, &c->v))			\
 				goto out_free_caches;			\
 
 		_R(level)
@@ -2926,22 +2931,25 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
 		#undef _R
 
 		#define _R(v)					\
-			c.v = do_read_string(ff);		\
-			if (!c.v)				\
-				goto out_free_caches;
+			c->v = do_read_string(ff);		\
+			if (!c->v)				\
+				goto out_free_caches;		\
 
 		_R(type)
 		_R(size)
 		_R(map)
 		#undef _R
-
-		caches[i] = c;
 	}
 
 	ff->ph->env.caches = caches;
 	ff->ph->env.caches_cnt = cnt;
 	return 0;
 out_free_caches:
+	for (i = 0; i < cnt; i++) {
+		free(caches[i].type);
+		free(caches[i].size);
+		free(caches[i].map);
+	}
 	free(caches);
 	return -1;
 }
@@ -3585,18 +3593,16 @@ static int perf_header__adds_write(struct perf_header *header,
 				   struct feat_copier *fc)
 {
 	int nr_sections;
-	struct feat_fd ff;
+	struct feat_fd ff = {
+		.fd  = fd,
+		.ph = header,
+	};
 	struct perf_file_section *feat_sec, *p;
 	int sec_size;
 	u64 sec_start;
 	int feat;
 	int err;
 
-	ff = (struct feat_fd){
-		.fd  = fd,
-		.ph = header,
-	};
-
 	nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
 	if (!nr_sections)
 		return 0;
@@ -3623,6 +3629,7 @@ static int perf_header__adds_write(struct perf_header *header,
 	err = do_write(&ff, feat_sec, sec_size);
 	if (err < 0)
 		pr_debug("failed to write feature section\n");
+	free(ff.buf);
 	free(feat_sec);
 	return err;
 }
@@ -3630,11 +3637,11 @@ static int perf_header__adds_write(struct perf_header *header,
 int perf_header__write_pipe(int fd)
 {
 	struct perf_pipe_file_header f_header;
-	struct feat_fd ff;
+	struct feat_fd ff = {
+		.fd = fd,
+	};
 	int err;
 
-	ff = (struct feat_fd){ .fd = fd };
-
 	f_header = (struct perf_pipe_file_header){
 		.magic	   = PERF_MAGIC,
 		.size	   = sizeof(f_header),
@@ -3645,7 +3652,7 @@ int perf_header__write_pipe(int fd)
 		pr_debug("failed to write perf pipe header\n");
 		return err;
 	}
-
+	free(ff.buf);
 	return 0;
 }
 
@@ -3658,11 +3665,12 @@ static int perf_session__do_write_header(struct perf_session *session,
 	struct perf_file_attr   f_attr;
 	struct perf_header *header = &session->header;
 	struct evsel *evsel;
-	struct feat_fd ff;
+	struct feat_fd ff = {
+		.fd = fd,
+	};
 	u64 attr_offset;
 	int err;
 
-	ff = (struct feat_fd){ .fd = fd};
 	lseek(fd, sizeof(f_header), SEEK_SET);
 
 	evlist__for_each_entry(session->evlist, evsel) {
@@ -3670,6 +3678,7 @@ static int perf_session__do_write_header(struct perf_session *session,
 		err = do_write(&ff, evsel->core.id, evsel->core.ids * sizeof(u64));
 		if (err < 0) {
 			pr_debug("failed to write perf header\n");
+			free(ff.buf);
 			return err;
 		}
 	}
@@ -3695,6 +3704,7 @@ static int perf_session__do_write_header(struct perf_session *session,
 		err = do_write(&ff, &f_attr, sizeof(f_attr));
 		if (err < 0) {
 			pr_debug("failed to write perf header attribute\n");
+			free(ff.buf);
 			return err;
 		}
 	}
@@ -3705,8 +3715,10 @@ static int perf_session__do_write_header(struct perf_session *session,
 
 	if (at_exit) {
 		err = perf_header__adds_write(header, evlist, fd, fc);
-		if (err < 0)
+		if (err < 0) {
+			free(ff.buf);
 			return err;
+		}
 	}
 
 	f_header = (struct perf_file_header){
@@ -3728,6 +3740,7 @@ static int perf_session__do_write_header(struct perf_session *session,
 
 	lseek(fd, 0, SEEK_SET);
 	err = do_write(&ff, &f_header, sizeof(f_header));
+	free(ff.buf);
 	if (err < 0) {
 		pr_debug("failed to write perf header\n");
 		return err;
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH v2 17/18] perf header: Fix various error path memory leaks

[Namhyung Kim]

On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
>
> Memory leaks were detected by clang-tidy.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/header.c | 63 ++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index d812e1e371a7..41b78e40b22b 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2598,8 +2598,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
>                         goto error;
>
>                 /* include a NULL character at the end */
> -               if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> +               if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
> +                       free(str);
>                         goto error;
> +               }
>                 size += string_size(str);
>                 free(str);
>         }
> @@ -2617,8 +2619,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
>                         goto error;
>
>                 /* include a NULL character at the end */
> -               if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> +               if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
> +                       free(str);
>                         goto error;
> +               }
>                 size += string_size(str);
>                 free(str);
>         }
> @@ -2681,8 +2685,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
>                         goto error;
>
>                 /* include a NULL character at the end */
> -               if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> +               if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
> +                       free(str);
>                         goto error;
> +               }
>                 size += string_size(str);
>                 free(str);
>         }

For these cases, it'd be simpler to free it in the error path.


> @@ -2736,10 +2742,9 @@ static int process_numa_topology(struct feat_fd *ff, void *data __maybe_unused)
>                         goto error;
>
>                 n->map = perf_cpu_map__new(str);
> +               free(str);
>                 if (!n->map)
>                         goto error;
> -
> -               free(str);
>         }
>         ff->ph->env.nr_numa_nodes = nr;
>         ff->ph->env.numa_nodes = nodes;
> @@ -2913,10 +2918,10 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
>                 return -1;
>
>         for (i = 0; i < cnt; i++) {
> -               struct cpu_cache_level c;
> +               struct cpu_cache_level *c = &caches[i];
>
>                 #define _R(v)                                           \
> -                       if (do_read_u32(ff, &c.v))\
> +                       if (do_read_u32(ff, &c->v))                     \
>                                 goto out_free_caches;                   \
>
>                 _R(level)
> @@ -2926,22 +2931,25 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
>                 #undef _R
>
>                 #define _R(v)                                   \
> -                       c.v = do_read_string(ff);               \
> -                       if (!c.v)                               \
> -                               goto out_free_caches;
> +                       c->v = do_read_string(ff);              \
> +                       if (!c->v)                              \
> +                               goto out_free_caches;           \
>
>                 _R(type)
>                 _R(size)
>                 _R(map)
>                 #undef _R
> -
> -               caches[i] = c;
>         }
>
>         ff->ph->env.caches = caches;
>         ff->ph->env.caches_cnt = cnt;
>         return 0;
>  out_free_caches:
> +       for (i = 0; i < cnt; i++) {
> +               free(caches[i].type);
> +               free(caches[i].size);
> +               free(caches[i].map);
> +       }
>         free(caches);
>         return -1;
>  }

Looks ok.


> @@ -3585,18 +3593,16 @@ static int perf_header__adds_write(struct perf_header *header,
>                                    struct feat_copier *fc)
>  {
>         int nr_sections;
> -       struct feat_fd ff;
> +       struct feat_fd ff = {
> +               .fd  = fd,
> +               .ph = header,
> +       };

I'm fine with this change.


>         struct perf_file_section *feat_sec, *p;
>         int sec_size;
>         u64 sec_start;
>         int feat;
>         int err;
>
> -       ff = (struct feat_fd){
> -               .fd  = fd,
> -               .ph = header,
> -       };
> -
>         nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
>         if (!nr_sections)
>                 return 0;
> @@ -3623,6 +3629,7 @@ static int perf_header__adds_write(struct perf_header *header,
>         err = do_write(&ff, feat_sec, sec_size);
>         if (err < 0)
>                 pr_debug("failed to write feature section\n");
> +       free(ff.buf);

But it looks like false alarams.  Since the feat_fd has fd
and no buf, it won't allocate the buffer in do_write() and
just use __do_write_fd().  So no need to free the buf.

Thanks,
Namhyung


>         free(feat_sec);
>         return err;
>  }
> @@ -3630,11 +3637,11 @@ static int perf_header__adds_write(struct perf_header *header,
>  int perf_header__write_pipe(int fd)
>  {
>         struct perf_pipe_file_header f_header;
> -       struct feat_fd ff;
> +       struct feat_fd ff = {
> +               .fd = fd,
> +       };
>         int err;
>
> -       ff = (struct feat_fd){ .fd = fd };
> -
>         f_header = (struct perf_pipe_file_header){
>                 .magic     = PERF_MAGIC,
>                 .size      = sizeof(f_header),
> @@ -3645,7 +3652,7 @@ int perf_header__write_pipe(int fd)
>                 pr_debug("failed to write perf pipe header\n");
>                 return err;
>         }
> -
> +       free(ff.buf);
>         return 0;
>  }
>
> @@ -3658,11 +3665,12 @@ static int perf_session__do_write_header(struct perf_session *session,
>         struct perf_file_attr   f_attr;
>         struct perf_header *header = &session->header;
>         struct evsel *evsel;
> -       struct feat_fd ff;
> +       struct feat_fd ff = {
> +               .fd = fd,
> +       };
>         u64 attr_offset;
>         int err;
>
> -       ff = (struct feat_fd){ .fd = fd};
>         lseek(fd, sizeof(f_header), SEEK_SET);
>
>         evlist__for_each_entry(session->evlist, evsel) {
> @@ -3670,6 +3678,7 @@ static int perf_session__do_write_header(struct perf_session *session,
>                 err = do_write(&ff, evsel->core.id, evsel->core.ids * sizeof(u64));
>                 if (err < 0) {
>                         pr_debug("failed to write perf header\n");
> +                       free(ff.buf);
>                         return err;
>                 }
>         }
> @@ -3695,6 +3704,7 @@ static int perf_session__do_write_header(struct perf_session *session,
>                 err = do_write(&ff, &f_attr, sizeof(f_attr));
>                 if (err < 0) {
>                         pr_debug("failed to write perf header attribute\n");
> +                       free(ff.buf);
>                         return err;
>                 }
>         }
> @@ -3705,8 +3715,10 @@ static int perf_session__do_write_header(struct perf_session *session,
>
>         if (at_exit) {
>                 err = perf_header__adds_write(header, evlist, fd, fc);
> -               if (err < 0)
> +               if (err < 0) {
> +                       free(ff.buf);
>                         return err;
> +               }
>         }
>
>         f_header = (struct perf_file_header){
> @@ -3728,6 +3740,7 @@ static int perf_session__do_write_header(struct perf_session *session,
>
>         lseek(fd, 0, SEEK_SET);
>         err = do_write(&ff, &f_header, sizeof(f_header));
> +       free(ff.buf);
>         if (err < 0) {
>                 pr_debug("failed to write perf header\n");
>                 return err;
> --
> 2.42.0.609.gbb76f46606-goog
>

Re: [PATCH v2 17/18] perf header: Fix various error path memory leaks

[Ian Rogers]

On Sun, Oct 8, 2023 at 11:57 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Memory leaks were detected by clang-tidy.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/header.c | 63 ++++++++++++++++++++++++----------------
> >  1 file changed, 38 insertions(+), 25 deletions(-)
> >
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index d812e1e371a7..41b78e40b22b 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -2598,8 +2598,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
> >                         goto error;
> >
> >                 /* include a NULL character at the end */
> > -               if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> > +               if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
> > +                       free(str);
> >                         goto error;
> > +               }
> >                 size += string_size(str);
> >                 free(str);
> >         }
> > @@ -2617,8 +2619,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
> >                         goto error;
> >
> >                 /* include a NULL character at the end */
> > -               if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> > +               if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
> > +                       free(str);
> >                         goto error;
> > +               }
> >                 size += string_size(str);
> >                 free(str);
> >         }
> > @@ -2681,8 +2685,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
> >                         goto error;
> >
> >                 /* include a NULL character at the end */
> > -               if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> > +               if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
> > +                       free(str);
> >                         goto error;
> > +               }
> >                 size += string_size(str);
> >                 free(str);
> >         }
>
> For these cases, it'd be simpler to free it in the error path.

It was a slightly larger change to do it that way, but I'll alter it.
The issue is not getting a double free on str, which is most easily
handled by switching frees to zfrees.


>
>
> > @@ -2736,10 +2742,9 @@ static int process_numa_topology(struct feat_fd *ff, void *data __maybe_unused)
> >                         goto error;
> >
> >                 n->map = perf_cpu_map__new(str);
> > +               free(str);
> >                 if (!n->map)
> >                         goto error;
> > -
> > -               free(str);
> >         }
> >         ff->ph->env.nr_numa_nodes = nr;
> >         ff->ph->env.numa_nodes = nodes;
> > @@ -2913,10 +2918,10 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
> >                 return -1;
> >
> >         for (i = 0; i < cnt; i++) {
> > -               struct cpu_cache_level c;
> > +               struct cpu_cache_level *c = &caches[i];
> >
> >                 #define _R(v)                                           \
> > -                       if (do_read_u32(ff, &c.v))\
> > +                       if (do_read_u32(ff, &c->v))                     \
> >                                 goto out_free_caches;                   \
> >
> >                 _R(level)
> > @@ -2926,22 +2931,25 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
> >                 #undef _R
> >
> >                 #define _R(v)                                   \
> > -                       c.v = do_read_string(ff);               \
> > -                       if (!c.v)                               \
> > -                               goto out_free_caches;
> > +                       c->v = do_read_string(ff);              \
> > +                       if (!c->v)                              \
> > +                               goto out_free_caches;           \
> >
> >                 _R(type)
> >                 _R(size)
> >                 _R(map)
> >                 #undef _R
> > -
> > -               caches[i] = c;
> >         }
> >
> >         ff->ph->env.caches = caches;
> >         ff->ph->env.caches_cnt = cnt;
> >         return 0;
> >  out_free_caches:
> > +       for (i = 0; i < cnt; i++) {
> > +               free(caches[i].type);
> > +               free(caches[i].size);
> > +               free(caches[i].map);
> > +       }
> >         free(caches);
> >         return -1;
> >  }
>
> Looks ok.
>
>
> > @@ -3585,18 +3593,16 @@ static int perf_header__adds_write(struct perf_header *header,
> >                                    struct feat_copier *fc)
> >  {
> >         int nr_sections;
> > -       struct feat_fd ff;
> > +       struct feat_fd ff = {
> > +               .fd  = fd,
> > +               .ph = header,
> > +       };
>
> I'm fine with this change.
>
>
> >         struct perf_file_section *feat_sec, *p;
> >         int sec_size;
> >         u64 sec_start;
> >         int feat;
> >         int err;
> >
> > -       ff = (struct feat_fd){
> > -               .fd  = fd,
> > -               .ph = header,
> > -       };
> > -
> >         nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
> >         if (!nr_sections)
> >                 return 0;
> > @@ -3623,6 +3629,7 @@ static int perf_header__adds_write(struct perf_header *header,
> >         err = do_write(&ff, feat_sec, sec_size);
> >         if (err < 0)
> >                 pr_debug("failed to write feature section\n");
> > +       free(ff.buf);
>
> But it looks like false alarams.  Since the feat_fd has fd
> and no buf, it won't allocate the buffer in do_write() and
> just use __do_write_fd().  So no need to free the buf.

This code looks like it has had a half-done optimization applied to it
- why have >1 buffer? why are we making the code's life hard? In
__do_write_buf there is a test that can be true even when a buf is
provided:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.c?h=perf-tools-next#n125
```
if (ff->size < new_size) {
        addr = realloc(ff->buf, new_size);
        if (!addr)
                return -ENOMEM;
        ff->buf = addr;
```
In the case clang-tidy (I'll put the analysis below) has determined
that new_size may be greater than size (I believe the intent in the
code is they both evaluate to 0) and this causes the memory leak being
fixed here. I'll add a TODO comment in v3 but things like 'buf' are
opaque and not intention revealing in the code, which makes any kind
of interpretation hard.

I think again this is a good signal that there is worthwhile
simplification/cleanup in this code.

Thanks,
Ian

```
tools/perf/util/header.c:3628:6: warning: Potential leak of memory
pointed to by 'ff.buf' [clang-analyzer-unix.Malloc]
        err = do_write(&ff, feat_sec, sec_size);
            ^
tools/perf/util/header.c:3778:9: note: Calling 'perf_session__do_write_header'
        return perf_session__do_write_header(session, evlist, fd, true, fc);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3675:2: note: Loop condition is false.
Execution continues on line 3685
        evlist__for_each_entry(session->evlist, evsel) {
        ^
tools/perf/util/evlist.h:276:2: note: expanded from macro
'evlist__for_each_entry'
        __evlist__for_each_entry(&(evlist)->core.entries, evsel)
        ^
tools/perf/util/evlist.h:268:9: note: expanded from macro
'__evlist__for_each_entry'
        list_for_each_entry(evsel, list, core.node)
        ^
tools/include/linux/list.h:458:2: note: expanded from macro
'list_for_each_entry'
        for (pos = list_first_entry(head, typeof(*pos), member);        \
        ^
tools/perf/util/header.c:3687:2: note: Loop condition is false.
Execution continues on line 3711
        evlist__for_each_entry(evlist, evsel) {
        ^
tools/perf/util/evlist.h:276:2: note: expanded from macro
'evlist__for_each_entry'
        __evlist__for_each_entry(&(evlist)->core.entries, evsel)
        ^
tools/perf/util/evlist.h:268:9: note: expanded from macro
'__evlist__for_each_entry'
        list_for_each_entry(evsel, list, core.node)
        ^
tools/include/linux/list.h:458:2: note: expanded from macro
'list_for_each_entry'
        for (pos = list_first_entry(head, typeof(*pos), member);        \
        ^
tools/perf/util/header.c:3711:6: note: Assuming field 'data_offset' is
not equal to 0
        if (!header->data_offset)
            ^~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3711:2: note: Taking false branch
        if (!header->data_offset)
        ^
tools/perf/util/header.c:3715:6: note: 'at_exit' is true
        if (at_exit) {
            ^~~~~~~
tools/perf/util/header.c:3715:2: note: Taking true branch
        if (at_exit) {
        ^
tools/perf/util/header.c:3716:9: note: Calling 'perf_header__adds_write'
                err = perf_header__adds_write(header, evlist, fd, fc);
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3606:6: note: Assuming 'nr_sections' is not equal
 to 0
        if (!nr_sections)
            ^~~~~~~~~~~~
tools/perf/util/header.c:3606:2: note: Taking false branch
        if (!nr_sections)
        ^
tools/perf/util/header.c:3610:6: note: Assuming 'feat_sec' is not equal to
 NULL
        if (feat_sec == NULL)
            ^~~~~~~~~~~~~~~~
tools/perf/util/header.c:3610:2: note: Taking false branch
        if (feat_sec == NULL)
        ^
tools/perf/util/header.c:3618:2: note: Assuming 'feat' is < HEADER_FEAT_BITS
        for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
        ^
tools/include/linux/bitops.h:54:7: note: expanded from macro 'for_each_set_bit'
             (bit) < (size);                                    \
             ^~~~~~~~~~~~~~
tools/perf/util/header.c:3618:2: note: Loop condition is true.
Entering loop body
        for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
        ^
tools/include/linux/bitops.h:53:2: note: expanded from macro 'for_each_set_bit'
        for ((bit) = find_first_bit((addr), (size));            \
        ^
tools/perf/util/header.c:3619:3: note: Taking true branch
                if (do_write_feat(&ff, feat, &p, evlist, fc))
                ^
tools/perf/util/header.c:3618:2: note: Assuming 'feat' is >= HEADER_FEAT_BITS
        for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
tools/include/linux/bitops.h:54:7: note: expanded from macro 'for_each_set_bit'
             (bit) < (size);                                    \
             ^~~~~~~~~~~~~~
tools/perf/util/header.c:3618:2: note: Loop condition is false.
Execution continues on line 3623
        for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
        ^
tools/include/linux/bitops.h:53:2: note: expanded from macro 'for_each_set_bit'
        for ((bit) = find_first_bit((addr), (size));            \
        ^
tools/perf/util/header.c:3628:8: note: Calling 'do_write'
        err = do_write(&ff, feat_sec, sec_size);
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:141:6: note: Assuming field 'buf' is non-null
        if (!ff->buf)
            ^~~~~~~~
tools/perf/util/header.c:141:2: note: Taking false branch
        if (!ff->buf)
        ^
tools/perf/util/header.c:143:9: note: Calling '__do_write_buf'
        return __do_write_buf(ff, buf, size);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:117:6: note: Assuming the condition is false
        if (size + ff->offset > max_size)
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:117:2: note: Taking false branch
        if (size + ff->offset > max_size)
        ^
tools/perf/util/header.c:120:9: note: Assuming the condition is true
        while (size > (new_size - ff->offset))
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:120:2: note: Loop condition is true.
Entering loop body
        while (size > (new_size - ff->offset))
        ^
tools/perf/util/header.c:120:9: note: Assuming the condition is false
        while (size > (new_size - ff->offset))
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:120:2: note: Loop condition is false.
Execution continues on line 122
        while (size > (new_size - ff->offset))
        ^
tools/perf/util/header.c:122:13: note: Assuming '_min1' is >= '_min2'
        new_size = min(max_size, new_size);
                   ^
tools/include/linux/kernel.h:53:2: note: expanded from macro 'min'
        _min1 < _min2 ? _min1 : _min2; })
        ^~~~~~~~~~~~~
tools/perf/util/header.c:122:13: note: '?' condition is false
        new_size = min(max_size, new_size);
                   ^
tools/include/linux/kernel.h:53:2: note: expanded from macro 'min'
        _min1 < _min2 ? _min1 : _min2; })
        ^
tools/perf/util/header.c:124:6: note: Assuming 'new_size' is > field 'size'
        if (ff->size < new_size) {
            ^~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:124:2: note: Taking true branch
        if (ff->size < new_size) {
        ^
tools/perf/util/header.c:125:10: note: Memory is allocated
                addr = realloc(ff->buf, new_size);
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:126:7: note: Assuming 'addr' is non-null
                if (!addr)
                    ^~~~~
tools/perf/util/header.c:126:3: note: Taking false branch
                if (!addr)
                ^
tools/perf/util/header.c:143:9: note: Returned allocated memory
        return __do_write_buf(ff, buf, size);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3628:8: note: Returned allocated memory
        err = do_write(&ff, feat_sec, sec_size);
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3628:6: note: Potential leak of memory
pointed to by 'ff.buf'
        err = do_write(&ff, feat_sec, sec_size);
```


> Thanks,
> Namhyung
>
>
> >         free(feat_sec);
> >         return err;
> >  }
> > @@ -3630,11 +3637,11 @@ static int perf_header__adds_write(struct perf_header *header,
> >  int perf_header__write_pipe(int fd)
> >  {
> >         struct perf_pipe_file_header f_header;
> > -       struct feat_fd ff;
> > +       struct feat_fd ff = {
> > +               .fd = fd,
> > +       };
> >         int err;
> >
> > -       ff = (struct feat_fd){ .fd = fd };
> > -
> >         f_header = (struct perf_pipe_file_header){
> >                 .magic     = PERF_MAGIC,
> >                 .size      = sizeof(f_header),
> > @@ -3645,7 +3652,7 @@ int perf_header__write_pipe(int fd)
> >                 pr_debug("failed to write perf pipe header\n");
> >                 return err;
> >         }
> > -
> > +       free(ff.buf);
> >         return 0;
> >  }
> >
> > @@ -3658,11 +3665,12 @@ static int perf_session__do_write_header(struct perf_session *session,
> >         struct perf_file_attr   f_attr;
> >         struct perf_header *header = &session->header;
> >         struct evsel *evsel;
> > -       struct feat_fd ff;
> > +       struct feat_fd ff = {
> > +               .fd = fd,
> > +       };
> >         u64 attr_offset;
> >         int err;
> >
> > -       ff = (struct feat_fd){ .fd = fd};
> >         lseek(fd, sizeof(f_header), SEEK_SET);
> >
> >         evlist__for_each_entry(session->evlist, evsel) {
> > @@ -3670,6 +3678,7 @@ static int perf_session__do_write_header(struct perf_session *session,
> >                 err = do_write(&ff, evsel->core.id, evsel->core.ids * sizeof(u64));
> >                 if (err < 0) {
> >                         pr_debug("failed to write perf header\n");
> > +                       free(ff.buf);
> >                         return err;
> >                 }
> >         }
> > @@ -3695,6 +3704,7 @@ static int perf_session__do_write_header(struct perf_session *session,
> >                 err = do_write(&ff, &f_attr, sizeof(f_attr));
> >                 if (err < 0) {
> >                         pr_debug("failed to write perf header attribute\n");
> > +                       free(ff.buf);
> >                         return err;
> >                 }
> >         }
> > @@ -3705,8 +3715,10 @@ static int perf_session__do_write_header(struct perf_session *session,
> >
> >         if (at_exit) {
> >                 err = perf_header__adds_write(header, evlist, fd, fc);
> > -               if (err < 0)
> > +               if (err < 0) {
> > +                       free(ff.buf);
> >                         return err;
> > +               }
> >         }
> >
> >         f_header = (struct perf_file_header){
> > @@ -3728,6 +3740,7 @@ static int perf_session__do_write_header(struct perf_session *session,
> >
> >         lseek(fd, 0, SEEK_SET);
> >         err = do_write(&ff, &f_header, sizeof(f_header));
> > +       free(ff.buf);
> >         if (err < 0) {
> >                 pr_debug("failed to write perf header\n");
> >                 return err;
> > --
> > 2.42.0.609.gbb76f46606-goog
> >

[PATCH v2 18/18] perf bpf_counter: Fix a few memory leaks

[Ian Rogers]

Memory leaks were detected by clang-tidy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/bpf_counter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index 6732cbbcf9b3..7f9b0e46e008 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c
@@ -104,7 +104,7 @@ static int bpf_program_profiler_load_one(struct evsel *evsel, u32 prog_id)
 	struct bpf_prog_profiler_bpf *skel;
 	struct bpf_counter *counter;
 	struct bpf_program *prog;
-	char *prog_name;
+	char *prog_name = NULL;
 	int prog_fd;
 	int err;
 
@@ -155,10 +155,12 @@ static int bpf_program_profiler_load_one(struct evsel *evsel, u32 prog_id)
 	assert(skel != NULL);
 	counter->skel = skel;
 	list_add(&counter->list, &evsel->bpf_counter_list);
+	free(prog_name);
 	close(prog_fd);
 	return 0;
 err_out:
 	bpf_prog_profiler_bpf__destroy(skel);
+	free(prog_name);
 	free(counter);
 	close(prog_fd);
 	return -1;
@@ -180,6 +182,7 @@ static int bpf_program_profiler__load(struct evsel *evsel, struct target *target
 		    (*p != '\0' && *p != ',')) {
 			pr_err("Failed to parse bpf prog ids %s\n",
 			       target->bpf_str);
+			free(bpf_str_);
 			return -1;
 		}
 
-- 
2.42.0.609.gbb76f46606-goog