1477073 - AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned: 0x60d00002a988 [@ malloc_usable_size]

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 63.0a1-20180719100142-https://hg.mozilla.org/mozilla-central/rev/183ee39bf309cd8463d8db5b5c8eb232cd0dac53.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Brian, thanks for your report! Do you remember any specific steps that caused this crash? It looks related to wasm and collecting memory reports (maybe automatic or from about:memory).

Comment 3

[Brian Carpenter [:geeknik]]

The browser never crashed, but I've been flipping knobs and twisting dials every since I read the blog post about this exciting project! I was indeed playing with about:memory earlier, but other then a brief hang while saving verbose gc & cc logs, I didn't notice any unusual behavior.

Comment 4

[Andrew McCreight [:mccr8]]

I can't tell if it is the same thing, but if I'm running the WebAssembly demo at https://webassembly.org/demo/ and I get a memory report in about:memory it seems to always crash.

Comment 5

[Andrew McCreight [:mccr8]]

Decoder told me how to find the ASan reports in my local profile directory (under an ASan subdirectory) and I can confirm that this looks the same as the original report.

Comment 6

[Mike Hommey [:glandium]]

Seems like a good candidate to run under rr.

Comment 7

[Brian Carpenter [:geeknik]]

Can confirm the https://webassembly.org/demo tab crashes when I go into about:memory and hit the measure button.

Comment 8

[Brian Carpenter [:geeknik]]

The 3 additional ASan logs that showed up in my profile directory as a result of testing that match the one attached to this report.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Wow, nice catch. Pretty sure it's benign: we're just double counting the memory of an attribute, which pointer is within the bounds of an another object malloc'd before for which we've counted mallocSizeOf(this). I think that's what ASAN is complaining about here.

Not a security issue, I think: this API is only used for about:memory reporting, as far as I can tell (I got through the call stack as reported by Searchfox up to JS::CollectRuntimeStats [1]).

Will make a patch.

[1] https://searchfox.org/mozilla-central/source/js/src/vm/MemoryMetrics.cpp#882

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

(Assigning review once I've confirmed this is the right fix)

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Hi Luke, welcome back!

(See previous comment for explanation of the issue)

Another count issue (revealed by a local browser ASAN build) happens when a CompileTask's sizeof(this) is added in HelperThread sizeOfExcludingThis. We can just make them use sizeOfExcludingThis.

This makes me think that our memory reporting might be incorrect in other places related to wasm, and it would not be caught by ASAN:
- either when we call sizeOfExcludingThis on an object several times during the same report collection,
- or when we forget to call it (for instance that might happen for some structures belonging to the ModuleGenerator).

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> This makes me think that our memory reporting might be incorrect in other
> places related to wasm, and it would not be caught by ASAN:
> - either when we call sizeOfExcludingThis on an object several times during
> the same report collection,
> - or when we forget to call it (for instance that might happen for some
> structures belonging to the ModuleGenerator).

You can check for both of these things using a DMD build.

Comment 13

[Eric Rahm [:erahm]]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> > This makes me think that our memory reporting might be incorrect in other
> > places related to wasm, and it would not be caught by ASAN:
> > - either when we call sizeOfExcludingThis on an object several times during
> > the same report collection,
> > - or when we forget to call it (for instance that might happen for some
> > structures belonging to the ModuleGenerator).
> 
> You can check for both of these things using a DMD build.

More specifically add:
  1) 'ac_add_options --enable-dmd' to your .mozconfig
  2) |./mach run --dmd https://webassembly.org/demo/|
  3) load about:memory, click 'Save' under 'Save DMD output'

Benjamin, in addition to your patch can you also add a test? We run tests under ASAN which should be enough to catch this type of issue in the future. The WebAudio reporter [1] might be a good example.

[1] https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/dom/media/webaudio/test/test_WebAudioMemoryReporting.html

Comment 14

[Christian Holler (:decoder)]

Opening this up based on Benjamin's comments.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 8993743 [details] [diff] [review]
fix.patch

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

Thanks!

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Eric Rahm [:erahm] from comment #13)
> Benjamin, in addition to your patch can you also add a test?

Opened bug 1477969 for this, to not block landing of this patch.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1be8ad5a7f3f
Don't double-count wasm structures when creating a memory report; r=luke

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1be8ad5a7f3f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Sounds like this can ride the trains to me, but feel free to nominate for Beta approval if you feel strongly otherwise.

Comment 20

[Intermittent Failures Robot]

1 failures in 2917 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:
* autoland: 1

Platform breakdown:
* windows10-64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1477073&startday=2018-11-12&endday=2018-11-18&tree=all