Assertion failure: chars[length] == 0, at c:\builds\moz2_slave\m-cen-w32-d-000000000000000000\build\src\js\src\vm/String-inl.h:326 found via bughunter and reproduced on latest m-c tinderbox windows debug build Steps to reproduce: -> Load https://www.google.fi/maps/%4060.1957779%2C24.9524125%2C17z --> Assertion failure affects aurora and trunk

jan could you take a look, thanks

[Tracking Requested - why for this release]: bughunter found on google page

jttps://www.google.com/maps?hl=en&tab=wl is also affected

Locking for now just to be sure.

We're creating an external string under XMLHttpRequestBinding::get_responseText, but that string isn't null-terminated. JSExternalString::new_ expects a null-terminated string and we crash.

This is not valid anymore after bug 1324430. In that bug I introduced: + MOZ_MUST_USE bool NS_FASTCALL Assign(const self_type& aStr, + size_type aLength, + const fallible_t&); where you can assign to string A part of string B. In this scenario, char[last] can be different than 0. bz, can we handle this case in webidl bindings?

Actually, I think the assertion is wrong. Any reason why I cannot call JSExternalString::new_ with a bigger string, but passing a particular length?

(In reply to Andrea Marchesini [:baku] from comment #7) > Actually, I think the assertion is wrong. Any reason why I cannot call > JSExternalString::new_ with a bigger string, but passing a particular length? The reason I think is that the JS engine sometimes wants a "flat" (null-terminated) string, so if external strings don't always have that property, we would have to do something special for them. I think we *could* copy the chars and turn it into a non-external string... ensureFlat isn't that common btw, most consumers don't need a null-terminated string, so supporting this could be a nice win. I'll think about it more.

So... The documentation for JS_NewExternalString's "chars" argument at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_NewExternalString says quite explicitly: The array does not need to be zero-terminated. Forcing it to be null-terminated in this case would require Gecko to do a bunch of string-copying and bloat memory rather significantly in some cases, so if we can change the implementation to match the documented contract instead that would be super. That said, there _is_ a problem with the path in bug 1324430 that I hadn't realized at the time. The problem is twofold: 1) The new Assign method explicitly sets F_TERMINATED, but when aLength < aStr.Length() that's clearly bogus. 2) The string classes explicitly document that F_SHARED implies F_TERMINATED (see http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/xpcom/string/nsTSubstring.h#1113-1115) and it's possible that things depend on that somewhere; we'd have to audit. One place where I'm sure this comes up is that if we take this non-terminated string we created and pass it back in through xpconnect, we will end up doing nsStringBuffer::ToString with it (see the XPCStringConvert::IsDOMString(str) case in XPCConvert::JSData2Native), which will set F_TERMINATED on the target string. Then PromiseFlatString will do the wrong thing and not produce a null-terminated thing... :(

> That said, there _is_ a problem with the path in bug 1324430 that I hadn't realized at the time I filed bug 1330759 on this.

I can fix the JS side of this. It's unfortunate we don't have external strings in the shell, for fuzzing. I'll add a testing function for this.

This patch makes JSExternalString inherit from JSLinearString instead of JSFlatString. JSString::ensureFlat copies the external chars to a new null-terminated buffer, calls the finalizer, and turns the string into a vanilla flat string. This should be fine: ensureFlat isn't common and the plan is to remove flat strings completely (bug 1330776). I added newExternalString and ensureFlatString shell functions so the fuzzers can hammer this.

tracking for 52 as sec-high

Comment on attachment 8827145 [details] [diff] [review] Patch Review of attachment 8827145 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TestingFunctions.cpp @@ +1266,5 @@ > + > + RootedString str(cx, args[0].toString()); > + size_t len = str->length(); > + > + UniqueTwoByteChars buf(js_pod_malloc<char16_t>(len)); Ugh, using the raw functions and stuffing into a UniquePtr is bad form. Much better to use js::MakeUnique to avoid manual allocation anywhere: auto buf = js::MakeUnique<char16_t[]>(len); (You could use MakeUnique directly, too, if it's js::MakeUnique, but I dunno from this if it's the mozilla:: function or not. Explicit seems clearer.) ::: js/src/jit-test/tests/basic/external-strings.js @@ +1,5 @@ > +assertEq(newExternalString(""), ""); > +assertEq(newExternalString("abc"), "abc"); > +assertEq(newExternalString("abc\0def\u1234"), "abc\0def\u1234"); > + > +var o = {foo: 2}; Perhaps tack in a "foo\0": 4 as well? @@ +3,5 @@ > +assertEq(newExternalString("abc\0def\u1234"), "abc\0def\u1234"); > + > +var o = {foo: 2}; > +var ext = newExternalString("foo"); > +assertEq(o[ext], 2); And then var ext2 = newExternalString("foo\0"); assertEq(o[ext2], 4); @@ +10,5 @@ > + > +// Make sure ensureFlat does the right thing for external strings. > +ext = newExternalString("abc\0defg\0"); > +assertEq(ensureFlatString(ext), "abc\0defg\0"); > +assertEq(ensureFlatString(ext), "abc\0defg\0"); We probably should test ensureFlatString on all the other various forms of string as well, for completeness. static bool RepresentativeStringArray(JSContext* cx, unsigned argc, JS::Value* vp) { JS::CallArgs args = JS::CallArgsFromVp(argc, vp); JS::Rooted<JSObject*> array(cx, JS_NewArrayObject(cx, JSString::REPRESENTATION_COUNT)); if (!array) return false; if (!JSString::fillWithRepresentatives(cx, array)) return false; args.rval().setObject(*array); return true; } and then pass all the elements of that string into the function. (This function seems like it'd be useful in other places, hence worth writing/adding now.) The patch looks correct, so feel free to work with it as-is -- but I'd like to see full representation-testing in a fast followup patch. ::: js/src/vm/String-inl.h @@ +403,5 @@ > JSExternalString::finalize(js::FreeOp* fop) > { > + if (!JSString::isExternal()) { > + // This started out as an external string, but was turned into a > + // non-external string by JSExternalString::ensureFlat. Is any of this if-block necessary? This looks vestigial; you only set the flat-bit at the tail end of JSExternalString::ensureFlat.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15) > We probably should test ensureFlatString on all the other various forms of > string as well, for completeness. > > <snip> > > but I'd like to see full representation-testing in a fast followup patch. I ~never complain about review comments or follow-up requests, but this one seems unfair. I'm fixing an ancient bug related to external strings, I'm even exposing both external strings + ensureFlat to the shell and I'm writing tests for them. Adding a third shell function that returns an array of strings is pretty orthogonal to the bug we're fixing. Sure, maybe an excellent follow-up, but I don't get the urgency. Well, nevermind. I'll write a follow-up patch for it this week. > Is any of this if-block necessary? This looks vestigial; you only set the > flat-bit at the tail end of JSExternalString::ensureFlat. Unfortunately it's necessary, because external strings have their own AllocKind/arenas and the GC will call JSExternalString::finalize, even if we turned this external string into a flat string. I do think we should probably clean up all these string finalize methods, I'll file a follow-up bug for that.

(In reply to Jan de Mooij [:jandem] from comment #16) > I ~never complain about review comments or follow-up requests, but this one > seems unfair. We have a large set of possible string types, for a large set of possible flags-values. Any fiddling with this at all, is super-sensitive to a mistake in flag-matching, in JSString::* functions and (more worryingly) in the JITs that have to encode procedural flags-checks, and in a significant number of places. It's not at all inconceivable that a change in value of some bit or flag-mask could make one of these places subtly buggy. And of course the consequences would be typically severe. The changes all look correct, and that's the reason I'm not asking for full-string-type testing as a condition of landing a security fix that's also targeted at release branches (and incidentally probably couldn't land on them, for fear of possibly prematurely giving the game away). But I think there's ample reason to be concerned about this, and ad hoc testing of manually-constructed strings of the various types can't cut it. (I actually started to write out such testing as a requested addition. I abandoned when it became clear how fragile such construction was, how it would have architecture-specific inadequacies, how far it was from vm/String.h implementing the types so that subsequent string-encoding changes would make the testing stale, &c.) As to doing this specifically for ensureFlatString, that's somewhat fair. But given we're changing the flattening mechanics and implementation, it doesn't seem beyond the pale to want those changes fully tested on all string encodings. > Unfortunately it's necessary, because external strings have their own > AllocKind/arenas and the GC will call JSExternalString::finalize, even if we > turned this external string into a flat string. Huh. This very much deserves a comment to that effect (although possibly such comment should land at a delay after this bug is fixed in releases). > I do think we should probably clean up all these string finalize methods I haven't looked at this code hard/closely enough to have a sense what that might entail -- current system didn't seem obviously horrific to me, or at least not enough I had a suggestion to make. But anything that might simplify the necessarily-complex string setup is good.

As to the precise array-of-strings-of-types mechanism I suggested, on thinking harder about it it *might* not be the ideal mechanism. It could return an object with key-value mappings of name-of-string-type to string-of-type, which would be useful in allowing code to request a string of a particular type. It could take a callback and repeatedly invoke it for the different strings/types (so you don't have to write a loop yourself). Or maybe other things. Probably worth thinking a little harder than my off-the-cuff array idea, so we have something providing desirably-generalized utility. (Oh, and I can't remember any more if at GC time we do anything to linearize ropes in certain cases [like if the rope is super-deep, or its subcomponents are only referred to by the rope], or to convert smaller strings to inline strings to save space, or to convert a currently-two-byte string into one-byte, or anything like that. Such transformations could make it finickier to match test-writer intent in what is meant to be tested.)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15) > auto buf = js::MakeUnique<char16_t[]>(len); This doesn't compile because the specialization is explicitly deleted: http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/js/public/UniquePtr.h#53 Maybe I can use mozilla::MakeUnique directly? I don't see any code in SM that does that though. Given that it's a testing function, I'll just stick with what I had and what we use elsewhere.

[Security approval request comment] > How easily could an exploit be constructed based on the patch? It's exploitable if we have code that assumes flat JS strings are actually null-terminated. I didn't check this, but best to assume there is. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, or at least nothing that's not obvious from reading the code. > Which older supported branches are affected by this flaw? 52+ > If not all supported branches, which bug introduced the flaw? Bug 1324430. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should apply or be easy to backport. > How likely is this patch to cause regressions; how much testing does it need? Unlikely, having this in a few Nightlies should be sufficient.

Comment on attachment 8829381 [details] [diff] [review] Part 1 - Allow non-flat external strings sec-approval+ for trunk. Let's get this into Aurora and Beta as well so we don't ship the issue.

Try uncovered a problem with this: we can now call the external string finalizer outside GC, when flattening external strings, so we need to clear XPConnect's zone cache.

Comment on attachment 8829414 [details] [diff] [review] Part 2 - Add representativeStringArray Review of attachment 8829414 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/String.cpp @@ +1441,5 @@ > } > #endif > + > +static bool > +AppendString(JSContext* cx, HandleArrayObject array, uint32_t* index, HandleString s) If you make AppendString a lambda, it can capture the CheckString parameter (see last comment on this file). @@ +1527,5 @@ > + // External. Note that we currently only support TwoByte external strings. > + RootedString external1(cx), external2(cx); > + if (IsSame<CharT, char16_t>::value) { > + external1 = JS_NewExternalString(cx, (const char16_t*)chars, len, > + &RepresentativeExternalStringFinalizer); Indent is off by one. @@ +1538,5 @@ > + return false; > + } > + > + // Assert this at the end, to make sure it still holds after creating the > + // other strings. Could you assert this both by the string creation and at end? Maybe have the asserts in lambdas that can be called by string creation, and then at the end here. @@ +1542,5 @@ > + // other strings. > + > + MOZ_ASSERT(atom1->isAtom()); > + MOZ_ASSERT(atom2->isAtom()); > + MOZ_ASSERT(atom3->isAtom()); Hm, we probably should add super-specific character-type-agnostic test functions for all the things this function wants to test, at some point. (For example, an isNormalAtom function that, with inline/fat, fully partitions all atoms.) These asserts are much vaguer than the comments above them claim. @@ +1593,5 @@ > + // Assert the character encoding is correct. > + RootedValue v(cx); > + for (uint32_t i = 0; i < index; i++) { > + if (!JS_GetElement(cx, array, i, &v)) > + return false; The checks here seem a little like overkill to me -- would rather pass in to FillWithRepresentatives auto CheckTwoByte = [](JSString* str) { return str->hasTwoByteChars(); }; auto CheckLatin1 = [](JSString* str) { return str->hasLatin1Chars(); }; that could be invoked, say, by AppendString. I'm also not too much a fan of invoking all of JS_GetElement's complexity if we can help it (and with the lambda trick, I think we can avoid it).

