Bug 230776 – IPC::Encoder::grow and IPC::Decoder::alignBufferPosition are incompatible when alignment != 8

• Component: WebKit2
• Status: NEW

[Wenson Hsieh]

IPC::Encoder rounds the buffer size to the given alignment, whereas IPC::Decoder rounds the buffer offset (a pointer) to the given alignment.

For instance, if the alignment is 16 bytes, then it is possible for IPC::Encoder to add more padding to adjust for alignment, which IPC::Decoder may not add if the buffer pointer offset (`m_bufferPos`) just so happens to land on a 16-byte-aligned address.

[Wenson Hsieh]

Note: this fix would allow us to encode/decode WebCore::TransformationMatrix using SimpleArgumentCoder.

The 16-byte alignment of TransformationMatrix currently prevents us from doing so — see https://bugs.webkit.org/show_bug.cgi?id=230714 for additional information.

[Wenson Hsieh]

Created attachment 439215 [details]
For EWS

[Wenson Hsieh]

Created attachment 439241 [details]
With ChangeLog

[Wenson Hsieh]

Comment on attachment 439241 [details]
With ChangeLog

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

> Source/WebKit/Platform/IPC/Encoder.cpp:189
> +    size_t alignedSize = roundUpToMultipleOf(alignment, m_bufferSize);

I just realized that if we go this route, StreamConnectionEncoder is going to also need this adjustment.

[Kimmo Kinnunen]

Good catch.
I remember something about the discrepancy, but IIRC I didn't dare to touch it.
I can understand if it does fail, I just didn't understand how.

This is how I interpreted the original design:
- We want to have memcpy's to aligned addresses (encoding) and from aligned addresses (decoding) due to getting memcpy fast path

- We want to have decoded data in aligned addresses for the option of in-place interpretation of the data

- Hence the buffer pointer needs to have the same address alignment in both sides, 
encoder and decoder, with the alignment defined as the maximum value used, which is std::max_align_t.

In normal IPC case, the buffer is mapped via mmap in allocation side and via the mach message machinery in the receive side, and I'm assuming both have the contract that it's mapped page aligned.

So for the example, if we have page alignment 4096 and max alignment 128.
So base addresses:
  e = map()
  d = receive()
  e % 4096 == d % 4096 (== 0)
  e_ptr = e
  d_ptr = d
  e_sz = 0
  d_sz = 0

My intuition is that following are still producing same alignment bumps at the same positions:

  e_ptr = e + ((e_sz + alignof(T) - 1) % alignof(T))
  d_ptr = (d_ptr + alignof(T) - 1) % alignof(T)

  ...
  e_sz = e_ptr + sizeof(T) - e;
  d_sz = d_ptr + sizeof(T) - d;


I think the align based on the size is superior, but it has one slight problem: the requirement is not to align the size, it is to align the pointer, mostly the decode pointer.

> > Source/WebKit/Platform/IPC/Encoder.cpp:189
> > +    size_t alignedSize = roundUpToMultipleOf(alignment, m_bufferSize);
> 
> I just realized that if we go this route, StreamConnectionEncoder is going
> to also need this adjustment.

I think StreamConnectionEncoder might be already covered, but it's better if you check.

The extra complication is that for IPC streams, there is no strict requirement of e % 4096 == d % 4096. The encoding / decoding base pointer moves according to the position in the ring buffer. However, the buffer is a SHM buffer, so by accident the base alignment is the same.

[Kimmo Kinnunen]

(In reply to Wenson Hsieh from comment #0)
> For instance, if the alignment is 16 bytes, then it is possible for
> IPC::Encoder to add more padding to adjust for alignment, which IPC::Decoder
> may not add if the buffer pointer offset (`m_bufferPos`) just so happens to
> land on a 16-byte-aligned address.

So in concrete terms:
If both start at `address % 4096` and are at size sz, then how is it possible that
`sz % 16 != m_bufferPos % 16`?

Where `m_bufferPos == ((decodeAddress % 4096) + sz`

[Kimmo Kinnunen]

Maybe decode buffer is not % 4096 for some reason?

[Wenson Hsieh]

(In reply to Kimmo Kinnunen from comment #7)
> Maybe decode buffer is not % 4096 for some reason?

To clarify — what guarantees that the decode buffer is aligned to 4096? (Is it that the decode buffer is expected to be aligned to the start of a page?)

[Kimmo Kinnunen]

(In reply to Wenson Hsieh from comment #8)
> (In reply to Kimmo Kinnunen from comment #7)
> > Maybe decode buffer is not % 4096 for some reason?
> 
> To clarify — what guarantees that the decode buffer is aligned to 4096? (Is
> it that the decode buffer is expected to be aligned to the start of a page?)

I guess it is just me rationalising why the code can be like that.

It seems obvious it's not -- for inline data it's the Vector allocation in readFromMachPort in ConnectionCocoa.mm. For out of line data, it might hold true, though.

Maybe the assumption was like that when inline data wasn't used, and then inline was added but the code stayed like that? I don't know..

[Radar WebKit Bug Importer]

<rdar://problem/83786333>