657530 - Memory leak in Array::sort() if getUintProperty (etc) throw exceptions

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Addition of testcases for Bug 654807 have revealed a latent memory leak in Array.sort: if getUintProperty() calls on the array throw exceptions, ArraySort's dtor is never run, which leaks the "index" array (allocated via mmfx). A minor leak in a weird situation, but generates asserts in debug runs.

Comment 1

[Steven Johnson]

(Side note: ArraySort is a horrific wart; everything runs in the ctor, and the result is via an out-arg -- why it isn't just a normal static function is beyond me, but rewriting it is definitely scope creep...)

Comment 2

[Steven Johnson]

Attachment: Patch

A bit of a quick-n-dirty fix: use VMPI_alloca instead of mmfx. (Alternately, we could use GCAlloc, but we want to get away from that... or we could use DataList, but that would necessitate other changes to this code... or we could wrap the ArraySort in TRY/CATCH, but this is an extremely degenerate failure case so it seems like a waste)

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 532810 [details] [diff] [review]
Patch

Feedback:

         // new class[n] compiles into code which tries to allocate n * sizeof(class).
         // unfortunately, the generated assembly does not protect against this product overflowing int.
         // So I limit the length -- for larger values, I expect new will fail anyways.
         if ((len > 0) && (len < (0x10000000)))

The comment text would no longer apply with your change to use CheckForCallocSizeOverflow now, right? 

On the other hand, I don't think the inputs to CheckForCallocSizeOverflow here will ever actually overflow given this if-guard.

I'm guessing we are stuck with the if-guard as it is.

But the disconnect between what the comment says and what lies beneath makes the code confusing; you should change the comment so that its clear that the if-guard is now a historical artifact.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #3)
> Comment on attachment 532810 [details] [diff] [review] [review]
> Patch
> 
> Feedback:
> 
>          // new class[n] compiles into code which tries to allocate n *
> sizeof(class).
>          // unfortunately, the generated assembly does not protect against
> this product overflowing int.
>          // So I limit the length -- for larger values, I expect new will
> fail anyways.
>          if ((len > 0) && (len < (0x10000000)))
> 
> The comment text would no longer apply with your change to use
> CheckForCallocSizeOverflow now, right? 

Actually that comment is not talking about the index array allocation.  Your change is probably leaving the net amount of confusion in the code unchanged.  So changing the comment should not block pushing this.

Comment 5

[Steven Johnson]

Comment on attachment 532810 [details] [diff] [review]
Patch

switching to Edwin as Lars is OOO today

Comment 6

[Steven Johnson]

Yeah, the comment is definitely a monkey-hack from some primordial committer; that said, we're probably safer to stick with it. Will update the comment though.

Comment 7

[Lars T Hansen]

IMO the new comment should not be inserted into ArraySort::~ArraySort, it does not contribute anything except confusion, unless you're prepared to make it a requirement that anyplace we use VMPI_alloc we have to add similar comments.  Instead, the appropriate place to add a comment is to the definition of ArraySort, where it is already required that ArraySort be stack-allocated because it uses VMPI_alloca for temporary data.

Otherwise no objections from me (but R? is not to me any longer :-)

Comment 8

[Steven Johnson]

Comment on attachment 532810 [details] [diff] [review]
Patch

OK then, back to Lars, since Edwin is dragging his heels...

Comment 9

[Rick Reitmaier]

Comment on attachment 532810 [details] [diff] [review]
Patch

steal review and r+

Comment 10

[Steven Johnson]

TR 6315:e5b85a0b655e