Closed Bug 1268900 Opened 8 years ago Closed 8 years ago

Add opt-in way to immediately deserialize a message after serializing it

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: mccr8, Assigned: cyu)

References

Details

(Whiteboard: btpp-fixlater, e10st?)

Attachments

(5 files, 5 obsolete files)

We have deserialization crashes like bug 1259480 and bug 1265680 that can be difficult to figure out because the receiver only has the message to go on. I think it would be possible to add an IPDL attribute to codegen a Read() right after the Write() in a message send, so we'd crash immediately in the sender, hopefully with information about the data that went into the message available in the crash dump. This should be an attribute because most messages are fine, and there's going to be a decent amount of overhead to do this, in terms of memory and CPU.
Whiteboard: btpp-fixlater
Blocks: e10s-crashes
We put checks in both serialization and deserialization code. One example is that the union values should fall within valid ranges. Given that we should see more crashes in the sender instead of the receiver. Interestingly, bug 1259480 shows the contrary. Anyway, I think getting the message dump would be hugely helpful.
This is the change to the IPDL parser for:

intr protocol messageVerify {
child:
    // we'll read the message back before sending it out to detect message corruption.
    async foo() verify; 
};

Andrew, if you are OK with that, I'll continue with this approach and work on the code generation part.
Attachment #8751715 - Flags: feedback?(continuation)
Assignee: nobody → cyu
Comment on attachment 8751715 [details] [diff] [review]
IPC message verifier, part 1 (WIP)

This looks reasonable to me, but an IPC peer should really look at it.
Attachment #8751715 - Flags: feedback?(wmccloskey)
Attachment #8751715 - Flags: feedback?(continuation)
Attachment #8751715 - Flags: feedback+
Bug 1265680, which is crashes involving deserializing a HandleAccessKey message, is one place this would be useful. Also, bug 1259480, which is for SendReadPrefsArray.
Blocks: 1265680, 1259480
Comment on attachment 8751715 [details] [diff] [review]
IPC message verifier, part 1 (WIP)

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

This generally looks fine.

::: ipc/ipdl/ipdl/parser.py
@@ +551,5 @@
>          p[0] = p[3]
>  
> +def p_MessageModifiers(p):
> +    """MessageModifiers : MessageModifiers MessageModifier
> +                        | MessageModifier"""

The grammar here is ambiguous. I think you probably want something like OptionalMessageModifiers and then require a modifer be non-empty.
Attachment #8751715 - Flags: feedback?(wmccloskey) → feedback+
Attached patch Part 2: Code generation (WIP) (obsolete) — Splinter Review
Attachment #8751715 - Attachment is obsolete: true
Attachment #8755303 - Flags: review?(wmccloskey)
Attachment #8754789 - Attachment is obsolete: true
Attachment #8755304 - Flags: review?(wmccloskey)
Attached patch Part 3: IPDL test case (obsolete) — Splinter Review
Attachment #8755306 - Flags: review?(wmccloskey)
Comment on attachment 8755303 [details] [diff] [review]
Part 1: parser change for the message verifier

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

::: ipc/ipdl/ipdl/parser.py
@@ +546,5 @@
>          p[0] = [ ]
>      else:
>          p[0] = p[3]
>  
> +def p_MessageModifiers(p):

Please call this OptionalMessageModifiers.
Attachment #8755303 - Flags: review?(wmccloskey) → review+
Can you please attach an example of the code that gets generated for one of the verification methods?
Flags: needinfo?(cyu)
Flags: needinfo?(cyu)
A small fix: don't generate code when unnecessary (that the param list is empty).
Attachment #8755304 - Attachment is obsolete: true
Attachment #8755304 - Flags: review?(wmccloskey)
Attachment #8755826 - Flags: review?(wmccloskey)
Comment on attachment 8755826 [details] [diff] [review]
Part 2 : Code generation for the mesasge verifier

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

I'm not sure that the data we'll get back here will be that useful. Keep in mind that Message stores its data in a heap-allocated buffer. So copying the message to the stack won't mean that the message data is in the crashdump. However, I guess it would be interesting to know if the bug is happening during serialization or at the IPC layer.

::: ipc/ipdl/ipdl/lower.py
@@ +5130,5 @@
>                  for r in md.returns ]
>              + self.setMessageFlags(md, replyvar, reply=1)
>              + [ self.logMessage(md, replyvar, 'Sending reply ') ])
>  
> +    def genVerifyMessage(self, md, reply=0):

