1525199 - nsTString length serialized as uint32_t on aarch64, deserialized as 8 byte size_t on Win32

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P1)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'm trying to run a 32bit x86 plugin-container process in order to host x86 GMPs and run them for an aarch64 parent process.

One of the startup problems is that we fail to send the initial PGMP Msg_ProvideStorageId__ID message. Deserializing the nsCString argument fails on the x86 side.

The problem turns out to be that the x86 side is deserialzing the length parameter as an uint32_t [1] which due to some C++ template specialization mumbo-gumbo results in us ending up running the size_t deserialization code here [2]. I guess because size_t is defined as "unsigned int" on x86. size_t is always serialized/deserialized using 8 bytes in order to be cross-arch safe [3].

This results in a mismatch in the IPC; we have a aarch64 parent process that writes the string length as 4 byte uint32_t, and the child tries to read an 8 byte size_t. Then we get out of sync and the wheels fall off the IPC in the child process.

Just deleting the specialization for size_t at [2] made us get past this error on GMP startup, but I worry that doing that would cause things that rely on serializing a size_t to be quietly incorrectly deserialized in a cross-arch IPC process at some point in future.

[1] https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/ipc/glue/IPCMessageUtils.h#373
[2] https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/ipc/chromium/src/chrome/common/ipc_message_utils.h#320
[3] https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/ipc/chromium/src/base/pickle.cc#278

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

So the problem is that the inheritance hierarchy of ParamTraits we use as described in ipc_message_utils.h [1], size_t is further down the hierarchy, and since size_t is defined as unsigned int on 32bit windows, the template for uint32_t matches against size_t instead of uint32_t. And then the size_t template expects to read 8 bytes instead of 4 as described above.

Looking through Chromium's code, I see they have comments claiming that they have static analysis to block for size_t being added in new IPC [2] messages.

They also no longer have Pickle::ReadSize() and WriteSize(). They must have removed it since we forked their IPC code.

Since we're trying to do IPC between 32/64bit processes, I don't think we can allow size_t to be sent across the boundary. If we have a ParamTraits for size_t, it'll match whenever we have an unsigned int on Win32, and uint64_t on Win64, so we'll get consistent results.

I've been unable to figure out how to write a template that can distinguish between a size_t and an unsigned int. I don't think it's possible.

So I think it makes sense to do what Chromium has done, and outlaw size_t in IPC messages, and remove it from ParamTraits.

We have 4 places where we send size_t over IPC [3], those will need to be changed.

Nathan: Are you the right person to comment on this proposal? Should it be someone else?

[1] https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/ipc/chromium/src/chrome/common/ipc_message_utils.h#64
[2] https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.h?l=104&rcl=fd2c2d308979cf9dbbc5ea2bc4309abc1f37f369
[3] https://searchfox.org/mozilla-central/search?q=size_t&path=*.ipdl

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

I took a swing at removing size_t support in IPDL. I ended up just replacing them all with uint32_t as a first pass https://hg.mozilla.org/try/rev/c0605318943ac29c8f2e0ef9aa1a0cdfdb2000a8 most of the size_t's are related to shmem sizes. Will we ever have a shmem that's >4GB?

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Chris Pearce [:cpearce (GMT+13)] from comment #1)

I've been unable to figure out how to write a template that can distinguish between a size_t and an unsigned int. I don't think it's possible.

I think this is correct.

So I think it makes sense to do what Chromium has done, and outlaw size_t in IPC messages, and remove it from ParamTraits.

The latter is easy enough, but the former would require some kind of static analysis, yes? Since people could still ReadParam/WriteParam size_t values, those values would just be written according to the existing template specializations for the underlying type, right?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #2)

I took a swing at removing size_t support in IPDL.

Thank you!

I ended up just replacing them all with uint32_t as a first pass https://hg.mozilla.org/try/rev/c0605318943ac29c8f2e0ef9aa1a0cdfdb2000a8 most of the size_t's are related to shmem sizes. Will we ever have a shmem that's >4GB?

I think it's reasonable to assume we're unlikely to want shmem's bigger than 4GB, and even if we do want them, we can handle sizes those that we want a case-by-case basis.

I think we should still be checking that we don't have a size > UINT_MAX when we convert from a 64bit size_t to a uint32_t, so that we fail in a defined fashion if this ever occurs.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

ipc::Shmem itself has a 32-bit size limit.

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1525199 - Part 1 - removed size_t from IPDL messages for Cameras; r?gcp

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1525199 - Part 2 - removed size_t from GfxVarUpdate IPC messages; r?jgilbert

These are indexes into an array of prefs so we're nowhere near needing to worry
about >32-bit values.

Depends on D19191

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1525199 - Part 3 - removed size_t from ShmemSection IPDL struct, shmems are already limited to 32-bit sizes; r?jrmuizel

Depends on D19193

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1525199 - Part 4 - removed size_t support from IPDL messages; r?froydnj

Depends on D19195

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd21500d5111
Part 1 - removed size_t from IPDL messages for Cameras; r=gcp
https://hg.mozilla.org/integration/autoland/rev/8fd5de43289b
Part 2 - removed size_t from GfxVarUpdate IPC messages; r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/439573cc34ee
Part 3 - removed size_t from ShmemSection IPDL struct, shmems are already limited to 32-bit sizes; r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/6a5675d27fd7
Part 4 - removed size_t support from IPDL messages; r=froydnj

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/cd21500d5111
https://hg.mozilla.org/mozilla-central/rev/8fd5de43289b
https://hg.mozilla.org/mozilla-central/rev/439573cc34ee
https://hg.mozilla.org/mozilla-central/rev/6a5675d27fd7