We sort of did that in a way in bug 1324430, except we didn't fix all the places that depended on the invariant. Patch coming up, sort of.

This needs a good once-over, both in terms of overall concept and in terms of actual implementation. And I'd welcome explicit feedback on the Assign thing.

The other option is to back out the xpcom/string bits of bug 1324430, change XMLHttpRequestStringSnapshot::GetAsString to take a DOMString, not an nsAString, and add machinery to DOMString to share the buffer with this different length (not hard, since it already keeps buffer and length separate). We'd still need something on the way back from JS into C++ to avoid creating F_SHARED XPCOM strings that are not F_TERMINATED, or F_TERMINATED but without '\0' at the end, but it wouldn't be hard to add a quick check for that. This might be a safer change...

Sorry for the delay in responding to this. (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2) > The other option is to back out the xpcom/string bits of bug 1324430, change > XMLHttpRequestStringSnapshot::GetAsString to take a DOMString, not an > nsAString, and add machinery to DOMString to share the buffer with this > different length (not hard, since it already keeps buffer and length > separate). > > We'd still need something on the way back from JS into C++ to avoid creating > F_SHARED XPCOM strings that are not F_TERMINATED, or F_TERMINATED but > without '\0' at the end, but it wouldn't be hard to add a quick check for > that. > > This might be a safer change... "This" being what you're proposing in the comment, correct? Where would be the check go? DOMString::ToString seems like it would need a check, are there any other places? Putting the smarts in DOMString seems safer; I can imagine that a lot of code implicitly relies on ns*String objects being null-terminated, and if we suddenly change^Wretroactively declare that they might not be, I think we'll be putting out fires for a long time.

I am 100% confident that many callsites rely on ns*String.get() producing a correctly null-terminated string. I expect that they also rely on BeginReading to produce a null-terminated string.

> "This" being what you're proposing in the comment, correct? Yes. > Where would be the check go? We'd need it in DOMString::ToString, and probably any code guarded by XPCStringConvert::IsDOMString that then makes an XPCOM string out the stringbuffer. At the moment that's just the code in XPCConvert::JSData2Native, but bug 1330671 might add more. > I am 100% confident that many callsites rely on ns*String.get() producing a correctly null-terminated string I am too, but the real question is whether it's possible for a non-null-terminated value to flow to those callsites. For example, you can already Rebind() an nsString to a non-nullterminated buffer (though triggering assertions in the process). Anyway, I guess I'll go ahead and do the comment 2 approach.