Comment on attachment 8829817 [details] [diff] [review] Part 1.1 - Clear ZoneStringCache r=me, though I wonder whether we still need the sweep zone callback. I guess it could be a bit faster in practice...

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #26) > r=me, though I wonder whether we still need the sweep zone callback. I > guess it could be a bit faster in practice... I was wondering the same thing, but it probably matters for compacting GC: when we move an external string in memory, without calling its finalizer, we should clear the cache or else its (untraced and unrooted) JSString* pointer will be dangling.

Ah, that's worth adding as a code comment!

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #28) > Ah, that's worth adding as a code comment! Done, added a comment to ClearZoneCache.

Need to land part 2 still.

This patch adds 2 new testing functions: * newExternalString(string): returns a new string with the same characters and length, but this string is an "external" string. We've had external strings in the browser for a long time, but now we will have them in the shell too. * ensureFlatString(string): returns the string and ensures it's flat (null-terminated). This was available to the shell via other functions, but ensureFlatString is more explicit. I also added a jit-test that uses these functions, so some of the fuzzers could learn about them from that test.

Filed a GH issue to keep this on the radar for jsfunfuzz: https://github.com/MozillaSecurity/funfuzz/issues/77

Hello Jan, should we consider uplifting this fix to Beta52 and Aurora53? I noticed it while reviewing 52+ tracked bugs.

