On Mon, 2025-11-03 at 10:15 -0500, David Malcolm wrote:
> On Mon, 2025-11-03 at 10:46 +0100, Kamil Dudka wrote:
> > On Wednesday, 29 October 2025 21:20:12 CET Stephen Gallagher wrote:
> > > On Wed, Oct 29, 2025 at 6:00 AM Siteshwar Vashisht
> > > <[email protected]>
> > > wrote:
> > > 
> > > > Hello,
> > > > 
> > > > I am writing this message to get feedback from the community on
> > > > new
> > > > findings by static analyzers in Critical Path Packages that
> > > > have
> > > > changed in Fedora 44.
> > > > 
> > > > TLDR: This report[1] contains a total of 47352 findings and 843
> > > > new
> > > > findings identified since Fedora 43. Please review the report
> > > > and
> > > > provide feedback. False positives can now be recorded in the
> > > > known-false-positives[5] repository.
> > > > 
> > > > A mass scan was performed on the packages that have changed in
> > > > Fedora
> > > > 44. This report[1] contains all the findings that have been
> > > > identified
> > > > in the Critical Path Packages. Newly added findings since
> > > > Fedora
> > > > 43
> > > > are listed under ‘+’ column and these should be prioritized
> > > > while
> > > > reviewing the findings (and fixing them upstream). Not all
> > > > findings
> > > > reported by OpenScanHub may be actual bugs, so please verify
> > > > reported
> > > > findings before investing time into fixing or reporting them.
> > > > We
> > > > have
> > > > used the current development version of GCC to perform the
> > > > scans,
> > > > which may increase the likelihood of having false positives in
> > > > the GCC
> > > > reports.
> > > > 
> > > > False positives can now be recorded in the known-false-
> > > > positives[5]
> > > > repository. These findings are automatically suppressed by
> > > > OpenScanHub
> > > > in scans that are triggered later. Also, you can filter
> > > > findings
> > > > with
> > > > the csgrep utility to make it easier to review reports that may
> > > > contain a large amount of false positives. Examples of csgrep
> > > > invocation are available on the Fedora wiki[4].
> > > > 
> > > > We hope this is helpful for the packages you maintain and for
> > > > the
> > > > upstream projects. Questions can be asked on the OpenScanHub
> > > > mailing
> > > > list[2]. If you want to see the full logs of the scans, they
> > > > are
> > > > available on the tasks[3] page. User documentation for
> > > > performing
> > > > a
> > > > scan is available on the Fedora wiki[4].
> > > > 
> > > > Please keep the feedback on this thread constructive. Thank
> > > > you!
> > > > 
> > > > [1]
> > > > https://svashisht.fedorapeople.org/openscanhub/mass-scans/f44-28-Oct-2025/
> > > > 
> > > > [2]
> > > > https://lists.fedoraproject.org/archives/list/[email protected]/
> > > > 
> > > > [3] https://openscanhub.fedoraproject.org/task/
> > > > 
> > > > [4] https://fedoraproject.org/wiki/OpenScanHub
> > > > 
> > > > [5] https://github.com/openscanhub/known-false-positives
> > > > 
> > > > --
> > > > 
> > > 
> > > 
> > > I'm pretty sure
> > > https://svashisht.fedorapeople.org/openscanhub/mass-scans/f44-28-Oct-2025/sscg-4.0.0-1.fc44/added.html
> > > is a false-positive. The line that it claims "leaks" isn't an
> > > exit
> > > to the
> > > function and the memory is freed just a few lines later. I'm not
> > > sure why
> > > OSH thinks that there's a problem. The BIO_read() function from
> > > OpenSSL is
> > > essentially just a memcpy() into the buffer that was passed in.
> > > 
> > > (FWIW, this is also only in a unit test; there's no impact to the
> > > actual
> > > delivered package.)
> > 
> > I agree this is a false positive.  It does not happen with gcc-
> > 15.2.1-3.fc44.
> > It happens with gcc-latest-16.0.0-5.20250914git38666cbccff5.fc44
> > from
> > COPR.
> > 
> > Dave, any idea what went wrong?
> > 
> > The original SARIF file produced by GCC can be downloaded with the
> > following
> > command:
> > 
> > curl -s
> > '
> > https://openscanhub.fedoraproject.org/task/78401/log/sscg-4.0.0-1.fc
> > 44.tar.xz?format=raw' \
> >     | xz -cd \
> >     | tar -xv sscg-4.0.0-1.fc44/debug/raw-results/builddir/gcc-
> > results/452-M3DZ.sarif
> 
> $ sarif-replay sscg-4.0.0-1.fc44/debug/raw-results/builddir/gcc-
> results/452-M3DZ.sarif
> In function 'verify_name_constraints':
> ../test/create_cert_test.c:118:3: warning: leak of ‘ext_str’ [-
> Wanalyzer-malloc-leak]
>    38 | #include <openssl/x509.h>
> ......
>   118 |   BIO_read (bio, ext_str, ext_len);
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   'verify_name_constraints': events 1-18
>    61 |   if (ext_idx < 0)
>       |      ^
>       |      |
>       |      (1) following ‘false’ branch...
> ......
>    69 |   name_constraints_ext = X509_get_ext (x509, ext_idx);
>       |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |                          |
>       |                          (2) ...to here
>    70 |   if (!name_constraints_ext)
>       |      ~
>       |      |
>       |      (3) following ‘false’ branch...
> ......
>    77 |   ext_data = X509_EXTENSION_get_data (name_constraints_ext);
>       |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |              |
>       |              (4) ...to here
>    78 |   if (!ext_data)
>       |      ~
>       |      |
>       |      (5) following ‘false’ branch...
> ......
>    85 |   bio = BIO_new (BIO_s_mem ());
>       |         ~~~~~~~~~~~~~~~~~~~~~~
>       |         |
>       |         (6) ...to here
>    86 |   if (!bio)
>       |      ~Is there a way 
>       |      |
>       |      (7) following ‘false’ branch...
> ......
>    93 |   if (!X509V3_EXT_print (bio, name_constraints_ext, 0, 0))
>       |      ~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |      | |
>       |      | (8) ...to here
>       |      (9) following ‘false’ branch...
> ......
>   101 |   ext_len = BIO_pending (bio);
>       |             ~
>       |             |
>       |             (10) ...to here
>   102 |   if (ext_len <= 0)
>       |      ~
>       |      |
>       |      (11) following ‘false’ branch (when ‘ext_len > 0’)...
> ......
>   109 |   ext_str = malloc (ext_len + 1);
>       |             ~~~~~~~~~~~~~~~~~~~~
>       |             |       |
>       |             |       (12) ...to here
>       |             (13) allocated here
>   110 |   if (!ext_str)
>       |      ~
>       |      |
>       |      (14) assuming ‘ext_str’ is non-NULL
>       |      (15) following ‘false’ branch (when ‘ext_str’ is non-
> NULL)...
> ......
>   118 |   BIO_read (bio, ext_str, ext_len);
>       |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |   |
>       |   (16) ...to here
>       |   (17) if ‘BIO_read’ throws an exception...
>       |   (18) ‘ext_str’ leaks here; was allocated at (13)
> 
> One of the things I've implemented for GCC 16 is exception-handling
> support in the analyzer.  Looking at the above, GCC's -fanalyzer
> "thinks" that BIO_read could throw an exception, and sure, if it did,
> then that malloc at line 109 would leak.

