945504 - Include JS stack in sandbox reporter logs

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Sometimes, the relevant code to blame for a sandbox violation is JavaScript — e.g., a component that calls FileUtils.  In these cases the C++ part of the stack (as extracted via the crash reporter, for example) frequently isn't very helpful.

Because this isn't a "real" crash and the memory state should be consistent, it should be okay to interrogate the current thread's JS context (if any) and write it to the logs.

For TBPL+B2G, the logcat will be captured and mozharness will flag the immediately preceding line about the violation itself as an error, so this information should be reasonably obvious.

Depends on bug 945498 because of patch dependency.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug945504-sandbox-js-stack-hg0.diff

Should be fairly straightforward, assuming I'm using these XPCOM interfaces correctly.

Comment 2

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 8342166 [details] [diff] [review]
bug945504-sandbox-js-stack-hg0.diff

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

Looks fine to me and yes indeed, that's needed.
I don't have much more experience with this than you do, but it looks correct.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug945504-sandbox-js-stack-hg1.diff

Newer, smaller patch.  (Based on the new version of bug 945498.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8367732 [details] [diff] [review]
bug945504-sandbox-js-stack-hg1.diff

Seems pretty reasonably, but this line is buggy:

  +    nsresult rv = frame->GetCaller(getter_AddRefs(frame));

because it depends on the getter_AddRefs(frame) (which will null out "frame") happening after the "frame->" deref, which is very much not guaranteed in C++.  Probably better to do:

  nsCOMPtr<nsIStackFrame> temp;
  nsresult rv = frame->GetCaller(getter_AddRefs(temp));
  NS_ENSURE_SUCCESS_VOID(rv);
  temp.swap(frame);

Comment 5

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 8367732 [details] [diff] [review]
bug945504-sandbox-js-stack-hg1.diff

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

I'd just fix what bz mentioned. From my pov otherwise its fine

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug945504-sandbox-js-stack-hg3.diff

Carrying over r=kang.  Fixed the nsCOMPtr sequence point thing and rebased.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

C-N warning: depends on patch from bug 945498.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Bitrotted, please rebase.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug945504-sandbox-js-stack-hg4.diff

There was a brief window after bug 945498 landed (see comment #7) and before bug 969126 landed where the previous patch would have applied cleanly.  That time is no longer.  So, rebased on b2g-inbound.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Should apply cleanly to b2g-inbound today, and other inbounds tomorrow (patch context dependency on Hg commit eb8002584068).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea08da71f1b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ea08da71f1b