Make DMD work with --enable-content-sandbox

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

Trunk
mozilla33
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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).
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.
Assignee: nobody → jld
Attachment #8448725 - Flags: review?(bent.mozilla)
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.
Attachment #8448727 - Flags: review?(n.nethercote)
Step 3: Stop disabling the sandbox when DMD is used.
Attachment #8448729 - Flags: review?(gdestuynder)
Comment on attachment 8448729 [details] [diff] [review]
bug956961-p2-resandbox-dmd-hg0.diff

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

nice :)
Attachment #8448729 - Flags: review?(gdestuynder) → review+
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 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 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.
Attachment #8448727 - Flags: review?(n.nethercote) → review+
(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?
Maybe just use InitANSIFileDesc(), which pairs up nicely with OpenANSIFileDesc(). Thanks.
With revisions; carrying over r+.
Attachment #8448727 - Attachment is obsolete: true
Attachment #8449110 - Flags: review+
Comment on attachment 8448725 [details] [diff] [review]
bug956961-p0-fdutils-hg0.diff

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

Ok
Attachment #8448725 - Flags: review?(bent.mozilla) → review+
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
Flags: needinfo?(jld)
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.
Attachment #8449913 - Flags: review?(n.nethercote)
Flags: needinfo?(jld)
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!
Attachment #8449913 - Flags: review?(n.nethercote) → review+
You need to log in before you can comment on or make changes to this bug.