Bug 224984 – Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Darin Adler]

Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes

[Darin Adler]

Created attachment 426923 [details]
Patch

[Darin Adler]

*** Bug 209270 has been marked as a duplicate of this bug. ***

[Sam Weinig]

Comment on attachment 426923 [details]
Patch

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

> Source/WebKit/Platform/IPC/ArgumentCoders.h:-517
>          HashMapType hashMap;
> -        hashMap.reserveInitialCapacity(hashMapSize);

Should we consider doing the trick we do for Vector here where if the key and value are both fixed size we can check that the buffer is big enough to contain size * (sizeof(key type) + sizeof(value type)) and then reserve? I don't have a good feeling about what type of HashMaps/HashSets we use so I am not sure if that is a useful optimization.

[Darin Adler]

Comment on attachment 426923 [details]
Patch

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

>> Source/WebKit/Platform/IPC/ArgumentCoders.h:-517
>> -        hashMap.reserveInitialCapacity(hashMapSize);
> 
> Should we consider doing the trick we do for Vector here where if the key and value are both fixed size we can check that the buffer is big enough to contain size * (sizeof(key type) + sizeof(value type)) and then reserve? I don't have a good feeling about what type of HashMaps/HashSets we use so I am not sure if that is a useful optimization.

Not sure what you suggest can be done, but I’m sure we can do some kind of reality check about the remaining size of the decoding stream and reserve if it passes that check.

But I don’t see this in VectorArgumentCoder. Where is the code for that trick you are referring to?

[Darin Adler]

Oh, I see now. That first bool in the template!

[Darin Adler]

(In reply to Darin Adler from comment #5)
> Oh, I see now. That first bool in the template!

That’s a surprisingly narrow optimization.

[Darin Adler]

(In reply to Darin Adler from comment #6)
> (In reply to Darin Adler from comment #5)
> > Oh, I see now. That first bool in the template!
> 
> That’s a surprisingly narrow optimization.

It’s a more specific optimization for a smaller set of types. It’s "treat this as a single buffer and read it all at once using decodeFixedLengthData" when it’s a simple scalar.

It does seem like there are conditions where we can sufficiently vet the size and use it to preallocate space in the hash table. Using the relationship between the size of the remaining data and the size that we pre-allocate.

But, like you, Sam, I don’t know how important and valuable this optimization is. It saves memory (very important) on hash tables that we send across IPC (wait, what?, why?).

[Darin Adler]

Committed r276510 (236967@main): <https://commits.webkit.org/236967@main>

[Radar WebKit Bug Importer]

<rdar://problem/77110020>