Bug 230245 – [bmalloc] freeableMemory and footprint of Heap are completely broken

• Component: bmalloc
• Status: RESOLVED FIXED

[Basuke Suzuki]

They became huge numbers which means the overflow happens at some point.

[Basuke Suzuki]

git bisect says the break happens on this commit:
https://github.com/WebKit/WebKit/commit/8e1ec1def4ca1b38f2fad2679d120adc9531ce08#diff-34efcb839f5ce590e8868d478f3801379e2c680f863116a740b63b99598691d6

[Basuke Suzuki]

(In reply to Basuke Suzuki from comment #1)
> git bisect says the break happens on this commit:
> https://github.com/WebKit/WebKit/commit/
> 8e1ec1def4ca1b38f2fad2679d120adc9531ce08#diff-
> 34efcb839f5ce590e8868d478f3801379e2c680f863116a740b63b99598691d6

But this change is correct because newly allocated range by VM has entire physical page. So the bug exists before this commit and this commit made that bug visible.

[Basuke Suzuki]

Created attachment 438160 [details]
PATCH

[Basuke Suzuki]

Comment on attachment 438160 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=438160&action=review

> Source/bmalloc/bmalloc/Heap.cpp:585
> +        adjustFootprint(lock, range.totalPhysicalSize(), "allocateLarge");

These two lines are the fix.

[Basuke Suzuki]

Comment on attachment 438160 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=438160&action=review

> Source/bmalloc/bmalloc/Heap.cpp:112
> +    constexpr bool verbose = false;

This constexpr controls the above logic and turning this false to delete the added complexity at all. Proven in compiler explorer: https://godbolt.org/z/eoK54xzq9

[Geoffrey Garen]

Comment on attachment 438160 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=438160&action=review

> Source/bmalloc/bmalloc/Heap.cpp:100
> +    BUNUSED(label);
> +    BUNUSED(note);

label and note are used, so these statements appear to be unnecessary / incorrect.

> Source/bmalloc/bmalloc/Heap.cpp:103
> +    BASSERT(label || (amount >= 0 && result >= value) || (amount < 0 && result < value));

Why does having a label opt you out of this assertion?

Side note: Are you trying to do checked arithmetic here? If so, I'd prefer using something from CheckedArithmetic.h.

>> Source/bmalloc/bmalloc/Heap.cpp:112
>> +    constexpr bool verbose = false;
> 
> This constexpr controls the above logic and turning this false to delete the added complexity at all. Proven in compiler explorer: https://godbolt.org/z/eoK54xzq9

Does putting verbose inside Heap::adjustStat() not achieve the same goal, even though it is inlined?

> Source/bmalloc/bmalloc/Heap.cpp:113
> +    BUNUSED(note);

Do we need this? 'note' appears in this function.

> Source/bmalloc/bmalloc/Heap.cpp:126
> +    BUNUSED(note);

Ditto

[Basuke Suzuki]

Comment on attachment 438160 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=438160&action=review

>> Source/bmalloc/bmalloc/Heap.cpp:100
>> +    BUNUSED(note);
> 
> label and note are used, so these statements appear to be unnecessary / incorrect.

Right. This was leftover from pre-cleanup code.

>> Source/bmalloc/bmalloc/Heap.cpp:103
>> +    BASSERT(label || (amount >= 0 && result >= value) || (amount < 0 && result < value));
> 
> Why does having a label opt you out of this assertion?
> 
> Side note: Are you trying to do checked arithmetic here? If so, I'd prefer using something from CheckedArithmetic.h.

For debugging purpose. I wanted to see the weird overflow using verbose output. On that case this assertion was not required. But that can be controlled by release/debug. I can remove this.

>>> Source/bmalloc/bmalloc/Heap.cpp:112
>>> +    constexpr bool verbose = false;
>> 
>> This constexpr controls the above logic and turning this false to delete the added complexity at all. Proven in compiler explorer: https://godbolt.org/z/eoK54xzq9
> 
> Does putting verbose inside Heap::adjustStat() not achieve the same goal, even though it is inlined?

This can control activating only footprint verbose output, for instance. If the result after optimization differ for moving this into adjustStat() then I will consider for moving, but they are practically same. Or I can extract the logging part from adjustStat and call separately for clarification.

>> Source/bmalloc/bmalloc/Heap.cpp:113
>> +    BUNUSED(note);
> 
> Do we need this? 'note' appears in this function.

Leftover. will remove.

>> Source/bmalloc/bmalloc/Heap.cpp:126
>> +    BUNUSED(note);
> 
> Ditto

Ditto. Thanks

[Basuke Suzuki]

Created attachment 438316 [details]
patch again.

Porting CheckedArithmetic from WTF is too big to include this patch. That should be separated bug to be achieved.

[Radar WebKit Bug Importer]

<rdar://problem/83339024>

[Geoffrey Garen]

> Porting CheckedArithmetic from WTF is too big to include this patch. That
> should be separated bug to be achieved.

Fair point. I had forgotten that CheckedArithmetic was in a separate library.

[Geoffrey Garen]

Comment on attachment 438316 [details]
patch again.

View in context: https://bugs.webkit.org/attachment.cgi?id=438316&action=review

r=me

> Source/bmalloc/ChangeLog:3
> +        [bmalloc] freeableMemory and footprint of Heap are broken

Can you make this title more precise? Are freeableMemory and footprint literally completely broken in all cases all the time, or only in certain cases, or only broken in a particular way?

Can you say anything about how observable behavior changes after this change is made?

[Basuke Suzuki]

Created attachment 438876 [details]
PATCH

Added example of output to display error information

[Basuke Suzuki]

> r=me

Thank you very much.


> > Source/bmalloc/ChangeLog:3
> > +        [bmalloc] freeableMemory and footprint of Heap are broken
> 
> Can you make this title more precise? Are freeableMemory and footprint
> literally completely broken in all cases all the time, or only in certain
> cases, or only broken in a particular way?
> 
> Can you say anything about how observable behavior changes after this change
> is made?


I've added those description in ChangeLog,

[EWS]

Committed r282850 (241981@main): <https://commits.webkit.org/241981@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438876 [details].

[Alexey Proskuryakov]

We started seeing many unreproducible crashes in bmalloc::Heap::decommitLargeRange on the bots recently, such as below. May need to revert this change to see if that fixes those.

https://bug-230719-attachments.webkit.org/attachment.cgi?id=439093
https://bug-230866-attachments.webkit.org/attachment.cgi?id=439406

[Eric Hutchison]

Reverted r282850 for reason:

Patch causing many crashes in bmalloc::Heap::decommitLargeRange

Committed r283264 (242293@main): <https://commits.webkit.org/242293@main>

[Basuke Suzuki]

(In reply to Alexey Proskuryakov from comment #15)
> We started seeing many unreproducible crashes in
> bmalloc::Heap::decommitLargeRange on the bots recently, such as below. May
> need to revert this change to see if that fixes those.
> 
> https://bug-230719-attachments.webkit.org/attachment.cgi?id=439093
> https://bug-230866-attachments.webkit.org/attachment.cgi?id=439406

Sure. Please do it. But I don't have idea why this causes the crash.

[Basuke Suzuki]

Wait a minute, are those debug build? If so, then that might be because I've added assertions there.

[Alexey Proskuryakov]

Yes, these look like assertions (at least two separate ones, judging from crash logs).

[Basuke Suzuki]

(In reply to Alexey Proskuryakov from comment #19)
> Yes, these look like assertions (at least two separate ones, judging from
> crash logs).

Give me an advice. Here's the background.
1. The numbers are broken before this patch landed
2. Those numbers are debugging information which is not used at runtime (as far as I know)
3. I fixed the one I've found and it seems the number is now healthy.
4. I've added assertion to detect future regression
5. Actually there's still case where the numbers are incorrect.
6. I basically need this fixed number to see my other patches effect.

I think to remove assertion is the best for now. What do you think?

[Basuke Suzuki]

Created attachment 439770 [details]
PATCH

Make assertion optional.

[Basuke Suzuki]

Comment on attachment 439770 [details]
PATCH

The optional assertion in adjustStat is required to find the hidden issue.

[Geoffrey Garen]

Comment on attachment 439770 [details]
PATCH

r=me

What's our plan to track down the remaining bugs and re-enable the assertion?

[EWS]

Committed r286029 (244417@main): <https://commits.webkit.org/244417@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439770 [details].

[Simon Fraser (smfr)]

Comment on attachment 439770 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=439770&action=review

> Source/bmalloc/ChangeLog:9
> +        This introduced in r279922. The physical size of the newly allocated range was changed from zero

Can we make API tests that would detect if we break this again in future?