Closed Bug 990807 Opened 6 years ago Closed 5 years ago

Valgrind detects leak - 4 bytes and/or 32 bytes are definitely lost (direct)

Categories

(Core :: JavaScript Engine: JIT, defect, major)

ARM
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: gkw, Assigned: mjrosenb)

References

(Blocks 1 open bug)

Details

(Keywords: testcase, valgrind, Whiteboard: [fuzzblocker])

Attachments

(3 files, 1 obsolete file)

Attached file stack
(function() {
    function f() {}
    [, -0, 0 / 0, , , , , 0x80000001, -0xfff, , 0x80000001, , -0xfff, -0, 0x100000001];
    function c() {
        f
    }
})()

valgrind --smc-check=all-non-file --leak-check=full --show-leak-kinds=definite ./js-opt-32-dm-vg-ts-hfp-linux-c16f36021958 --ion-parallel-compile=off --ion-eager testcase.js

==29967== 4 bytes in 1 blocks are definitely lost in loss record 1 of 7
==29967==    at 0x48253C0: operator new[](unsigned int) (vg_replace_malloc.c:379)
==29967==    by 0x8334B: js::jit::AssemblerBufferWithConstantPool<1024, 4, js::jit::Instruction, js::jit::Assembler, 1>::finishPool() (IonAssemblerBufferWithConstantPools.h:722)
==29967==    by 0x830C1: js::jit::AssemblerBufferWithConstantPool<1024, 4, js::jit::Instruction, js::jit::Assembler, 1>::dumpPool() (IonAssemblerBufferWithConstantPools.h:939)
/snip

==29967== 32 bytes in 2 blocks are definitely lost in loss record 4 of 7
==29967==    at 0x48253C0: operator new[](unsigned int) (vg_replace_malloc.c:379)
==29967==    by 0x8332D: js::jit::AssemblerBufferWithConstantPool<1024, 4, js::jit::Instruction, js::jit::Assembler, 1>::finishPool() (IonAssemblerBufferWithConstantPools.h:721)
==29967==    by 0x830C1: js::jit::AssemblerBufferWithConstantPool<1024, 4, js::jit::Instruction, js::jit::Assembler, 1>::dumpPool() (IonAssemblerBufferWithConstantPools.h:939)
/snip

Tested on ARM Ubuntu Linux on m-c rev c16f36021958. Marty, any idea how to move this forward?

My configure flags are:

AR=ar sh /home/fuzz5lin/trees/mozilla-central/js/src/configure --enable-optimize=-O1 --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-valgrind --with-ccache --enable-threadsafe <other NSPR flags>
Flags: needinfo?(mrosenberg)
This blocks fuzzing using Valgrind on ARM. (which has become fairly usable recently on a pandaboard, hence this bug)
Whiteboard: [fuzzblocker]
The pool packing failure bail out path does not appear to free resources allocated in finishPool().  I can not reproduce this path with the test so it might not be this.  There are some other OOM paths that do not free resources in finishPool.
Comment on attachment 8400385 [details] [diff] [review]
Free resources in the bail out path.

I've to double check myself, but it does seem that this fixes the issue, perhaps time for a review? :)
Attachment #8400385 - Flags: feedback+
Flags: needinfo?(mrosenberg) → needinfo?(dtc-moz)
Comment on attachment 8400385 [details] [diff] [review]
Free resources in the bail out path.

Looks like this path was being hit, although I could not reproduce it.  The larger issue will be addressed in bug 972710.  There are also some OOM paths that do not free these resources, and should they also be addressed?
Attachment #8400385 - Flags: review?(mrosenberg)
Flags: needinfo?(dtc-moz)
Comment on attachment 8400385 [details] [diff] [review]
Free resources in the bail out path.

