Bug 224840 – Crash due to VectorBuffer pre-allocation failure

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ian Gilbert]

Crash due to VectorBuffer pre-allocation failure

[Ian Gilbert]

Created attachment 426610 [details]
Patch

[Ian Gilbert]

Created attachment 426614 [details]
Patch

[Ian Gilbert]

<rdar://problem/76698411>

[Sam Weinig]

Comment on attachment 426614 [details]
Patch

I don't quite know how to review the test, but the change looks good.

I think there might be a similar issue in:

ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode(...)

which does a `keyStatuses.reserveInitialCapacity(dataSize);` from a decoded size.

[Sam Weinig]

Looks like the generic HashMap and HashSet decoders also do a reservation in ArgumentCoders.h

[Ryosuke Niwa]

Comment on attachment 426614 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:363
> -    Vector<RefPtr<ApplePayError>> errors(size);
> +    Vector<RefPtr<ApplePayError>> errors;

Huh, I guess this fix is correct but we should get rid of this whole encode/decode pair and just add ones for ApplePayError instead
since we already have encoder/decoder for Vector<T>.

[EWS]

Committed r276341 (236819@main): <https://commits.webkit.org/236819@main>

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

[Darin Adler]

Comment on attachment 426614 [details]
Patch

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

>> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:363
>> +    Vector<RefPtr<ApplePayError>> errors;
> 
> Huh, I guess this fix is correct but we should get rid of this whole encode/decode pair and just add ones for ApplePayError instead
> since we already have encoder/decoder for Vector<T>.

Yes, we should do that.

[Darin Adler]

(In reply to Sam Weinig from comment #5)
> Looks like the generic HashMap and HashSet decoders also do a reservation in
> ArgumentCoders.h

Really!? That is not good. Let’s fix that. I hope there’s a way to do it without costing too much memory or performance.

[Darin Adler]

(In reply to Sam Weinig from comment #4)
> I think there might be a similar issue in:
> 
> ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode(...)
> 
> which does a `keyStatuses.reserveInitialCapacity(dataSize);` from a decoded
> size.

Yes, that must be removed.

[Sam Weinig]

(In reply to Darin Adler from comment #9)
> (In reply to Sam Weinig from comment #5)
> > Looks like the generic HashMap and HashSet decoders also do a reservation in
> > ArgumentCoders.h
> 
> Really!? That is not good. Let’s fix that. I hope there’s a way to do it
> without costing too much memory or performance.

I don't think the reservation is too likely to be that much of a win.

For avoid doing multiple allocations in the general case, we could pick a maximum value for reservations (maybe, 16 or something), and do the reservation if the size is at or below that.

For memory use we could add some sort of "shrinkToFix" for HashMap/HashSet, but we would probably be better off using the new RobinHoodHashMaps if we want to improve memory use.

[Sam Weinig]

(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 426614 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426614&action=review
> 
> > Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:363
> > -    Vector<RefPtr<ApplePayError>> errors(size);
> > +    Vector<RefPtr<ApplePayError>> errors;
> 
> Huh, I guess this fix is correct but we should get rid of this whole
> encode/decode pair and just add ones for ApplePayError instead
> since we already have encoder/decoder for Vector<T>.

Does the generic one work for T == RefPtr<Foo>?

[Darin Adler]

(In reply to Sam Weinig from comment #12)
> Does the generic one work for T == RefPtr<Foo>?

Almost certainly yes. I meant to try it but had other code in my tree.

[Darin Adler]

Did some follow-on work in bug 224984 and would like to continue to discuss there.