1664553 - Explicitly default-initialize local variables generated by IPDL

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

Doing this helps lower the chances of accidentally trying to send an uninitialized primitive value, like a raw pointer or integer, over IPC due to a sync method or IPDLParamTraits::Read implementation failing to initialize the outparameter.

This would avoid potential errors like those fixed in bug 1664323 from recurring.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1664553 - Default initialize outparameters in ipdl generated code,

Doing this helps lower the chances of accidentally trying to send an
uninitialized primitive value, like a raw pointer or integer, over IPC due to a
sync method or IPDLParamTraits::Read implementation failing to initialize the
outparameter.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f28c2c439d5b
Default initialize outparameters in ipdl generated code, r=jld

Comment 3

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f28c2c439d5b

Comment 4

[Simon Giesecke [:sg] [he/him]]

Hm, I am not sure if that's really a good idea. While this would have "fixed" bug 1664323, I think it's more appropriate to say this would have masked bug 1664323, and will mask similar errors. In general, it's not set that the default value is sensible (for pointers, it might still make sense to initialize them to nullptr for security reasons, but for other types I doubt that). It's a general problem of output parameters that it's ambiguous of what to assume about their value on call. An alternative (or rather follow-up) to doing this, would be to replace output parameters by return values altogether (maybe using mozilla::Result), which avoids masking such mistakes, and them not impossible but much less likely, and also reduces the chances of brain overload due to the ambiguities of output parameters. Unfortunately, this will probably require quite extensive changes across the codebase.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

I don't think this counts as "masking" an issue - it's turning a bug from being something buggy which sends uninitialized memory over IPC to a bug which doesn't interact with uninitialized memory. Callers should obviously still initialize their outparameters, but it's now much less likely to cause serious issues in untested edge-cases down the line if someone forgets (which is somewhat easy to do).

The nicer solution of using mozilla::Result instead of an outparameter sounds nice, but I don't think it's worth doing. The particular codepath we're talking about here is for sync IPC, which is somewhat uncommon in our codebase, quite error-prone, and we're explicitly trying to avoid adding new users of. I think it's OK to continue to have bad ergonomics for sync IPC, and it's not worth the effort to make it nicer to use.

Comment 6

[Simon Giesecke [:sg] [he/him]]

I wasn't aware that this is only relevant for a few cases. Then I also thinik it's not worth changing this.