Comment on attachment 8826296 [details] [diff] [review] F_SHARED no longer implies F_TERMINATED for XPCOM strings Review of attachment 8826296 [details] [diff] [review]: ----------------------------------------------------------------- Canceling this review, then, since it sounds like we're doing something else.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5) > > I am 100% confident that many callsites rely on ns*String.get() producing a correctly null-terminated string > > I am too, but the real question is whether it's possible for a > non-null-terminated value to flow to those callsites. For example, you can > already Rebind() an nsString to a non-nullterminated buffer (though > triggering assertions in the process). Can you ensure these strings never reach strings that are statically typed as a nsTString (rather than nsTSubstring), and thus never reach a class on which you can call a get() method? I'd feel somewhat more comfortable with this sort of thing if we had better assertions (or even opt runtime checks, eventually) to enforce the invariants that we're currently somewhat lacking. (Callers really shouldn't be assuming that BeginReading() returns a null-terminated buffer -- that's why there's an EndReading()! And why get() is on nsTString rather than nsTSubstring!)

> Can you ensure these strings never reach strings that are statically typed as a nsTString No, that is precisely the problem, and why I proposed the alternate approach. With the alternate approach as planned, we will be able to ensure such a thing, yes.

Resummarizing to indicate what we're really doing here.

We're going to need it because we're going to add a consumer that cannot in fact promise that its stringbuffer reference will outlive the DOMString.

Comment on attachment 8827783 [details] [diff] [review] part 1. Change various bits of DOMString code to work better when it has a stringbuffer which is effectively not null-terminated (in the sense that indexing into it at the DOMString's length doesn't yield '\0') Review of attachment 8827783 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the additional comments.

Comment on attachment 8827784 [details] [diff] [review] part 2. Fix XPCConvert::JSData2Native to not share an external string stringbuffer if it would create a non-null-terminated string Review of attachment 8827784 [details] [diff] [review]: ----------------------------------------------------------------- Can you add a patch to this bug that strengthens the assertion here: http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsSubstring.cpp#300 to be a MOZ_ASSERT (maybe even a MOZ_DIAGNOSTIC_ASSERT or MOZ_RELEASE_ASSERT), so we can catch bad cases earlier? Perhaps we should just move some of the logic you added in this patch and the previous one into that function, though I think that'd be tricky with the aMoveOwnership flag...

Comment on attachment 8827785 [details] [diff] [review] part 3. Add an 'owned stringbuffer' mode to DOMString Review of attachment 8827785 [details] [diff] [review]: ----------------------------------------------------------------- I think this seems reasonable. None of my comments are blockers for landing this, but the Variant one might be worth a followup. ::: dom/bindings/DOMString.h @@ +141,5 @@ > mLength = aLength; > } > > + // Like SetStringBuffer, but holds a reference to the nsStringBuffer. > + void SetEphemeralStringBuffer(nsStringBuffer* aStringBuffer, uint32_t aLength) My first instinct is to apply the "Ephemeral" modifier to the string buffer. That is, the string buffer can go away at any time, and I need to be careful about lifetimes. But that's not how this class works: the "SetOwned" methods are the unsafe ones, along with SetStringBuffer, and this one is the safe one. Am I off base for thinking that? @@ +143,5 @@ > > + // Like SetStringBuffer, but holds a reference to the nsStringBuffer. > + void SetEphemeralStringBuffer(nsStringBuffer* aStringBuffer, uint32_t aLength) > + { > + SetStringBuffer(aStringBuffer, aLength); Possibly a comment saying that we're relying on SetStringBuffer to ensure the state invariants rather than checking them ourselves? @@ +234,5 @@ > "documented above and enforced through " > "assertions") mStringBuffer; > uint32_t mLength; > bool mIsNull; > + bool mStringBufferOwned; I wonder if, whether trying to keep all the member variables in sync, we shouldn't just turn everything into some mozilla::Variant type (ala Rust enums), and let the type system help us out here.

Comment on attachment 8827786 [details] [diff] [review] part 4. Change XMLHttpRequest's responseText getter to take a DOMString, not an nsAString Review of attachment 8827786 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xhr/XMLHttpRequestString.cpp @@ +75,5 @@ > + return true; > + } > + > + // XXXbz Can we really reach this? I'd expect us to always have an > + // nsStringBuffer here. I guess we could reach here if we don't have any data? But I think we'd always create a new string buffer when appending, so we should hit the case above otherwise.

Comment on attachment 8827787 [details] [diff] [review] part 5. Back out the nsTSubstring changes we made in bug 1324430, because they can lead to non-null-terminated nsStrings Review of attachment 8827787 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!

