570291 - dump() inside sandbox truncates string's high bytes

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Drew Willcoxon :adw]

dump()ing a string inside a sandbox seems to cause all multi-byte characters to have their high bytes lopped off.  In this snippet, the first dump prints "?", while the subsequent three dumps print the expected multi-byte char:

  var code = '
    var str = "\\u6587";
    dump(str);
    win.dump(str);
    doDump1(str);
    doDump2();
  ';
  var p = Components.classes["@mozilla.org/systemprincipal;1"].
          createInstance(Components.interfaces.nsIPrincipal);
  var sb = new Components.utils.Sandbox(p);
  sb.win = window;
  sb.doDump1 = function (str) dump(str);
  sb.doDump2 = function () dump("\u6587");
  Components.utils.evalInSandbox(code, sb);

Via Jetpack bug 569271.

Comment 2

[Boris Zbarsky [:bzbarsky]]

How is this a duplicate of something about exception messages?  It's just a bug in SandboxDump (in xpccomponents.cpp), which uses JS_GetStringBytes on the JSString.  window.dump, on the other hand, uses ToNewUTF8String on the given Unicode string, which is at least guaranteed non-lossy (though will give you terminal conniptions if it's not speaking UTF-8, of course).

I assume that the UTF-8 behavior is the one we'd want out of SandboxDump?

Comment 3

[Drew Willcoxon :adw]

Attachment: patch

Thanks Boris.  Yes, the UTF-8 behavior is the one we'd want.  Here's a patch.  I'm only an aspiring JS and platform hacker, so apologies if this is way off base.

I'm not sure if casting jschar* to PRUnichar* is bulletproof on all platforms.  (There are multiple typedefs of PRUnichar and JSUint16, and it's difficult to follow which are operative here, but sometimes they're explicitly 16 bits, sometimes just shorts, sometimes signed, sometimes unsigned.)  Anyhow, there are three instances of such a cast already in xpccomponents.js, so that's what I've done.

At first I tried JS_EncodeCharacters, but it didn't work.  Apparently that's because UTF-8 support is disabled (?), since JS_CStringsAreUTF8 is returning false.

I copied the \r -> \n conversion from nsGlobalWindow::Dump, except it's not quite right, since it uses nsAString.Length() as the upper bound of iteration rather than the number of bytes.  (Indeed I tested window.dump() with a unicode string with a \r at the end, and it didn't convert the \r.)  So I've fixed that here.

Finally, nsGlobalWindow::Dump prints to stdout, while SandboxDump uses stderr.  Seems like it would be better if they were the same, but that's a different bug I guess.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 450062 [details] [diff] [review]
patch

> I'm not sure if casting jschar* to PRUnichar* is bulletproof on all
> platforms. 

It is.

> Apparently that's because UTF-8 support is disabled (?)

In Gecko's embedding of Spidermonkey, yes.

> except it's not quite right

Indeed.  Want to just fix nsGlobalWindow::Dump too?

I think we should fflush in the SandboxDump like we do in nsGlobalWindow.

One other comment; pass JS_GetStringLength(str) as the second arg to the wstr constructor?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Especially because JS_GetStringChars doesn't guarantee null-termination, in theory.

Comment 6

[Drew Willcoxon :adw]

Attachment: patch v2

Awesome, thanks Boris.  This patch adds the JS_GetStringLength arg to the wstr ctor.  I'll push it when I get a chance.

(In reply to comment #4)
> Indeed.  Want to just fix nsGlobalWindow::Dump too?

Filed bug 570986.  Thanks for the quick review there too.

Comment 7

[Drew Willcoxon :adw]

http://hg.mozilla.org/mozilla-central/rev/e7aef5f91259