1272704 - NSPR logging appears to be broken on Windows from Content processes

Status: RESOLVED DUPLICATE of bug 1320458

(Core :: XPCOM, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

Per bobowen, it's broken even if you disable the sandbox.  This was tested with Nightly 5/13.

from mozilla-build shell:
NSPR_LOG_MODULES=.... NSPR_LOG_FILE=.... ./firefox.exe -P foo

Comment 1

[Bob Owen (:bobowen)]

Looks like this was the sandbox after all.

The issue is that while nspr (through CreateFile) allows forward slashes in the path on Windows, nsLocalFile (that I use to check and potentially create an absolute path) rejects forward slashes as one of its first checks at [1].

Looks like it's been like this since the first commit in 1999, see [2], the committer was probably partying.

The question is ... given that nsLocalFile also uses CreateFile, should we allow forward slash or is it too dangerous to change this given it's been like this for so long?

If not I'll just translate before I do anything, which I don't think carries any additional risk.

[1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/xpcom/io/nsLocalFileWin.cpp#1166
[2] https://github.com/ehsan/mozilla-cvs-history/blob/77fa6a44206f403aef8fb78798eb3d47144dfce8/xpcom/io/nsLocalFileWin.cpp#L538

Comment 2

[Nathan Froyd [:froydnj]]

If the underlying Windows APIs allow it, we should probably just go ahead and allow it.  My sense is that since we're only adding behavior in a former error case, and not taking any away, it shouldn't cause any problems.  The nsLocalFile code probably needs a good bit of auditing after making the change, though.  Doing translation in the sandbox code seems...not good, but I am far from an expert there.

That said, I'm not a Windows expert, so I'd like to see what Jim says.

Comment 3

[Jim Mathies [:jimm]]

> The question is ... given that nsLocalFile also uses CreateFile, should we
> allow forward slash or is it too dangerous to change this given it's been
> like this for so long?

Yes lets put this together and land it, see if there's any fallout.

Comment 4

[Bob Owen (:bobowen)]

Looks like this will be fixed by the new code that handles this in bug 1320458.