digitalmars.D - Static analysis at Mozilla

bearophile (5/5) Jun 09 2010 C++ Static Analysis as done on the large Mozilla codebase:

Sean Kelly (14/17) Jun 10 2010 As much as I like static analysis, it still has a long way to go. For e...

Sean Kelly (2/3) Jun 10 2010 Oops, I mistyped that comment. I don't think Purify does static analysi...
=?UTF-8?B?QWxpIMOHZWhyZWxp?= (7/32) Jun 10 2010 For example, here's some C code that a static analysis tool recently

Sean Kelly (2/38) Jun 10 2010 I suppose so. Maybe I should change the if statement to a loop and see ...

Steven Schveighoffer (6/45) Jun 10 2010 What about if srclen is 0? Won't memcpy then be passed a null pointer v...

div0 (6/53) Jun 10 2010 memcpy may get passed a null, but it's never uninitialized.
Sean Kelly (2/50) Jun 10 2010 That's an artifact of my simplified example. The real code doesn't dere...

bearophile <bearophileHUGS lycos.com> writes:

C++ Static Analysis as done on the large Mozilla codebase:
http://blog.ezyang.com/2010/06/static-analysis-mozilla/
It shows that it's important to have a more powerful static reflection in D. It
works well with scoped user-defined attributes too.

Bye,
bearophile

Jun 09 2010

Sean Kelly <sean invisibleduck.org> writes:

bearophile Wrote:

 C++ Static Analysis as done on the large Mozilla codebase:
 http://blog.ezyang.com/2010/06/static-analysis-mozilla/
 It shows that it's important to have a more powerful static reflection in D.
It works well with scoped user-defined attributes too.

As much as I like static analysis, it still has a long way to go.  For example,
here's some C code that a static analysis tool recently flagged as broken:

    size_t fn( char** pdst, char* src, size_t srclen ) {
        __thread static char* dst      = NULL;
        __thread static size_t dstcap = 0;
        if( dstcap < srclen ) {
            dstcap = srclen;
            dst      = realloc( dst, dstcap );
        }
        memcpy( dst, src, srclen ); // Purify: ERROR - uninitialized write
        *pdst = dst;
        return srclen;
    }

Basically, it wasn't smart enough to realize that dst would always be non-NULL
when the memcpy occurred, let alone that it would also always be large enough. 
For such false positives, it's generally necessary to insert pointless code
simply to silence the error, thus complicating the function and increasing the
cost of maintenance.  I still believe that the benefits of static analysis
vastly outweigh the cost, but I'd love to see more intelligence in branch
analysis if nothing else.

Jun 10 2010

Sean Kelly <sean invisibleduck.org> writes:

Sean Kelly Wrote:
         memcpy( dst, src, srclen ); // Purify: ERROR - uninitialized write

Oops, I mistyped that comment.  I don't think Purify does static analysis :-)

Jun 10 2010

=?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:

Sean Kelly wrote:
 bearophile Wrote:

 C++ Static Analysis as done on the large Mozilla codebase:
 http://blog.ezyang.com/2010/06/static-analysis-mozilla/
 It shows that it's important to have a more powerful static 


reflection in D. It works well with scoped user-defined attributes too.
 As much as I like static analysis, it still has a long way to go. 

For example, here's some C code that a static analysis tool recently 
flagged as broken:
     size_t fn( char** pdst, char* src, size_t srclen ) {
         __thread static char* dst      = NULL;
         __thread static size_t dstcap = 0;
         if( dstcap < srclen ) {
             dstcap = srclen;
             dst      = realloc( dst, dstcap );
         }
         memcpy( dst, src, srclen ); // Purify: ERROR - uninitialized 

write
         *pdst = dst;
         return srclen;
     }

 Basically, it wasn't smart enough to realize that dst would
 always be non-NULL when the memcpy occurred, let alone that it
 would also always be large enough.  For such false positives,
 it's generally necessary to insert pointless code simply to
 silence the error, thus complicating the function and
 increasing the cost of maintenance.  I still believe that the
 benefits of static analysis vastly outweigh the cost, but I'd
 love to see more intelligence in branch analysis if nothing
 else.

