Closed Bug 1330759 Opened 3 years ago Closed 3 years ago

Change the way XMLHttpRequests creates non-null-terminated external JS strings to make it impossible to create non-null-terminated nsStrings using the same mechanism

Categories

(Core :: String, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(6 files, 1 obsolete file)

3.29 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.76 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
6.26 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
10.10 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.94 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.83 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
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.
Attachment #8826296 - Flags: review?(nfroyd)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
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.
Attachment #8826296 - Flags: review?(nfroyd)
(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!)
Flags: needinfo?(bzbarsky)
> 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.
Flags: needinfo?(bzbarsky)
Resummarizing to indicate what we're really doing here.
Summary: Allow F_SHARED to not imply F_TERMINATED → Change the way XMLHttpRequests creates non-null-terminated external JS strings to make it impossible to create non-null-terminated nsStrings using the same mechanism
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.
Attachment #8827785 - Flags: review?(nfroyd)
Attachment #8826296 - Attachment is obsolete: true
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.
Attachment #8827783 - Flags: review?(nfroyd) → review+
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...
Attachment #8827784 - Flags: review?(nfroyd) → review+
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.
Attachment #8827785 - Flags: review?(nfroyd) → review+
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.
Attachment #8827786 - Flags: review?(nfroyd) → review+
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!
Attachment #8827787 - Flags: review?(nfroyd) → review+
(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!
Attachment #8828060 - Flags: review?(nfroyd) → review+
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/1873ff6e9179
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Boris, should we backport this to 52? Bug 1324430 was uplifted to Aurora, IIUC.
Flags: needinfo?(bzbarsky)
Ugh.  Yes, this needs to be backported to 52.  :(
Flags: needinfo?(bzbarsky)
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.
Attachment #8827783 - Flags: approval-mozilla-aurora?
Attachment #8827784 - Flags: approval-mozilla-aurora?
Attachment #8827785 - Flags: approval-mozilla-aurora?
Attachment #8827786 - Flags: approval-mozilla-aurora?
Attachment #8827787 - Flags: approval-mozilla-aurora?
Attachment #8828060 - Flags: approval-mozilla-aurora?
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+
Attachment #8827783 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
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]
Attachment #8827784 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Attachment #8827785 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Attachment #8827786 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Attachment #8827787 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Attachment #8828060 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Setting qe-verify- per Comment 30.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.