linux-kernel - Re: [PATCH v2] tracing/histograms: Fix memory leak problem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 12 Jul 2022 19:48:44 +0800
From:   Zheng Yejian <zhengyejian1@...wei.com>
To:     <rostedt@...dmis.org>
CC:     <linux-kernel@...r.kernel.org>, <mingo@...hat.com>,
        <stable@...r.kernel.org>, <tom.zanussi@...ux.intel.com>,
        <trix@...hat.com>, <zhangjinhao2@...wei.com>,
        <zhengyejian1@...wei.com>
Subject: Re: [PATCH v2] tracing/histograms: Fix memory leak problem

On Mon, 11 Jul 2022 11:52:11 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:
> On Mon, 11 Jul 2022 09:27:44 -0500
> "Zanussi, Tom" <tom.zanussi@...ux.intel.com> wrote:
> 
> > So I'm wondering if this means that that the original unnecessary bugfix
> > was based on a bug in the clang static analyzer or if that would just be
> > considered a false positive...
> 
> Good question. I'd like to know this, as if it is the case, I want to
> report that in my pull request to Linus.
> 
> -- Steve

I didn't use clang static analyzer before, but from its home page,
'False Positives' seems to exist, see https://clang-analyzer.llvm.org/:
    > Static analysis is not perfect. It can falsely flag bugs in a program
    > where the code behaves correctly. Because some code checks require more
    > analysis precision than others, the frequency of false positives can
    > vary widely between different checks. Our long-term goal is to have the
    > analyzer have a low false positive rate for most code on all checks.

So I try the clang-14 which comes from
https://github.com/llvm/llvm-project/releases/tag/llvmorg-14.0.0,
then execute like:
  $ scan-build make -j16

Then I take a rough look at following warnings related to 'trace_events_hist.c'
(serial number is manually added, no double free warning, maybe due to clang version):
  1. kernel/trace/trace_events_hist.c:1540:6: warning: Branch condition evaluates to a garbage value [core.uninitialized.Branch]
          if (!attrs->keys_str) {
              ^~~~~~~~~~~~~~~~
  2. kernel/trace/trace_events_hist.c:1580:9: warning: Array access (via field 'field_var_str') results in a null pointer dereference [core.NullDereference]
                  kfree(elt_data->field_var_str[i]);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~
  3. kernel/trace/trace_events_hist.c:1898:3: warning: 1st function call argument is an uninitialized value [core.CallAndMessage]
                  destroy_hist_field(hist_field->operands[i], level + 1);
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  4. kernel/trace/trace_events_hist.c:2095:2: warning: 1st function call argument is an uninitialized value [core.CallAndMessage]
          kfree(ref_field->system);
          ^~~~~~~~~~~~~~~~~~~~~~~~
  5. kernel/trace/trace_events_hist.c:2099:2: warning: 1st function call argument is an uninitialized value [core.CallAndMessage]
          kfree(ref_field->name);
          ^~~~~~~~~~~~~~~~~~~~~~
  6. kernel/trace/trace_events_hist.c:2158:4: warning: Potential leak of memory pointed to by 'ref_field' [unix.Malloc]
                          return NULL;

Since I'm not very familiar with trace_events_hist.c, I roughly conclude that:
  1. warning 1/3/6 are plausible but false-positive;
  2. warning 2/4/5 seems positive although they don't cause practical problems because
elt_data->field_var_str[i] / ref_field->system / ref_field->name can be 'NULL'
on 'kfree'. Do we need to explicitly check 'NULL' there?