realloc may return NULL. Perhaps they are catching that condition?

Ali

Jun 10 2010

Sean Kelly <sean invisibleduck.org> writes:

Ali Çehreli Wrote:

 Sean Kelly wrote:
  > bearophile Wrote:
  >
  >> C++ Static Analysis as done on the large Mozilla codebase:
  >> http://blog.ezyang.com/2010/06/static-analysis-mozilla/
  >> It shows that it's important to have a more powerful static 
 reflection in D. It works well with scoped user-defined attributes too.
  >
  > As much as I like static analysis, it still has a long way to go. 
 For example, here's some C code that a static analysis tool recently 
 flagged as broken:
  >
  >     size_t fn( char** pdst, char* src, size_t srclen ) {
  >         __thread static char* dst      = NULL;
  >         __thread static size_t dstcap = 0;
  >         if( dstcap < srclen ) {
  >             dstcap = srclen;
  >             dst      = realloc( dst, dstcap );
  >         }
  >         memcpy( dst, src, srclen ); // Purify: ERROR - uninitialized 
 write
  >         *pdst = dst;
  >         return srclen;
  >     }
  >
  > Basically, it wasn't smart enough to realize that dst would
  > always be non-NULL when the memcpy occurred, let alone that it
  > would also always be large enough.  For such false positives,
  > it's generally necessary to insert pointless code simply to
  > silence the error, thus complicating the function and
  > increasing the cost of maintenance.  I still believe that the
  > benefits of static analysis vastly outweigh the cost, but I'd
  > love to see more intelligence in branch analysis if nothing
  > else.
 
 realloc may return NULL. Perhaps they are catching that condition?

I suppose so.  Maybe I should change the if statement to a loop and see what
happens.

Jun 10 2010

"Steven Schveighoffer" <schveiguy yahoo.com> writes:

On Thu, 10 Jun 2010 15:55:18 -0400, Sean Kelly <sean invisibleduck.org>  
wrote:

 Ali Ã‡ehreli Wrote:

 Sean Kelly wrote:
  > bearophile Wrote:
  >
  >> C++ Static Analysis as done on the large Mozilla codebase:
  >> http://blog.ezyang.com/2010/06/static-analysis-mozilla/
  >> It shows that it's important to have a more powerful static
 reflection in D. It works well with scoped user-defined attributes too.
  >
  > As much as I like static analysis, it still has a long way to go.
 For example, here's some C code that a static analysis tool recently
 flagged as broken:
  >
  >     size_t fn( char** pdst, char* src, size_t srclen ) {
  >         __thread static char* dst      = NULL;
  >         __thread static size_t dstcap = 0;
  >         if( dstcap < srclen ) {
  >             dstcap = srclen;
  >             dst      = realloc( dst, dstcap );
  >         }
  >         memcpy( dst, src, srclen ); // Purify: ERROR - uninitialized
 write
  >         *pdst = dst;
  >         return srclen;
  >     }
  >
  > Basically, it wasn't smart enough to realize that dst would
  > always be non-NULL when the memcpy occurred, let alone that it
  > would also always be large enough.  For such false positives,
  > it's generally necessary to insert pointless code simply to
  > silence the error, thus complicating the function and
  > increasing the cost of maintenance.  I still believe that the
  > benefits of static analysis vastly outweigh the cost, but I'd
  > love to see more intelligence in branch analysis if nothing
  > else.

 realloc may return NULL. Perhaps they are catching that condition?

 I suppose so.  Maybe I should change the if statement to a loop and see  
 what happens.

What about if srclen is 0?  Won't memcpy then be passed a null pointer via  
dst?  Does the static analyzer look inside memcpy to see if it uses the  
pointer when the size is 0?

-Steve

Jun 10 2010

div0 <div0 users.sourceforge.net> writes:

