1154522 - Why don't we initialize IPDL structs?

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It seems like the constructor for IPDL structs could initialize fields like this:
  mFoo(),
  mBar(),
  mBaz()

This way, primitive types would get initialized to something. That would have avoided bug 1152921.

Ben, does this sound reasonable to you?

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We talked through this in the office today and I'm not 100% against it but I'm also not sold on the value.

Currently you're supposed to set all the members of the struct before you send it over IPC. The easy way to do that is to use the generated constructor that takes all the values. We have a default constructor for the cases where you can't do everything at once, but you're still expected to set all the members.

Initializing sounds kind of nice but then we never would have noticed that this field isn't being explicitly set... And if we added a new member to the struct later on the old code would continue to compile just fine... And in this case if we had default-initialized then we wouldn't have crashed but we might have gone down the wrong code path silently.

It's cool that DEBUG builds caught this on the receiving side, but ideally we'd have similar checks on the sending side. And of course there's no guarantee that an uninitialized value could be detected (bool is easy since it should be 0 or 1, but what about other stuff?).

I don't know what we should do here really. We could force IPDL structs to default-initialize to some kind of poison pattern and beef up our DEBUG checking somehow. Or we could get rid of the default constructor entirely and force the use of the all-args constructor.

But default-initializing doesn't seem to solve anything really.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Also, valgrind catches stuff like this. If we default-initialize then we lose that too.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> But default-initializing doesn't seem to solve anything really.

Well, it would have solved 1152921 :-).

I can see that default initialization can have downsides. But most languages do it and nobody complains. Even when default initialization hides bugs, it usually hides them in a non-security critical way.

There's certainly room for disagreement here, and some sort of static checking would of course be best. But the argument for valgrind here just seems kinda weak. We don't have very good valgrind coverage and we're unlikely to improve it.

Not default-initializing can also be confusing. We do initialize strings and arrays. People may assume, based on that, that we initialize everything. We could poison everything, but I suspect that would just unearth a bunch of "bugs" that aren't actually problems.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Bill McCloskey (:billm) from comment #3)
> Well, it would have solved 1152921 :-)

Maybe... Did the author think the bool member defaulted to false? Or did they forget it?

> Even when default initialization hides bugs, it
> usually hides them in a non-security critical way.

We're not talking about security problems here though. Presumably everyone has added arg verification to their Recv* functions to make sure that they know what they're dealing with. (Wishful thinking?) Default-initializing won't change that.

> We don't have very good valgrind coverage and we're
> unlikely to improve it.

I hope that's not the case! I thought we had folks working on that?

> Not default-initializing can also be confusing. We do initialize strings and
> arrays.

Maybe we should just get rid of the default constructors then?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #4)
> (In reply to Bill McCloskey (:billm) from comment #3)
> > Well, it would have solved 1152921 :-)
> 
> Maybe... Did the author think the bool member defaulted to false? Or did
> they forget it?

Author here. Probably a mixture of both. Coming from JS-land, I just (erroneously) assumed that the struct would be initialized all around, and that booleans would be "false-y" to begin with. Uninitialized memory had not crossed my mind, I'm afraid.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Can we support defining the default value in the .ipdl(h) file?  That way its still explicit, but we don't have to duplicate boilerplate everywhere the type is used.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

I'm kinda holding off on reviewing dbaron's fix for a crash caused by uninitialized struct values in bug 1152921 - work that would be invalidated if we initialized struct primitives.

Are we likely to come to a decision here soon-ish? If not, I'll just review his patch and not block on this.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7)

I think you want dbaron's patch no matter what because it sends less data over IPC in failure cases.

Not sure if we have agreement here yet. I'm sorta "meh" on the whole thing.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

This seems pretty valuable for safety. It's also convenient since IPDL lacks the ability to specify default values, so it eliminates boilerplate (either from message handling code or having to do your own C++ struct).

I would like a syntax for explicit default values, like:
  STRUCT : TYPE ID
         | TYPE ID "=" VALUE

Where VALUE could be a constant or a name. But for now this is simpler.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3f61cc23c9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e3f61cc23c9