Zero ScreenDetails outparam in RecvScreenForBrowser() so Valgrind doesn't complain

RESOLVED FIXED in mozilla36

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
There's one undefined value error left on Try when I run the valgrind test with
e10s enabled. It's another case where sendmsg() is passed undefined bytes.

Here's the complaint after I added a VALGRIND_CHECK_MEM_IS_DEFINED request in
Pickle::WriteBytes, which results in easier-to-understand errors:

> Uninitialised byte(s) found during client check request
>    at 0x85C784D: Pickle::WriteBytes(void const*, int, unsigned int) (pickle.cc:564)
>    by 0x877D752: WriteUInt32 (pickle.h:133)
>    by 0x877D752: Write (ipc_message_utils.h:297)
>    by 0x877D752: WriteParam<unsigned int> (ipc_message_utils.h:115)
>    by 0x877D752: Write<unsigned int> (PScreenManagerParent.h:239)
>    by 0x877D752: mozilla::dom::PScreenManagerParent::Write(mozilla::dom::ScreenDetails const&, IPC::Message*) (PScreenManagerParent.cpp:505)
>    by 0x877E123: mozilla::dom::PScreenManagerParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (PScreenManagerParent.cpp:365)
>    by 0x86A085F: mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (PContentParent.cpp:4640)
>  Address 0xffeffe7c4 is on thread 1's stack
>  in frame #1, created by mozilla::dom::PScreenManagerParent::Write(mozilla::dom::ScreenDetails const&, IPC::Message*) (pickle.h:504)
>  Uninitialised value was created by a stack allocation
>    at 0x877DA94: mozilla::dom::PScreenManagerParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (PScreenManagerParent.cpp:177)

Around PScreenManagerParent.cpp:365 we have this generated code:

>   ScreenDetails screen;
>   bool success;
>   int32_t __id = mId;
>   if ((!(RecvScreenForBrowser(aBrowser, (&(screen)), (&(success))))))
>   {
>       mozilla::ipc::ProtocolErrorBreakpoint("Handler for ScreenForBrowser returned error code");
>       return MsgProcessingError;
>   }
> 
>   __reply = new PScreenManager::Reply_ScreenForBrowser(__id);
> 
>   Write(screen, __reply);     // line 365

Around PScreenManagerParent.cpp:505 we have this generated code:

>   auto PScreenManagerParent::Write(
>           const ScreenDetails& __v,
>           Message* __msg) -> void
>   {   
>       Write((__v).id(), __msg);                       // line 505
>       Write((__v).rect(), __msg);
>       Write((__v).rectDisplayPix(), __msg);
>       Write((__v).availRect(), __msg);
>       Write((__v).availRectDisplayPix(), __msg);
>       Write((__v).pixelDepth(), __msg);               // line 512
>       Write((__v).colorDepth(), __msg);               // line 513
>       Write((__v).contentsScaleFactor(), __msg);      // line 514
>   }

Valgrind complains about lines 505, 512, 513 and 514, which correspond to the
POD members of ScreenDetails, which are the ones that don't get initialized by
the zero-arg constructor.

So it looks like the RecvScreenForBrowser() call on line 358 is succeeding
without initializing |screen|. And sure enough...

> bool
> ScreenManagerParent::RecvScreenForBrowser(PBrowserParent* aBrowser,
>                                           ScreenDetails* aRetVal,
>                                           bool* aSuccess)
> {   
>   *aSuccess = false;
>     
>   // Find the mWidget associated with the tabparent, and then return
>   // the nsIScreen it's on.
>   TabParent* tabParent = static_cast<TabParent*>(aBrowser);
>   nsCOMPtr<nsIWidget> widget = tabParent->GetWidget();
>   if (!widget) {
>     return true;
>   }
> 
>   nsCOMPtr<nsIScreen> screen;
>   if (widget->GetNativeData(NS_NATIVE_WINDOW)) {
>     mScreenMgr->ScreenForNativeWidget(widget->GetNativeData(NS_NATIVE_WINDOW),
>                                       getter_AddRefs(screen));
>   }
> 
>   NS_ENSURE_TRUE(screen, true);
> 
>   ScreenDetails details;
>   if (!ExtractScreenDetails(screen, details)) {
>     return true;
>   }
> 
>   *aRetVal = details;
>   *aSuccess = true; 
>   return true;
> }

... look at all those |true| values returned on error paths. It's definitely
possible for |*aRetVal| to be left uninitialized. In fact, this whole function
looks odd, but I guess the fact that |*aSuccess| is false on those paths
somehow makes it ok? I.e. the process receiving the message won't inspect
ScreenDetails if |*aSuccess| is false? (I see |*aSuccess| is part of the
message.)

Anyway, zeroing |*aRetVal| should do be enough to satisfy Valgrind.
(Assignee)

Updated

3 years ago
Blocks: 1069695
(Assignee)

Comment 1

3 years ago
Created attachment 8523634 [details] [diff] [review]
Zero ScreenDetails outparam in RecvScreenForBrowser() so Valgrind doesn't complain
Attachment #8523634 - Flags: review?(mconley)
It's kinda lame that IPDL doesn't initialize the POD members of its own structs though. Shouldn't we just fix IPDL to do this for us? (I guess as a followup)
Comment on attachment 8523634 [details] [diff] [review]
Zero ScreenDetails outparam in RecvScreenForBrowser() so Valgrind doesn't complain

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

Thanks for the analysis, njn.

(In reply to Nicholas Nethercote [:njn] from comment #0)
> 
> ... look at all those |true| values returned on error paths. It's definitely
> possible for |*aRetVal| to be left uninitialized. In fact, this whole
> function
> looks odd, but I guess the fact that |*aSuccess| is false on those paths
> somehow makes it ok? I.e. the process receiving the message won't inspect
> ScreenDetails if |*aSuccess| is false? (I see |*aSuccess| is part of the
> message.)

That's the idea, yes. We're returning true in this case, because a false forces the child process to crash (because as far as I can tell, the boolean return value for these IPC functions has less to do about the success of the operation, and more to do with the success of the communication).

Correct - we do not inspect ScreenDetails if *aSuccess is false.

> 
> Anyway, zeroing |*aRetVal| should do be enough to satisfy Valgrind.

I'll take your word for it. :)
Attachment #8523634 - Flags: review?(mconley) → review+
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/28036503ada1
https://hg.mozilla.org/mozilla-central/rev/28036503ada1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.