On 10/06/2010 20:57, Steven Schveighoffer wrote:
 On Thu, 10 Jun 2010 15:55:18 -0400, Sean Kelly <sean invisibleduck.org>
 wrote:

 Ali Ã‡ehreli Wrote:

 Sean Kelly wrote:
 bearophile Wrote:

 C++ Static Analysis as done on the large Mozilla codebase:
 http://blog.ezyang.com/2010/06/static-analysis-mozilla/
 It shows that it's important to have a more powerful static


 reflection in D. It works well with scoped user-defined attributes too.
 As much as I like static analysis, it still has a long way to go.

 For example, here's some C code that a static analysis tool recently
 flagged as broken:
 size_t fn( char** pdst, char* src, size_t srclen ) {
 __thread static char* dst = NULL;
 __thread static size_t dstcap = 0;
 if( dstcap < srclen ) {
 dstcap = srclen;
 dst = realloc( dst, dstcap );
 }
 memcpy( dst, src, srclen ); // Purify: ERROR - uninitialized

 write
 *pdst = dst;
 return srclen;
 }

 Basically, it wasn't smart enough to realize that dst would
 always be non-NULL when the memcpy occurred, let alone that it
 would also always be large enough. For such false positives,
 it's generally necessary to insert pointless code simply to
 silence the error, thus complicating the function and
 increasing the cost of maintenance. I still believe that the
 benefits of static analysis vastly outweigh the cost, but I'd
 love to see more intelligence in branch analysis if nothing
 else.

 realloc may return NULL. Perhaps they are catching that condition?

 I suppose so. Maybe I should change the if statement to a loop and see
 what happens.

 What about if srclen is 0? Won't memcpy then be passed a null pointer
 via dst? Does the static analyzer look inside memcpy to see if it uses
 the pointer when the size is 0?

 -Steve

memcpy may get passed a null, but it's never uninitialized.
The analysis is wrong and so is the code. doh!

-- 
My enormous talent is exceeded only by my outrageous laziness.
http://www.ssTk.co.uk

Jun 10 2010

Sean Kelly <sean invisibleduck.org> writes:

Steven Schveighoffer Wrote:

 On Thu, 10 Jun 2010 15:55:18 -0400, Sean Kelly <sean invisibleduck.org>  
 wrote:
 
 Ali Ã‡ehreli Wrote:

 Sean Kelly wrote:
  > bearophile Wrote:
  >
  >> C++ Static Analysis as done on the large Mozilla codebase:
  >> http://blog.ezyang.com/2010/06/static-analysis-mozilla/
  >> It shows that it's important to have a more powerful static
 reflection in D. It works well with scoped user-defined attributes too.
  >
  > As much as I like static analysis, it still has a long way to go.
 For example, here's some C code that a static analysis tool recently
 flagged as broken:
  >
  >     size_t fn( char** pdst, char* src, size_t srclen ) {
  >         __thread static char* dst      = NULL;
  >         __thread static size_t dstcap = 0;
  >         if( dstcap < srclen ) {
  >             dstcap = srclen;
  >             dst      = realloc( dst, dstcap );
  >         }
  >         memcpy( dst, src, srclen ); // Purify: ERROR - uninitialized
 write
  >         *pdst = dst;
  >         return srclen;
  >     }
  >
  > Basically, it wasn't smart enough to realize that dst would
  > always be non-NULL when the memcpy occurred, let alone that it
  > would also always be large enough.  For such false positives,
  > it's generally necessary to insert pointless code simply to
  > silence the error, thus complicating the function and
  > increasing the cost of maintenance.  I still believe that the
  > benefits of static analysis vastly outweigh the cost, but I'd
  > love to see more intelligence in branch analysis if nothing
  > else.

 realloc may return NULL. Perhaps they are catching that condition?

 I suppose so.  Maybe I should change the if statement to a loop and see  
 what happens.

 
 What about if srclen is 0?  Won't memcpy then be passed a null pointer via  
 dst?  Does the static analyzer look inside memcpy to see if it uses the  
 pointer when the size is 0?

That's an artifact of my simplified example.  The real code doesn't dereference
src if srclen is 0.

Jun 10 2010

D Programming

C/C++ Programming

Other

digitalmars.D - Static analysis at Mozilla