[Bug analyzer/94858] New: False report of memory leak

[Bug analyzer/94858] New: False report of memory leak

[addw at phcomp dot co.uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94858

            Bug ID: 94858
           Summary: False report of memory leak
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: addw at phcomp dot co.uk
  Target Milestone: ---

Report of a memory leak - that does not happen.
If malloc()/realloc() fails then td->hs_index is left untouched. 

#include <stdlib.h>                                                             

#define FALSE   0                                                               
#define TRUE    1                                                               
#define HASH_EMPTY      -1                                                      

typedef short   hashNx;                                                         

typedef struct  hashSt  {                                                       
        hashNx* hs_index;       // Indicies into table - hashed to find index   
        int     hs_used;        // Entries used in hs_index                     
        int     hs_slots;       // Available slots in hs_index                  
} hashSt;                                                                       

void    hashEmpty(hashSt* td);                                                  

int     hashAlloc(hashSt* td, int slots)                                        
{                                                                               
        hashNx* index;                                                          

        // Is the index already at least that big ?                             
        if(slots > td->hs_slots) {                                              

                // New or reallocate ?                                          
                if(td->hs_index != NULL)                                        
                        index = realloc(td->hs_index, (size_t)slots *
sizeof(hashNx));                                
                else                                                            
                        index = malloc((size_t)slots * sizeof(hashNx));         

                if(index == NULL)                                               
                        return(FALSE);                                          

                // If we get here malloc()/realloc() worked                     
                td->hs_index = index;                                           
                td->hs_slots = slots;                                           
        }                                                                       

        hashEmpty(td);  // Clear the index                                      

        return(TRUE);                                                           
}                                                                               

/* Mark everything in the hash index as empty.                                  
 * This is useful if you have deleted something and need to reindex.            
 * It is OK to call this on a table that has not been initialised.              
 */                                                                             
void    hashEmpty(hashSt* td)                                                   
{                                                                               
        hashNx* index;                                                          
        int     slots;                                                          

        for(slots = td->hs_slots, index = td->hs_index; --slots >= 0; )         
                *index++ = HASH_EMPTY;                                          

        td->hs_used = 0;                                                        
}                                                                               


cc -O2 -Wall -Wno-pointer-sign -Wconversion -fanalyzer -c -o hasha.o hasha.c    

In function ‘hashAlloc’:                                                        
hasha.c:54:14: warning: leak of ‘index’ [CWE-401] [-Wanalyzer-malloc-leak]      
   54 |  td->hs_used = 0;                                                       
      |  ~~~~~~~~~~~~^~~                                                        
  ‘hashAlloc’: events 1-9                                                       
    |                                                                           
    |   22 |  if(slots > td->hs_slots) {                                        
    |      |    ^
    |      |    |
    |      |    (1) following ‘true’ branch...
    |......                                                                     
    |   25 |   if(td->hs_index != NULL)                                         
    |      |     ~~~~~~~~~~~~~                                                  
    |      |     |  |                                                           
    |      |     |  (2) ...to here
    |      |     (3) following ‘false’ branch...                                
    |......
    |   28 |    index = malloc((size_t)slots * sizeof(hashNx));                 
    |      |                   ~~~~~~~~~~~~~
    |      |                   |                                                
    |      |                   (4) ...to here                                   
    |      |                   (5) allocated here                               
    |   29 |
    |   30 |   if(index == NULL)
    |      |     ~                                                              
    |      |     |
    |      |     (6) assuming ‘index’ is non-NULL                               
    |      |     (7) following ‘false’ branch (when ‘index’ is non-NULL)...     
    |......
    |   33 |   td->hs_index = index;                                            
    |      |   ~~~~~~~~~~~~~~~~~~~~
    |      |                |
    |      |                (8) ...to here                                      
    |......                                                                     
    |   54 |  td->hs_used = 0;                                                  
    |      |  ~~~~~~~~~~~~~~~
    |      |              |                                                     
    |      |              (9) ‘index’ leaks here; was allocated at (5)          
    |

[Bug analyzer/94858] False report of memory leak

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94858

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |94839

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think this is an almost exact dup of bug 94839 (which I filed yesterday).


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94839
[Bug 94839] False positive with -fanalyzer and direct field assignment from
calloc

[Bug analyzer/94858] False report of memory leak

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94858
Bug 94858 depends on bug 94839, which changed state.

Bug 94839 Summary: False positive with -fanalyzer and direct field assignment from calloc
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94839

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

[Bug analyzer/94858] False report of memory leak

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94858

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-08-19
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks for filing this.  Still affects trunk (my rewrite of
r11-2694-g808f4dfeb3a95f50f15e71148e5c1067f90a126d didn't fix it).

Seems to be an issue with state-merging; passing -fno-analyzer-state-merge
eliminates the false positive.

Immediately before the call to hashEmpty we have:

  cluster for: (*INIT_VAL(td_14(D)))
    ESCAPED
    key:   {kind: direct, start: 0, size: 64, next: 64}
    value: ‘hashNx *’ {&HEAP_ALLOCATED_REGION(0)}
    key:   {kind: direct, start: 96, size: 32, next: 128}
    value: ‘int’ {INIT_VAL(slots_15(D))}
  malloc: &HEAP_ALLOCATED_REGION(0): nonnull (‘index_17’)

but afterwards (with state merging) we have:

  cluster for: (*INIT_VAL(td_14(D)))
    ESCAPED
    key:   {kind: direct, start: 0, size: 64, next: 64}
    value: ‘hashNx *’ {UNKNOWN(hashNx *)}
    key:   {kind: direct, start: 64, size: 32, next: 96}
    value: ‘int’ {(int)0}
    key:   {kind: direct, start: 96, size: 32, next: 128}
    value: ‘int’ {UNKNOWN(int)}
    key:   {kind: default, start: 0, size: 128, next: 128}
    value: ‘struct hashSt’ {UNKNOWN(struct hashSt)}

so the pointer (first 64 bits) has somehow become UNKNOWN.

[Bug analyzer/94858] False report of memory leak

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94858

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
It looks like we lose the:
        cluster for: (*INIT_VAL(td_14(D)))
          ESCAPED
          key:   {kind: direct, start: 0, size: 64, next: 64}
          value: ‘hashNx *’ {&HEAP_ALLOCATED_REGION(0)}
at the "*index.0_1 = -1;", where the whole cluster for *INIT_VAL(td_14(D))
becomes unknown.

I think the aliasing code conservatively assumes that this write could affect
the *INIT_VAL(td_14(D)).

But that was the initial value of a pointer param, whereas
HEAP_ALLOCATED_REGION is a freshly allocated heap region, so they can't alias.

Looks like I need to teach that to the pointer-aliasing logic.

[Bug analyzer/94858] False report of memory leak

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94858

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:2fc201382d3498778934f1262b57acd20f76f300

commit r11-2855-g2fc201382d3498778934f1262b57acd20f76f300
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Aug 21 18:34:16 2020 -0400

    analyzer: fix leak false positive/widening on pointer iteration [PR94858]

    PR analyzer/94858 reports a false diagnostic from
    -Wanalyzer-malloc-leak, where the allocated pointer is pointed to by a
    field of a struct, and a loop writes to a buffer, writing through an
    iterating pointer value.

    There were several underlying problems, relating to clobbering of the
    struct holding the malloc-ed pointer; in each case the analyzer was
    conservatively assuming that a write could affect this region,
    clobbering it to "unknown", and this was detected as a leak.

    The initial write within the loop dereferences the initial value of
    a field, and the analyzer was assuming that that pointer could
    point to the result of the malloc call.  The patch extends
    store::eval_alias_1 so that it "knows" that the initial value of a
    pointer at the beginning of a path can't point to a region that was
    allocated on the heap after the beginning of the path.

    On fixing that, the next issue is that within the loop the iterated
    pointer value becomes "unknown", and hence *ptr becomes a write to a
    symbolic region, and thus might clobber the struct (which it can't).
    This patch adds enough logic to svalue::can_merge_p to merge the
    iterating pointer value so that at the 2nd iteration analyzing
    the loop it becomes a widening_svalue from the initial svalue, so
    that this becomes a fixed point of the analysis, and is not an
    unknown_svalue.  The patch further extends store::eval_alias_1 so that
    it "knows" that this widening_svalue can only point to the same base
    region as the initial value did; in particular, symbolic writes through
    this pointer can only clobber that base region, not the struct holding
    the malloc-ed pointer.

    gcc/analyzer/ChangeLog:
            PR analyzer/94858
            * region-model-manager.cc
            (region_model_manager::get_or_create_widening_svalue): Assert that
            neither of the inputs are themselves widenings.
            * store.cc (store::eval_alias_1): The initial value of a pointer
            can't point to a region that was allocated on the heap after the
            beginning of the path.  A widened pointer value can't alias
anything
            that the initial pointer value can't alias.
            * svalue.cc (svalue::can_merge_p): Merge BINOP (X, OP, CST) with X
            to a widening svalue.  Merge
            BINOP(WIDENING(BASE, BINOP(BASE, X)), X) and BINOP(BASE, X) to
            to the LHS of the first BINOP.

    gcc/testsuite/ChangeLog:
            PR analyzer/94858
            * gcc.dg/analyzer/loop-start-up-to-end-by-1.c: Remove xfail.
            * gcc.dg/analyzer/pr94858-1.c: New test.
            * gcc.dg/analyzer/pr94858-2.c: New test.
            * gcc.dg/analyzer/torture/loop-inc-ptr-2.c: Update expected number
            of enodes.
            * gcc.dg/analyzer/torture/loop-inc-ptr-3.c: Likewise.

[Bug analyzer/94858] False report of memory leak

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94858

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks again for filing this.  Should be fixed by the above commit.