Bug 215737 – IPC::encodeSharedBuffer combines SharedBuffer data segments when copying to SharedMemory

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

WebKit::encodeSharedBuffer(E) needlessly combines SharedBuffer data segments when creating a SharedMemory

[Andy Estes]

WebKit::encodeSharedBuffer() needlessly combines SharedBuffer data segments when creating a SharedMemory

[Radar WebKit Bug Importer]

<rdar://problem/67568099>

[Andy Estes]

Created attachment 407020 [details]
Patch

[Andy Estes]

<rdar://problem/67568099>

[Darin Adler]

Comment on attachment 407020 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        * Platform/SharedMemory.cpp:
> +        (WebKit::SharedMemory::copyBuffer): Changed iterate data segments using a for loop rather
> +        than repeated calls to SharedBuffer::getSomeData().

So this is just a coding style improvement, not the fix?

> Source/WebKit/ChangeLog:14
> +        * Shared/WebCoreArgumentCoders.cpp:
> +        (IPC::encodeSharedBuffer): Changed to call SharedMemory::copyBuffer() rather than
> +        SharedMemory::allocate() + memcpy.

And this is the fix?

> Source/WebKit/Platform/SharedMemory.cpp:44
>      char* const sharedMemoryPtr = reinterpret_cast<char*>(sharedMemory->data());

This should be static_cast, not reinterpret_cast.

I also suggest using auto rather than char* const.

> Source/WebKit/Platform/SharedMemory.cpp:46
> +        ASSERT(segmentEntry.beginPosition + segmentEntry.segment->size() <= sharedMemory->size());

While it’s OK to assert this, seems unnecessary.

> Source/WebKit/Platform/SharedMemory.cpp:48
> +        auto result = memcpy(sharedMemoryPtr + segmentEntry.beginPosition, segmentEntry.segment->data(), segmentEntry.segment->size());
> +        ASSERT_UNUSED(result, result == sharedMemoryPtr + segmentEntry.beginPosition);

This seems strange in both the old and new code. Why look at the return value from memcpy? Just not what we normally need to do.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:144
> +    auto sharedMemoryBuffer = SharedMemory::copyBuffer(*buffer);
>      sharedMemoryBuffer->createHandle(handle, SharedMemory::Protection::ReadOnly);

Not sure about the failure handling here. For some reason copyBuffer returns null when it fails, rather than doing an explicit crash. Seems like we should be doing an explicit crash rather than just letting it crash "naturally" by dereferencing null.

But this is no different from when we were calling allocate directly. As a cleanup I suggest we tighten this up later and crash rather than returning null. If we really need the "return null" behavior I suggest we add a tryAllocate and a tryCopyBuffer.

[Andy Estes]

Created attachment 407025 [details]
Patch for landing

[Andy Estes]

(In reply to Darin Adler from comment #5)
> Comment on attachment 407020 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407020&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        * Platform/SharedMemory.cpp:
> > +        (WebKit::SharedMemory::copyBuffer): Changed iterate data segments using a for loop rather
> > +        than repeated calls to SharedBuffer::getSomeData().
> 
> So this is just a coding style improvement, not the fix?
> 
> > Source/WebKit/ChangeLog:14
> > +        * Shared/WebCoreArgumentCoders.cpp:
> > +        (IPC::encodeSharedBuffer): Changed to call SharedMemory::copyBuffer() rather than
> > +        SharedMemory::allocate() + memcpy.
> 
> And this is the fix?

Yeah. I've updated the ChangeLog with better words.

[EWS]

Committed r266014: <https://trac.webkit.org/changeset/266014>

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