910010 - Child process crash when sending more than 4 file descriptors in a single IPC message

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

Bug 908432 works around the problem of sending too many file descriptors in one message by reducing unnecessary blob wrapping. It doesn't solve the underlying problem. We need to send file descriptors separately from the serialized inputstream data or blobs.

Comment 1

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

Attachment: Patch, v1

Comment 2

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

khuey, what's the eta here?

Comment 3

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

khuey, are you trying to beat my record for slowest review? I won't give up the trophy easily.

Comment 4

[Andrew Overholt [:overholt]]

I spoke with Ben and this is something we should take for 1.2 as it avoids a potential crash with gluing together 4 slices of a file together into a single blob.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #3)
> khuey, are you trying to beat my record for slowest review? I won't give up
> the trophy easily.

Don't give me ideas.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why do we need a set actor?  Why can't we just send an array of FileDescriptors?

Comment 7

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

There's a hardcoded limit of file descriptors for posix messages, something to do with the way the kernel copies the descriptors or something. See http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/file_descriptor_set_posix.h#33

We can bump that number up (I think? Not sure what kind of effects that would have) but we'll never be able to prevent exceeding a hardcoded limit if we let JS create blobs built on an arbitrary number of slices.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 796361 [details] [diff] [review]
Patch, v1

Review of attachment 796361 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentChild.cpp
@@ +1341,5 @@
> +
> +bool
> +ContentChild::DeallocPFileDescriptorSet(PFileDescriptorSetChild* aActor)
> +{
> +    delete static_cast<FileDescriptorSetChild*>(aActor);

Does PWhatever not have a virtual dtor?  Seems like this should be unnecessary.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is just waiting on you to land, right?

Comment 10

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

No, this patch is based on the b2g18 branch, but we missed that release, and are now on to koi, which is based on trunk (or 26 or something).

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It seems pretty unlikely that people will hit this to a significant extent, and since we've already branched v1.2 I think we should just aim this for v1.3.

If the patch turns out to be safe we can always uplift to v1.2.

Comment 12

[Jason Smith [:jsmith]]

If we were okay with shipping this in 1.2, then why would this block 1.3?

Comment 13

[Andrew Overholt [:overholt]]

Removing blocking nom.  Let's uplift a patch for b2g26 when we have one.

baku told me he was going to work on porting bent's b2g18 patch to 26.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch m-c

This is just a rebase of the original patch.
I want to see if it's green on try.

https://tbpl.mozilla.org/?tree=Try&rev=2535b008e8ee

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8362948 [details] [diff] [review]
patch m-c

This broke xpcshell tests.  Clearing review flags.

If we want this in 1.4 please set the flag so that I know I should prioritize this review.

Comment 16

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

Hey Andrea, are you still working on this?

Comment 18

[Andrea Marchesini [:baku]]

not actively. I updated the patch but I guess I still need to fix a couple of xpcshell tests:

https://tbpl.mozilla.org/?tree=Try&rev=07ec14ae7903

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch m-c

Comment 20

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

Comment on attachment 8386198 [details] [diff] [review]
patch m-c

Canceling review until the xpcshell crash is worked out.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: patch 2

I still have problems with netwerk/test/unit_ipc/test_post_wrap.js xpcshell test but at least it doesn't crash. The file is moved to the parent and the parent is able to use it but somehow this is what is expected by the test:

--AaB03x
Content-Disposition: form-data; name="body"

0123456789
--AaB03x
Content-Disposition: form-data; name="files"; filename="test_readline6.txt"
Content-Type: application/octet-stream
Content-Length: 4097

xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx...

and this is what the current code does:

Content-Type: multipart/form-data; boundary=AaB03x
Content-Length: 4329

--AaB03x
Content-Disposition: form-data; name="body"

0123456789
--AaB03x
Content-Disposition: form-data; name="files"; filename="test_readline6.txt"
Content-Type: application/octet-stream
Content-Length: 4097

xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx...

still debugging it.

Comment 22

[Jason Smith [:jsmith]]

Why is this needed for 1.4?

Comment 23

[Andrea Marchesini [:baku]]

Attachment: patch 2

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch 2

A file was missing in the previous patch.

Comment 25

[Andrea Marchesini [:baku]]

Comment on attachment 8390420 [details] [diff] [review]
patch 2

Review of attachment 8390420 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/FileDescriptorSetParent.h
@@ +43,5 @@
> +  }
> +
> +  ~FileDescriptorSetParent()
> +  {
> +    MOZ_ASSERT(mFileDescriptors.IsEmpty());

This has to be removed because it can happen that the child is killed before removing using the fds. Correct?

Comment 26

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

Comment on attachment 8390420 [details] [diff] [review]
patch 2

Review of attachment 8390420 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Thanks.

::: dom/ipc/FileDescriptorSetParent.h
@@ +47,5 @@
> +    MOZ_ASSERT(mFileDescriptors.IsEmpty());
> +  }
> +
> +  virtual bool
> +  RecvAddFileDescriptor(const FileDescriptor& aFileDescriptor) MOZ_OVERRIDE

Nit: Let's go ahead and split this into a cpp file.

@@ +53,5 @@
> +    mFileDescriptors.AppendElement(aFileDescriptor);
> +    return true;
> +  }
> +
> +  nsAutoTArray<FileDescriptor, 1> mFileDescriptors;

I'd just use nsTArray. In most cases the point of this protocol is to hold more than one fd so we're probably not using the internal slot anyway.

::: netwerk/ipc/PNecko.ipdl
@@ +18,5 @@
>  include protocol PRemoteOpenFile;
>  include protocol PDNSRequest;
>  include protocol PChannelDiverter;
>  include protocol PBlob; //FIXME: bug #792908
> +include protocol PFileDescriptorSet;

Is this needed?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1208,5 @@
> +  if (!fds.IsEmpty()) {
> +    MOZ_ASSERT(gNeckoChild->Manager());
> +
> +    fdSet = gNeckoChild->Manager()->SendPFileDescriptorSetConstructor(fds[0]);
> +    if (fdSet) {

This can't fail on the child side without aborting, no need for null check.

@@ +1247,5 @@
>  
> +  if (fdSet) {
> +    FileDescriptorSetChild* fdSetActor =
> +      static_cast<FileDescriptorSetChild*>(fdSet);
> +    MOZ_ASSERT(fdSetActor);

Nit: unnecessary assert

Comment 27

[Andrea Marchesini [:baku]]

> ::: netwerk/ipc/PNecko.ipdl
> @@ +18,5 @@
> >  include protocol PRemoteOpenFile;
> >  include protocol PDNSRequest;
> >  include protocol PChannelDiverter;
> >  include protocol PBlob; //FIXME: bug #792908
> > +include protocol PFileDescriptorSet;
> 
> Is this needed?

yep because PNecko uses PFileDescriptorSet. and without this line it doesn't compile.

Comment 28

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=1401a4779903 green on try.
Can we land it?

Comment 29

[Andrea Marchesini [:baku]]

Landed after discuss it with bent on IRC.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6a131333a0b1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9546241b9345

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a131333a0b1
https://hg.mozilla.org/mozilla-central/rev/9546241b9345