Bug 109014 - [13/14/15 Regression] -Wanalyzer-use-of-uninitialized-value seen in pcre2-10.42's pcre2test.c
Summary: [13/14/15 Regression] -Wanalyzer-use-of-uninitialized-value seen in pcre2-10....
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: analyzer (show other bugs)
Version: 13.0
: P2 normal
Target Milestone: 13.4
Assignee: David Malcolm
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-03-03 18:24 UTC by David Malcolm
Modified: 2024-05-21 09:14 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Partially reducer reproducer (10.92 KB, text/plain)
2023-03-03 18:24 UTC, David Malcolm
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Malcolm 2023-03-03 18:24:14 UTC
Created attachment 54579 [details]
Partially reducer reproducer

I'm about to commit a patch that provides a prototype implementation of sprintf; this leads to a new false positive on the attached:


pcre2test.c: In function 'format_list_item':
pcre2test.c:4448:21: warning: use of uninitialized value '*ff_44(D) + _2' [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
 4448 |   for (count = 0; ff[count] >= 0; count++) {
      |                   ~~^~~~~~~
  'main': events 1-12
    |
    | 4565 | main(int argc, char** argv)
    |      | ^~~~
    |      | |
    |      | (1) entry to 'main'
    |......
    | 4579 |   while (argc > 1 && argv[op][0] == '-' && argv[op][1] != 0) {
    |      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                                         |
    |      |                                         (2) following 'true' branch...
    |      |                                         (8) following 'true' branch...
    | 4580 |     /* [...snip...] */
    | 4581 |     char* arg = argv[op];
    |      |                     ~
    |      |                     |
    |      |                     (3) ...to here
    |      |                     (9) ...to here
    |......
    | 4585 |     if (strcmp(arg, "-LP") == 0) {
    |      |        ~
    |      |        |
    |      |        (4) following 'false' branch (when the strings are non-equal)...
    |      |        (10) following 'true' branch (when the strings are equal)...
    | 4586 |       display_properties(0);
    |      |       ~~~~~~~~~~~~~~~~~~~~~
    |      |       |
    |      |       (11) ...to here
    |      |       (12) calling 'display_properties' from 'main'
    |......
    | 4592 |     if (strcmp(arg, "-8") == 0) {
    |      |        ~~~~~~~~~~~~~~~~~~
    |      |        ||
    |      |        |(5) ...to here
    |      |        (6) following 'true' branch (when the strings are equal)...
    | 4593 |       test_mode = 8;
    |      |       ~~~~~~~~~~~~~
    |      |                 |
    |      |                 (7) ...to here
    |
    +--> 'display_properties': events 13-25
           |
           | 4481 | display_properties(BOOL wantscripts)
           |      | ^~~~~~~~~~~~~~~~~~
           |      | |
           |      | (13) entry to 'display_properties'
           |......
           | 4487 |   int16_t found[256][5 + 1];
           |      |           ~~~~~
           |      |           |
           |      |           (14) region created on stack here
           |......
           | 4492 |   if (wantscripts) {
           |      |      ~
           |      |      |
           |      |      (15) following 'false' branch (when 'wantscripts == 0')...
           |......
           | 4496 |     n = ucp_Bprop_Count;
           |      |     ~~~~~~~~~~~~~~~~~~~
           |      |       |
           |      |       (16) ...to here
           |......
           | 4500 |   for (size_t i = 0; i < utt_size; i++) {
           |      |                      ~~~~~~~~~~~~
           |      |                        |
           |      |                        (17) following 'true' branch...
           | 4501 |     int k;
           | 4502 |     int m = 0;
           |      |         ~
           |      |         |
           |      |         (18) ...to here
           |......
           | 4507 |     if (wantscripts) {
           |      |        ~
           |      |        |
           |      |        (19) following 'false' branch (when 'wantscripts == 0')...
           |......
           | 4511 |       if (t->type != 13)
           |      |          ~~~~~~~~
           |      |          | |
           |      |          | (20) ...to here
           |      |          (21) following 'true' branch...
           |      |          (22) ...to here
           |......
           | 4544 |   for (int k = 0; k < (n + 1) / 2; k++) {
           |      |                   ~~~~~~~~~~~~~~~
           |      |                     |
           |      |                     (23) following 'true' branch...
           |......
           | 4549 |     format_list_item(found[k], buff1, wantscripts);
           |      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |     |                     |
           |      |     |                     (24) ...to here
           |      |     (25) calling 'format_list_item' from 'display_properties'
           |
           +--> 'format_list_item': events 26-27
                  |
                  | 4441 | format_list_item(int16_t* ff, char* buff, BOOL isscript)
                  |      | ^~~~~~~~~~~~~~~~
                  |      | |
                  |      | (26) entry to 'format_list_item'
                  |......
                  | 4448 |   for (count = 0; ff[count] >= 0; count++) {
                  |      |                   ~~~~~~~~~
                  |      |                     |
                  |      |                     (27) use of uninitialized value '*ff_44(D) + _2' here
                  |
Comment 1 David Malcolm 2023-03-03 18:29:54 UTC
I believe the issue here is that:

* display_properties partially initializes the "found" buffer, writing a -1 terminator at the end of the initialized part at:

    fv[m] = -1;

* display_properties then calls format_list_item, which tries to find the terminator with:

 for (count = 0; ff[count] >= 0; count++) {

* -fanalyzer isn't smart enough to know that a -1 terminator has been written, and simulates iterating past the end of the initialized region

and this is currently masked by the "sprintf" call, which in the absence of a known_function implementation is currently assumed to potentially write to fv.
Comment 2 GCC Commits 2023-03-03 23:01:03 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:56572a08ec4a0fc1a7802d3737cd7f7cc9089c4b

commit r13-6466-g56572a08ec4a0fc1a7802d3737cd7f7cc9089c4b
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Mar 3 17:59:21 2023 -0500

    analyzer: provide placeholder implementation of sprintf
    
    Previously, the analyzer lacked a known_function implementation of
    sprintf, and thus would handle calls to sprintf with the "anything could
    happen" fallback.
    
    Whilst working on PR analyzer/107565 I noticed that this was preventing
    a lot of genuine memory leaks from being reported for Doom; fixing
    thusly.
    
    Integration testing of the effect of the patch shows a big increase in
    true positives due to the case mentioned in Doom, and one new false
    positive (in pcre2), which I'm tracking as PR analyzer/109014.
    
    Comparison:
      GOOD:  67 -> 123 (+56); 10.91% -> 18.33%
       BAD: 547 -> 548 (+1)
    
    where the affected warnings/projects are:
    
      -Wanalyzer-malloc-leak:
        GOOD:  0 -> 56 (+56);  0.00% -> 41.48%
         BAD: 79
          True positives: 0 -> 56 (+56)
            (all in Doom)
    
      -Wanalyzer-use-of-uninitialized-value:
        GOOD: 0;  0.00%
         BAD: 80 -> 81 (+1)
          False positives:
            pcre2-10.42: 0 -> 1 (+1)
    
    gcc/analyzer/ChangeLog:
            * kf.cc (class kf_sprintf): New.
            (register_known_functions): Register it.
    
    gcc/testsuite/ChangeLog:
            * gcc.dg/analyzer/doom-d_main-IdentifyVersion.c: New test.
            * gcc.dg/analyzer/sprintf-1.c: New test.
            * gcc.dg/analyzer/sprintf-concat.c: New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Comment 3 Jakub Jelinek 2024-05-21 09:14:12 UTC
GCC 13.3 is being released, retargeting bugs to GCC 13.4.