look at clang static analyzer output and flag any real bugs

NEW
Assigned to

Status

Tamarin
Virtual Machine
P3
minor
7 years ago
6 years ago

People

(Reporter: Tommy Reilly, Assigned: Tommy Reilly)

Tracking

unspecified
Future
x86
Mac OS X
Bug Flags:
flashplayer-injection -
flashplayer-qrb +
flashplayer-bug -
flashplayer-needsversioning -

Details

(Whiteboard: static-analysis, clang)

Attachments

(6 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
I find 46 errors and they look mostly harmless when I do the "scan-build xcodebuild" thing.   Mostly dead code issues but 16 could be real issues.
(Assignee)

Comment 1

7 years ago
Created attachment 539278 [details] [diff] [review]
necessary patch to make scan-build work
(Assignee)

Comment 2

7 years ago
Created attachment 539280 [details]
results
(Assignee)

Updated

7 years ago
Depends on: 667881
(Assignee)

Comment 3

7 years ago
results are from running (in platform/mac/avmshell directory)

$ scan-build xcodebuild 

scan-build is from http://clang-analyzer.llvm.org/ checker-257.tar.bz2
(Assignee)

Comment 4

7 years ago
Created attachment 542557 [details] [diff] [review]
Fix bits in page possible NULL analyzer warning
Assignee: nobody → treilly
Status: NEW → ASSIGNED
Attachment #542557 - Flags: review?(lhansen)
(Assignee)

Comment 5

7 years ago
Created attachment 542559 [details] [diff] [review]
Fix currentTotal dead init
Attachment #542559 - Flags: review?(lhansen)
(Assignee)

Comment 6

7 years ago
Created attachment 542560 [details] [diff] [review]
ifdef DEBUG GetUserPointer call
Attachment #542560 - Flags: review?(lhansen)
(Assignee)

Comment 7

7 years ago
Created attachment 542561 [details] [diff] [review]
Remove switch
Attachment #542561 - Flags: review?(lhansen)
(Assignee)

Comment 8

7 years ago
These patches fix all the MMgc issues found by the analyzer
(Assignee)

Comment 9

7 years ago
This almost works with xp build but it appears to be analyzing system headers and barfing profusely:

$ LDPLUSPLUS=~/llvm-2.9/checker-257/libexec/c++-analyzer CCC_CC=gcc-4.2 CCC_CXX=g++-4.2 CLANG=~/llvm-2.9/checker-257/bin/clang CLANG_CXX=~/llvm-2.9/checker-257/bin/clang++ CXX=~/llvm-2.9/checker-257/libexec/c++-analyzer CC=~/llvm-2.9/checker-257/libexec/ccc-analyzer make

Comment 10

7 years ago
Comment on attachment 542559 [details] [diff] [review]
Fix currentTotal dead init

If startingTotal isn't the proper init then 0 should be - missing inits are problems far more often than redundant inits.
Attachment #542559 - Flags: review?(lhansen) → review-

Comment 11

7 years ago
Comment on attachment 542557 [details] [diff] [review]
Fix bits in page possible NULL analyzer warning

OK i guess, though making m_bitsInPage const would be the better fix.
Attachment #542557 - Flags: review?(lhansen) → review+

Comment 12

7 years ago
Comment on attachment 542560 [details] [diff] [review]
ifdef DEBUG GetUserPointer call

The tag for the patch says DEBUG, the patch says MMGC_MEMORY_INFO.  Which is it?  Also, if it is DEBUG then the patch is pointless, GetUserPointer is a no-op in release builds so that we can avoid these kinds of #ifdefs.
Attachment #542560 - Flags: review?(lhansen) → review-

Comment 13

7 years ago
Comment on attachment 542561 [details] [diff] [review]
Remove switch

What's the justification here?  The code is clearly correct as is, and may even be faster in the common case (though nobody cares about that).
(Assignee)

Comment 14

7 years ago
(In reply to comment #13)
> Comment on attachment 542561 [details] [diff] [review] [review]
> Remove switch
> 
> What's the justification here?  The code is clearly correct as is, and may
> even be faster in the common case (though nobody cares about that).

The static analyzer isn't smart enough to realize that the switch value is in the set [0-4] so it thinks allocs could be NULL.
(Assignee)

Comment 15

7 years ago
(In reply to comment #12)
> The tag for the patch says DEBUG, the patch says MMGC_MEMORY_INFO.  Which is
> it?  Also, if it is DEBUG then the patch is pointless, GetUserPointer is a
> no-op in release builds so that we can avoid these kinds of #ifdefs.

Agreed, I'll see if I can come up with a solution that keeps us ifdef clean.
(Assignee)

Comment 16

7 years ago
Created attachment 542778 [details] [diff] [review]
Better version that addresses Lar's feedback
Attachment #542559 - Attachment is obsolete: true
Attachment #542778 - Flags: review?(lhansen)
(Assignee)

Comment 17

7 years ago
(In reply to comment #11)
> Comment on attachment 542557 [details] [diff] [review] [review]
> Fix bits in page possible NULL analyzer warning
> 
> OK i guess, though making m_bitsInPage const would be the better fix.

m_bitsInPage is const!   So this is another case where the static analyzer just isn't smart enough.
(Assignee)

Comment 18

7 years ago
Created attachment 542792 [details] [diff] [review]
Fix the bitsInPage issue with cleaner code
Attachment #542557 - Attachment is obsolete: true
Attachment #542792 - Flags: review?(lhansen)

Updated

7 years ago
Attachment #542778 - Flags: review?(lhansen) → review+

Comment 19

7 years ago
Comment on attachment 542792 [details] [diff] [review]
Fix the bitsInPage issue with cleaner code

This patch appears to be identical to the previous one.  Did you upload the wrong one?
Attachment #542792 - Flags: review?(lhansen) → review+

Comment 20

7 years ago
Comment on attachment 542561 [details] [diff] [review]
Remove switch

I will not, nor do I think that we in general should, rewrite the code just because the analyzer is too dumb to figure out what the code does, so long as it is clear to a human reader what is going on.
Attachment #542561 - Flags: review?(lhansen) → review-
(Assignee)

Comment 21

7 years ago
Created attachment 542795 [details] [diff] [review]
New bitsInPage patch, forgot to qref last time.
Attachment #542792 - Attachment is obsolete: true
Attachment #542795 - Flags: review?(lhansen)

Comment 22

7 years ago
changeset: 6436:97c56359f161
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 664257 - Clean up dead initialization found by clang analyzer (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/97c56359f161

Comment 23

7 years ago
Comment on attachment 542795 [details] [diff] [review]
New bitsInPage patch, forgot to qref last time.

I'm not sure about this one.  I seem to remember there was a good reason (probably related to OOM) why the bits alloc came before the block alloc.  I have to think about it.  Maybe it is that if the bits alloc fails and we shut down due to OOM the allocated block will be orphaned if it is not owned by the collector; if it is owned by the collector it may be reaped because it is empty.

Comment 24

7 years ago
(In reply to comment #23)
> ... if it is owned by the collector it may be reaped because
> it is empty.

And then, if we /don't/ shut down we have an inconsistent heap.
(Assignee)

Comment 25

7 years ago
(In reply to comment #24)
> (In reply to comment #23)
> > ... if it is owned by the collector it may be reaped because
> > it is empty.
> 
> And then, if we /don't/ shut down we have an inconsistent heap.

So right now the bits are still allocated before the block is linked in so in the case of bits oom then the block is just orphaned (which is fine because the shutdown sequence will return the memory to the OS anyways).
(Assignee)

Comment 26

7 years ago
(In reply to comment #20)
> I will not, nor do I think that we in general should, rewrite the code just
> because the analyzer is too dumb to figure out what the code does, so long
> as it is clear to a human reader what is going on.

Yeah in this case I think readability is a wash so I don't feel strongly about it, its not like the patch makes it harder or easier to read.   

Tangential q: why isn't the bibopAllocFloat allocator in this list?  It appears to have a quick list from a cursory reading.

Comment 27

7 years ago
> Tangential q: why isn't the bibopAllocFloat allocator in this list?  It
> appears to have a quick list from a cursory reading.

That's probably an oversight.  I could wave my hands and say something about how that list will usually be short and there's no explicit or RC freeing onto it so it'll stay short blabla but it wouldn't be completely convincing.

I'll file a bug.  Thanks.

Comment 28

7 years ago
Comment on attachment 542795 [details] [diff] [review]
New bitsInPage patch, forgot to qref last time.

Trying to work through what would happen here.

We allocate a block for the allocator and that succeeds.  GC::AllocBlock marks the block as kGCAllocPage.  But the block header is still uninitialized.

Then we initialize the first few fields, and we get to the point where we need to initialize b->bits.  Suppose the free list is empty so we call heapAlloc().  Suppose GCHeap::AllocHelper fails, so we call SendFreeMemorySignal.  This results in GC::memoryStatusChange being invoked, and we're OnThread, so we Collect().  The collector performs a conservative stack scan and scans some objects conservatively; assume the stack or one such object contains a bit pattern that looks like a pointer into the freshly allocated block.  This would happen in GC::TraceConservativePointer.  Remember, that page is marked kGCAllocPage, so that's the first big block in TraceConservativePointer.  We get to this:

    itemNum = GCAlloc::GetObjectIndex(block, item);

which will fail because GetObjectIndex will require block->items to be initialized but it was not initialized yet in GCAlloc.  That's fixable, so assume we fix it.  We then get to the main test:

    gcbits_t& bits2 = block->bits[GCAlloc::GetBitsIndex(block, item)];

block->bits is NULL and this is not fixable in GCAlloc because we're in the process of allocating it.

Am I missing something?  ISTR that the reason there is a comment in GCAlloc, "Get bitmap space; this may trigger OOM handling.", is precisely because of this possibility.

We could fix this by not marking the page as a GC page in GC::AllocBlock until we've allocated the bits space (and that would arguably be cleanest in any case), but that's somewhat more radical surgery, and all for the purpose of pacifying Clang.

Tentative R- then.
Attachment #542795 - Flags: review?(lhansen) → review-

Updated

7 years ago
Flags: flashplayer-qrb+
Flags: flashplayer-needsversioning-
Flags: flashplayer-injection-
Flags: flashplayer-bug-
Priority: -- → P3
Target Milestone: --- → Q1 12 - Brannan
(Assignee)

Updated

7 years ago
Target Milestone: Q1 12 - Brannan → Future
(I cannot trivially tell how much of this task remains uncompleted.  We could probably safely close this ticket and reopen it later if someone else decided we should give clang another whirl.)
Status: ASSIGNED → NEW
Whiteboard: static-analysis, clang
You need to log in before you can comment on or make changes to this bug.