(In reply to Ritu Kothari (:ritu) from comment #35) > Hello Jan, should we consider uplifting this fix to Beta52 and Aurora53? I > noticed it while reviewing 52+ tracked bugs. i guess so, see comment #c23

(In reply to Ritu Kothari (:ritu) from comment #35) > Hello Jan, should we consider uplifting this fix to Beta52 and Aurora53? I > noticed it while reviewing 52+ tracked bugs. Yes it's on my list, but let's wait a few days to see if this works correctly in Nightly. Keeping the needinfo, I also need to land part 2 still.

Approval Request Comment [Feature/Bug causing the regression]: Bug 1324430. [User impact if declined]: Crashes, potential security issues. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: Not very risky. [Why is the change risky/not risky?]: It has been in Nightly for a while without causing any problems, patch is not trivial but also not super complicated. [String changes made/needed]: None.

See comment 38.

Comment on attachment 8831957 [details] [diff] [review] Patch for aurora fix string handling regression, aurora53+ Looks like we got three regressions so far from bug 1324430 :(

Comment on attachment 8831958 [details] [diff] [review] Patch for beta let's get this in 52.0b3 as well.

(In reply to Jan de Mooij [:jandem] from comment #32) > This patch adds 2 new testing functions: > > * newExternalString(string): returns a new string with the same characters > and length, but this string is an "external" string. We've had external > strings in the browser for a long time, but now we will have them in the > shell too. > > * ensureFlatString(string): returns the string and ensures it's flat > (null-terminated). This was available to the shell via other functions, but > ensureFlatString is more explicit. > > I also added a jit-test that uses these functions, so some of the fuzzers > could learn about them from that test. I also added those identifiers to the whitelist in LangFuzz.

https://hg.mozilla.org/releases/mozilla-aurora/rev/662e5af3a928 we still need part 2 at some point or ?

(In reply to Carsten Book [:Tomcat] from comment #43) > we still need part 2 at some point or ? I'll land it soon, but we don't need to backport it.

needs rebasing for beta Hunk #2 FAILED at 646 1 out of 4 hunks FAILED -- saving rejects to file js/src/vm/String.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh 1330593-b.patch

> Looks like we got three regressions so far from bug 1324430 :( Yes, that's what happens when you change invariants....

(In reply to Ryan VanderMeulen [:RyanVM] from comment #47) This time a version that actually builds. https://hg.mozilla.org/releases/mozilla-beta/rev/93b639dcd0c2

Pushed part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ec674fd3010ad151b0285e59d4ef7d0beeb67d

Build ID: 20170302120751 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0 Verified as fixed on Firefox RC build2, Firefox Nightly 54.0a1 on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x 64.

Removing leave-open keyword from resolved bugs, per :sylvestre.