Please pass in errfn, params, and msgsrc as arguments and remove the reply param. I think you're using the wrong errfn in some cases, and it's also nicer to pass this stuff rather than assume anything about the caller.
Attachment #8755826 - Flags: review?(wmccloskey)
Comment on attachment 8755306 [details] [diff] [review]
Part 3: IPDL test case

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

Please add constructor and destructor messages to test those. Also, build with --enable-ipdl-tests to actually make sure this works :-).

::: ipc/ipdl/test/ipdl/ok/messageVerify.ipdl
@@ +1,5 @@
> +intr protocol messageVerify {
> +child:
> +    async msg1() verify;
> +    async msg2(uint32_t aParam1) verify;
> +    synv msg3()

sync
Attachment #8755306 - Flags: review?(wmccloskey) → review+
Attachment #8755307 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #15)
> Comment on attachment 8755826 [details] [diff] [review]
> Part 2 : Code generation for the mesasge verifier
> 
> Review of attachment 8755826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure that the data we'll get back here will be that useful. Keep in
> mind that Message stores its data in a heap-allocated buffer. So copying the
> message to the stack won't mean that the message data is in the crashdump.
> However, I guess it would be interesting to know if the bug is happening
> during serialization or at the IPC layer.
> 
I think we can alloca() some buffer (1MB max maybe) and copy part of the message of interest when we fail to readback the value and is about to crash.
IPDL test case.

Passed IPDL codegen front end test run locally.
Attachment #8755306 - Attachment is obsolete: true
Attachment #8760243 - Flags: review+
Changes:
- Rebased for message moving and sentinels.
- Use correct errfns.

Generated code:
--- PContentParent.cpp  2016-06-07 15:47:57.000000000 +0800
+++ /tmp/PContentParent_Verify.cpp      2016-06-07 15:47:00.041008797 +0800
@@ -8679,16 +8679,31 @@
             // Sentinel = 'prefs'
             (reply__)->WriteSentinel(3194398011);
             (reply__)->set_sync();
             (reply__)->set_reply();
 
             if (mozilla::ipc::LoggingEnabledFor("PContentParent")) {
                 mozilla::ipc::LogMessageForProtocol("PContentParent", OtherPid(), "Sending reply ", (reply__)->name(), mozilla::ipc::MessageDirection::eSending);
             }
+            IPC::Message msgverify__ = mozilla::Move((*(reply__)));
+            PickleIterator msgverifyIter__ = PickleIterator(msgverify__);
+            nsTArray<PrefSetting> prefsCopy;
+
+            if ((!(Read((&(prefsCopy)), (&(msgverify__)), (&(msgverifyIter__)))))) {
+                FatalError("Error deserializing nsTArray");
+                return MsgValueError;
+            }
+            // Sentinel = 'prefs'
+            if ((!(((&(msgverify__)))->ReadSentinel((&(msgverifyIter__)), 3194398011)))) {
+                FatalError("Error deserializing nsTArray");
+                return MsgValueError;
+            }
+            (msgverify__).EndRead(msgverifyIter__);
+            (*(reply__)) = mozilla::Move(msgverify__);
             return MsgProcessed;
         }
     case PContent::Msg_ReadFontList__ID:
         {
             (const_cast<Message&>(msg__)).set_name("PContent::Msg_ReadFontList");
             if (mozilla::ipc::LoggingEnabledFor("PContentParent")) {
                 mozilla::ipc::LogMessageForProtocol("PContentParent", OtherPid(), "Received ", ((&(msg__)))->name(), mozilla::ipc::MessageDirection::eReceiving);
             }
Attachment #8755826 - Attachment is obsolete: true
Attachment #8760625 - Flags: review?(wmccloskey)
Whiteboard: btpp-fixlater → btpp-fixlater, e10st?
Comment on attachment 8760625 [details] [diff] [review]
Part 2 : Code generation for the mesasge verifier

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

::: ipc/ipdl/ipdl/lower.py
@@ +4796,5 @@
>          sendok, sendstmts = self.sendAsync(md, msgvar)
>          method.addstmts(
>              stmts
> +            + self.genVerifyMessage(md.decl.type.verify, md.params,
> +                                    errfnSendCtor, ExprDeref(ExprVar('msg__')))

Every call to genVerifyMessage uses ExprDeref on the last arg. Maybe just have genVerifyMessage do that?
Attachment #8760625 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #21)
> Every call to genVerifyMessage uses ExprDeref on the last arg. Maybe just
> have genVerifyMessage do that?

Thanks for catching this. It's fixed in the checkin version.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: