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

RESOLVED FIXED in mozilla31

Status

()

--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bent.mozilla, Assigned: baku)

Tracking

({crash, testcase})

18 Branch
mozilla31
ARM
Gonk (Firefox OS)
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2g-crash])

Attachments

(2 attachments, 4 obsolete attachments)

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.
khuey, what's the eta here?
Flags: needinfo?(khuey)
khuey, are you trying to beat my record for slowest review? I won't give up the trophy easily.
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.
blocking-b2g: koi? → koi+
(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.
Flags: needinfo?(khuey)
Why do we need a set actor?  Why can't we just send an array of FileDescriptors?
Flags: needinfo?(bent.mozilla)
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.
Flags: needinfo?(bent.mozilla)
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.
Attachment #796361 - Flags: review?(khuey) → review+
This is just waiting on you to land, right?
Flags: needinfo?(bent.mozilla)
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).
Flags: needinfo?(bent.mozilla)
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.
blocking-b2g: koi+ → 1.3+
If we were okay with shipping this in 1.2, then why would this block 1.3?
blocking-b2g: 1.3+ → 1.3?
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.
Assignee: bent.mozilla → amarchesini
blocking-b2g: 1.3? → ---
(Assignee)

Comment 14

5 years ago
Created attachment 8362948 [details] [diff] [review]
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
Attachment #796361 - Attachment is obsolete: true
Attachment #8362948 - Flags: review?(khuey)
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.
Attachment #8362948 - Flags: review?(khuey)
Hey Andrea, are you still working on this?
Flags: needinfo?(amarchesini)
Duplicate of this bug: 979138
(Assignee)

Comment 18

5 years ago
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
Flags: needinfo?(amarchesini)
(Assignee)

Comment 19

5 years ago
Created attachment 8386198 [details] [diff] [review]
patch m-c
Attachment #8362948 - Attachment is obsolete: true
Attachment #8386198 - Flags: review?(bent.mozilla)
Comment on attachment 8386198 [details] [diff] [review]
patch m-c

Canceling review until the xpcshell crash is worked out.
Attachment #8386198 - Flags: review?(bent.mozilla)
(Assignee)

Comment 21

5 years ago
Created attachment 8389124 [details] [diff] [review]
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.
Attachment #8389124 - Flags: feedback?(bent.mozilla)

Updated

5 years ago
blocking-b2g: --- → 1.4?
Blocks: 959089
Why is this needed for 1.4?

Updated

5 years ago
blocking-b2g: 1.4? → 1.4+

Updated

5 years ago
blocking-b2g: 1.4+ → 1.5?
(Assignee)

Comment 23

5 years ago
Created attachment 8389945 [details] [diff] [review]
patch 2
Attachment #8389124 - Attachment is obsolete: true
Attachment #8389124 - Flags: feedback?(bent.mozilla)
Attachment #8389945 - Flags: review?(bent.mozilla)
(Assignee)

Comment 24

5 years ago
Created attachment 8390420 [details] [diff] [review]
patch 2

A file was missing in the previous patch.
Attachment #8389945 - Attachment is obsolete: true
Attachment #8389945 - Flags: review?(bent.mozilla)
Attachment #8390420 - Flags: review?(bent.mozilla)
(Assignee)

Comment 25

5 years ago
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 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
Attachment #8390420 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 27

5 years ago
> ::: 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.
(Assignee)

Comment 28

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=1401a4779903 green on try.
Can we land it?
https://hg.mozilla.org/mozilla-central/rev/6a131333a0b1
https://hg.mozilla.org/mozilla-central/rev/9546241b9345
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 988251
See Also: → bug 988251

Updated

5 years ago
blocking-b2g: 2.0? → ---
You need to log in before you can comment on or make changes to this bug.