956961 - Make DMD work with --enable-content-sandbox

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

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

Currently, my patches for bug 946407 (e10s'ing about:memory to not do file I/O from content processes) doesn't cover DMD — sending IPC messages from the DMD analysis causes a deadlock — so, as a workaround, it disables sandboxing for DMD builds.

Because DMD builds are normally used only when a memory leak can't be investigated without it, it is hoped that the security impact is minimal.  Even so, this should ideally be fixed at some point.

Possibilities: letting the I/O thread bypass malloc interception during the DMD logging (problem: might cause spurious under/over-reporting on the next run); buffering the entire gzipped log in memory (problem: allocating that memory); and sending a plain file descriptor to a local file or pipe (problem: extracting the fd/handle from the NSPR or XPCOM interfaces we'd normally use).

Comment 1

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

Attachment: bug956961-p0-fdutils-hg0.diff

Step 1: fix a slight oversight in FileDescriptorUtils so that sending a null FILE* over IPC will work.  No effect on non-debug builds intended.

Comment 2

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

Attachment: bug956961-p1-dmd-fileopen-hg0.diff

Step 2: separate the file-opening and file-writing parts of DumpDMD and move the former into the parent, similarly to how GC/CC logs are now handled.

Comment 3

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

Attachment: bug956961-p2-resandbox-dmd-hg0.diff

Step 3: Stop disabling the sandbox when DMD is used.

Comment 4

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

Comment on attachment 8448729 [details] [diff] [review]
bug956961-p2-resandbox-dmd-hg0.diff

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

nice :)

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8448727 [details] [diff] [review]
bug956961-p1-dmd-fileopen-hg0.diff

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

Passing on a few notes after a quick look. Has this been tested on B2G? It seems like each process being run as a different user could cause issues with passing FDs around. Maybe not, just curious. Also I'm guessing that UUID's in the IDL files will need to be updated.

::: dom/ipc/ContentParent.cpp
@@ +2465,5 @@
> +            nsresult rv = nsMemoryInfoDumper::OpenDMDFile(dmdIdent, Pid(), &dmdFile);
> +            if (!NS_WARN_IF(NS_FAILED(rv)) && dmdFile) {
> +                dmdFileDesc = FILEToFileDescriptor(dmdFile);
> +                fclose(dmdFile);
> +            }

Presumably there should be error handling here?

::: xpcom/base/nsGZFileWriter.cpp
@@ +58,4 @@
>  
>    // gzdopen returns nullptr on error.
>    if (NS_WARN_IF(!mGZFile)) {
>      return NS_ERROR_FAILURE;

Not your bug, but aren't we leaking a dup'd FD at this point?

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ +731,5 @@
> +  rv = OpenDMDFile(aIdentifier, getpid(), &dmdFile);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +  if (!dmdFile) {

Maybe just combine this with the previous if:
  if (WARN || !dmdFile) return rv;

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8448727 [details] [diff] [review]
bug956961-p1-dmd-fileopen-hg0.diff

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

::: dom/ipc/ContentParent.cpp
@@ +2465,5 @@
> +            nsresult rv = nsMemoryInfoDumper::OpenDMDFile(dmdIdent, Pid(), &dmdFile);
> +            if (!NS_WARN_IF(NS_FAILED(rv)) && dmdFile) {
> +                dmdFileDesc = FILEToFileDescriptor(dmdFile);
> +                fclose(dmdFile);
> +            }

Sorry I misread that, I see what's going on now.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8448727 [details] [diff] [review]
bug956961-p1-dmd-fileopen-hg0.diff

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

Thanks for fixing this. It's annoying how much plumbing is required for all this stuff :(

r=me with the uuids changed, as erahm mentioned.

::: xpcom/base/nsGZFileWriter.cpp
@@ +46,5 @@
>    nsresult rv = aFile->OpenANSIFileDesc("wb", &file);
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
> +  return InitANSI(file);

I see that OpenANSIFileDesc() already exists... but I have no idea what the "ANSI" means here. Well, I guess it's short for "American National Standards Institute" but I don't know why that's relevant.

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ +731,5 @@
> +  rv = OpenDMDFile(aIdentifier, getpid(), &dmdFile);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +  if (!dmdFile) {

Eric suggested merging these, but I actually like having them separate.

Comment 8

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

(In reply to Eric Rahm [:erahm] from comment #5)
> Passing on a few notes after a quick look. Has this been tested on B2G?

Yes — at the moment that's the only place where the seccomp-bpf sandboxing support is known to work, in fact.

> It seems like each process being run as a different user could cause issues
> with passing FDs around. Maybe not, just curious.

Nope.  Permission checks are done when opening the file; once it's opened, the fd is a capability to access the file.  (This is the Unix model; the NFS model is different, but that's not an issue on B2G.)

> Also I'm guessing that UUID's in the IDL files will need to be updated.

Yes; thanks for catching that.

> ::: dom/ipc/ContentParent.cpp
> @@ +2465,5 @@
> > +            nsresult rv = nsMemoryInfoDumper::OpenDMDFile(dmdIdent, Pid(), &dmdFile);
> > +            if (!NS_WARN_IF(NS_FAILED(rv)) && dmdFile) {
> > +                dmdFileDesc = FILEToFileDescriptor(dmdFile);
> > +                fclose(dmdFile);
> > +            }
> 
> Presumably there should be error handling here?

Or a comment, to clarify that the "error handling" is deliberately proceeding with the memory report as if DMD were disabled.

(In reply to Nicholas Nethercote [:njn] from comment #7)
> ::: xpcom/base/nsGZFileWriter.cpp
> @@ +46,5 @@
> >    nsresult rv = aFile->OpenANSIFileDesc("wb", &file);
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> >      return rv;
> >    }
> > +  return InitANSI(file);
> 
> I see that OpenANSIFileDesc() already exists... but I have no idea what the
> "ANSI" means here. Well, I guess it's short for "American National Standards
> Institute" but I don't know why that's relevant.

I assume it's a reference to the ANSI C standard, and by extension <stdio.h> and FILE*, in contradistinction to nsIFile::OpenNSPRFileDesc.  I was trying to follow what little existing precedent there was for FILE* in XPIDL, but maybe something else would be clearer: InitStdio? InitOpened?

Comment 9

[Nicholas Nethercote [inactive]]

Maybe just use InitANSIFileDesc(), which pairs up nicely with OpenANSIFileDesc(). Thanks.

Comment 10

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

Attachment: bug956961-p1-dmd-fileopen-hg1.diff

With revisions; carrying over r+.

Comment 11

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

Comment on attachment 8448725 [details] [diff] [review]
bug956961-p0-fdutils-hg0.diff

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

Ok

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/56907af18c66
https://hg.mozilla.org/integration/mozilla-inbound/rev/272b01e4f856
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1be89cb58b9

Comment 13

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

Backed these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/466138f414d8 for being the likely cause of this non-unified build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=42969344&tree=Mozilla-Inbound

Comment 14

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

Attachment: Incremental diff for non-unified build breakage.

Trying: https://tbpl.mozilla.org/?tree=Try&rev=f3da3c9c7628 (based on the ill-fated m-i push, so TBPL doesn't show it here)

I'll squash this with attachment 8449110 [details] [diff] [review] when relanding.

Comment 15

[Nicholas Nethercote [inactive]]

Comment on attachment 8449913 [details] [diff] [review]
Incremental diff for non-unified build breakage.

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

This change is small and simple enough that I wouldn't have asked for a review if I had written it. Still, r=me!

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f46f5f29bd86
https://hg.mozilla.org/integration/mozilla-inbound/rev/35790c8d0aaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/63538074b343

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f46f5f29bd86
https://hg.mozilla.org/mozilla-central/rev/35790c8d0aaf
https://hg.mozilla.org/mozilla-central/rev/63538074b343