Interestingly, looking at
https://svashisht.fedorapeople.org/openscanhub/mass-scans/f44-28-Oct-2025/sscg-4.0.0-1.fc44/added.html
the test printing of the execution path there seems to have events (1)
to (16) and (18), but not event (17) which is the important one for
understanding what's going on.

In "Scan Properties" I see:
analyzer-version-gcc: 15.2.1
analyzer-version-gcc-analyzer: 16.0.0

I don't understand the distinction here; could it be showing gcc 15
results?

IMHO sarif-replay's way of printing this is much more readable than
what added.html is currently showing.  Is it possible to use sarif-
replay there? (I can help if you like).  But note that GCC 16 and its
sarif-replay also has HTML output now, as per:
  https://dmalcolm.fedorapeople.org/gcc/2025-11-03/452-M3DZ.html
below (and potentially can show extra debugging info when stepping
though the code with "j" and "k" if you supply "state-graphs=yes" when
adding the SARIF sink):
  -fdiagnostics-add-output=sarif:state-graphs=yes
see the example at:
https://dmalcolm.fedorapeople.org/gcc/2025-06-23/state-diagram-1.c.html

Dave

> 
> Looking in the SARIF file, I see "-fexceptions" in the command-line
> flags.  Is that deliberate?  Is this code meant to support
> exceptions?
> 
> FWIW I also made an HTML version of the above with:
> $ sarif-replay \
>   -fdiagnostics-add-output=experimental-html:file=452-M3DZ.html \
>   sscg-4.0.0-1.fc44/debug/raw-results/builddir/gcc-results/452-
> M3DZ.sarif
> 
> and uploaded it here:
>   https://dmalcolm.fedorapeople.org/gcc/2025-11-03/452-M3DZ.html
> Would it be useful if the mass scans provided this to package
> maintainers?
> (this HTML generation is new in GCC 16; you can use "j" and "k" to
> cycle through the events in the log)
> 
> Hope this is constructive
> Dave

-- 
_______________________________________________
devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
Fedora Code of Conduct: 
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/[email protected]
Do not reply to spam, report it: 
https://pagure.io/fedora-infrastructure/new_issue

Reply via email to