Review of attachment 8400385 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about the delay on this.  I looked into this before gkw filed this bug.  I think there are some cases that your patch misses.  If for whatever reason, it turns out the extra paths are dead, feel free to re-r? me, and It'll be a quick review :-)
Attachment #8400385 - Flags: review?(mrosenberg)
Attached patch fix_oom_leak-r0.patch (obsolete) — Splinter Review
This should handle a couple of cases that doug's patch doesn't handle.
Attachment #8402704 - Flags: review?(dtc-moz)
Attachment #8402704 - Flags: feedback?(gary)
Comment on attachment 8402704 [details] [diff] [review]
fix_oom_leak-r0.patch

Review of attachment 8402704 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch.  Looks good, but a few questions below.  Please resubmit for review.

::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h
@@ +760,5 @@
>                      IonSpew(IonSpew_Pools, "[%d]***Offset was still out of range!***", id, codeOffset - magicAlign);
>                      IonSpew(IonSpew_Pools, "[%d] Too complicated; bailingp", id);
>                      this->fail_bail();
> +                    // only free up to the current offset
> +                    for (int pi = 0; pi <= poolIdx; pi++)

Perhaps this should be 'for (int pi = poolIdx; pi < numPoolKinds; pi++)' as the outer loop allocates from numPoolKinds-1 down?

@@ +786,5 @@
>                  p->numEntries -= numSkips;
>              }
>              poolOffset += p->numEntries * p->immSize;
>              delete[] preservedEntries;
> +            preservedEntries = nullptr;

nit: is this necessary?  Were you going to handle the allocation of preservedEntries failing.
Attachment #8402704 - Flags: review?(dtc-moz)
Comment on attachment 8402704 [details] [diff] [review]
fix_oom_leak-r0.patch

Seems like this patch might go through some more iterations, please request feedback when r+ and ready.
Attachment #8402704 - Flags: feedback?(gary)
(In reply to Douglas Crosher [:dougc] from comment #7)
> Perhaps this should be 'for (int pi = poolIdx; pi < numPoolKinds; pi++)' as
> the outer loop allocates from numPoolKinds-1 down?
Good Catch!  That hopefully would have thrown assertions all over the underlying code, but you never know...
> 
> @@ +786,5 @@
> >                  p->numEntries -= numSkips;
> >              }
> >              poolOffset += p->numEntries * p->immSize;
> >              delete[] preservedEntries;
> > +            preservedEntries = nullptr;
> 
> nit: is this necessary?  Were you going to handle the allocation of
> preservedEntries failing.
I'm doing that because if we may delete it again later on another code path, so this prevents us from deleting it twice, which afaict, is a bug, and will likely make free complain.
Feel free to set feedback on it if/when you're done reviewing it.
Attachment #8402704 - Attachment is obsolete: true
Attachment #8402899 - Flags: review?(dtc-moz)
Comment on attachment 8402899 [details] [diff] [review]
fix_oom_leak-r1.patch

Review of attachment 8402899 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h
@@ +786,5 @@
>                  p->numEntries -= numSkips;
>              }
>              poolOffset += p->numEntries * p->immSize;
>              delete[] preservedEntries;
> +            preservedEntries = nullptr;

Thank you for the explanation.  I must be missing something as it appears to just loop back to the loop head where preservedEntries is re-assigned so setting it to null appears unnecessary.
Attachment #8402899 - Flags: review?(dtc-moz) → review+
Attachment #8402899 - Flags: feedback?(gary)
Comment on attachment 8402899 [details] [diff] [review]
fix_oom_leak-r1.patch

Nice, this seems to work fine. Thanks! :)
Attachment #8402899 - Flags: feedback?(gary) → feedback+
I guess this can be landed?
Flags: needinfo?(mrosenberg)
Flags: needinfo?(dtc-moz)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #13)
> I guess this can be landed?

Look ok to me.
Flags: needinfo?(dtc-moz)
Helped to land this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc71ac7b9074
Flags: needinfo?(mrosenberg)
Target Milestone: --- → mozilla31
https://hg.mozilla.org/mozilla-central/rev/bc71ac7b9074
Assignee: nobody → mrosenberg
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.