(In reply to Nathan Froyd [:froydnj] from comment #19) > Thank you! ...and sorry for not thinking through the review in the other bug more carefully. :(

> Can you add a patch to this bug that strengthens the assertion here: Yes, assuming you meant https://dxr.mozilla.org/mozilla-central/rev/80eac484366ad881c6a10bf81e8d9b8f7a676c75/xpcom/string/nsSubstring.cpp#307 and similar for ACString. MOZ_DIAGNOSTIC_ASSERT makes sense. I'd rather avoid the possible cache effects of poking at the end of the stringbuffer unnecessarily in release builds. > Though I think that'd be tricky with the aMoveOwnership flag Well, we could just Release() if aMoveOwnership and we're not going to use the stringbuffer. In practice the only caller that passes aMoveOwnership very clearly null-terminates the buffer right before that point anyway. > Am I off base for thinking that? You're not. The names currently describe the _caller's_ relationship to the data. That is, SetOwnedString means the caller owns the string and promises to keep doing that. This confused me at first too; I initially named the new method SetOwnedStringBuffer, then realized that's not what "Owned" means in this API. I'm not super-happy about this; maybe we should rename all this stuff wholesale... I'm just not sure to what. If you have naming suggestions, I'm all ears. > Possibly a comment saying that we're relying on SetStringBuffer to ensure the state invariants Will do. > we shouldn't just turn everything into some mozilla::Variant type (ala Rust enums) Possibly. Filed bug 1332042. > I guess we could reach here if we don't have any data? Ah, good point. I'll update the comment.

Comment on attachment 8828060 [details] [diff] [review] part 6. Make the null-termination asserts in nsStringBuffer::ToString into diagnostic asserts Review of attachment 8828060 [details] [diff] [review]: ----------------------------------------------------------------- Great, thank you!

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1cc3dbae3b31 part 1. Change various bits of DOMString code to work better when it has a stringbuffer which is effectively not null-terminated (in the sense that indexing into it at the DOMString's length doesn't yield '\0'). r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/3afcfb43c465 part 2. Fix XPCConvert::JSData2Native to not share an external string stringbuffer if it would create a non-null-terminated string. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/04378bf08ef9 part 3. Add a "stringbuffer we own" mode to DOMString. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/22ae3a4fb412 part 4. Change XMLHttpRequest's responseText getter to take a DOMString, not an nsAString. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/1873ff6e9179 part 5. Back out the nsTSubstring changes we made in bug 1324430, because they can lead to non-null-terminated nsStrings. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/5f327e934dfe part 6. Make the null-termination asserts in nsStringBuffer::ToString into diagnostic asserts. r=froydnj

https://hg.mozilla.org/mozilla-central/rev/1cc3dbae3b31 https://hg.mozilla.org/mozilla-central/rev/3afcfb43c465 https://hg.mozilla.org/mozilla-central/rev/04378bf08ef9 https://hg.mozilla.org/mozilla-central/rev/22ae3a4fb412 https://hg.mozilla.org/mozilla-central/rev/5f327e934dfe

Boris, should we backport this to 52? Bug 1324430 was uplifted to Aurora, IIUC.

Ugh. Yes, this needs to be backported to 52. :(

Comment on attachment 8827783 [details] [diff] [review] part 1. Change various bits of DOMString code to work better when it has a stringbuffer which is effectively not null-terminated (in the sense that indexing into it at the DOMString's length doesn't yield '\0') Approval Request Comment [Feature/Bug causing the regression]: Bug 1324430 [User impact if declined]: Possible security issues due to strings being read past their end. [Is this code covered by automated tests?]: Generally, 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 that I'm aware of. [Is the change risky?]: Medium risk, I think. [Why is the change risky/not risky?]: we're changing string behavior and XHR behavior. Within that constraint, this is as safe as it gets, I think... [String changes made/needed]: None.

In terms of the JSExternalString, I assume you meant back out bug 1324430 from 52? We _could_ do that, at the cost of the memory regression it was fixing being reintroduced...

Track as regression in 52.

Comment on attachment 8827783 [details] [diff] [review] part 1. Change various bits of DOMString code to work better when it has a stringbuffer which is effectively not null-terminated (in the sense that indexing into it at the DOMString's length doesn't yield '\0') fix recent string handling regression, beta52+

Comment on attachment 8827784 [details] [diff] [review] part 2. Fix XPCConvert::JSData2Native to not share an external string stringbuffer if it would create a non-null-terminated string [Triage Comment]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ca3b97b5e42 https://hg.mozilla.org/releases/mozilla-beta/rev/8dcb11ae3b13 https://hg.mozilla.org/releases/mozilla-beta/rev/0f3d229ed368 https://hg.mozilla.org/releases/mozilla-beta/rev/cdaba011494a https://hg.mozilla.org/releases/mozilla-beta/rev/121ee6ad585e https://hg.mozilla.org/releases/mozilla-beta/rev/1522cb56a651

Setting qe-verify- per Comment 30.