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.

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.

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

Comment on attachment 8448729 [details] [diff] [review] bug956961-p2-resandbox-dmd-hg0.diff Review of attachment 8448729 [details] [diff] [review]: ----------------------------------------------------------------- nice :)

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.

(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+.

Comment on attachment 8448725 [details] [diff] [review] bug956961-p0-fdutils-hg0.diff Review of attachment 8448725 [details] [diff] [review]: ----------------------------------------------------------------- Ok

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

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

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 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!

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

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