add assertions about string class null-termination invariants

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(7 attachments, 1 obsolete attachment)

12.55 KB, patch
erahm
: review+
Details | Diff | Splinter Review
2.40 KB, patch
erahm
: review+
Details | Diff | Splinter Review
9.70 KB, patch
erahm
: review+
Details | Diff | Splinter Review
5.06 KB, patch
Details | Diff | Splinter Review
3.19 KB, patch
Details | Diff | Splinter Review
4.69 KB, patch
erahm
: review+
Details | Diff | Splinter Review
1002 bytes, patch
erahm
: review+
Details | Diff | Splinter Review
I have some patches to add some assertions to the string classes about null-termination invariants.  More details coming in the patch descriptions...
This is needed for patch 4.

MozReview-Commit-ID: 5ikQFIL9O0i
Attachment #8886693 - Flags: review?(erahm)
This is needed for patch 4.

MozReview-Commit-ID: 7xKQv37cy5k
Attachment #8886694 - Flags: review?(erahm)
This is needed for patch 4.

MozReview-Commit-ID: 4BFlTtQdtoN
Attachment #8886695 - Flags: review?(erahm)
I actually couldn't figure out a way to trigger this assertion with the
current string code without doing invalid casts, but there are things we
may want to add to the string code in the future that might risk hitting
this (e.g., move constructors, promoting various rebind methods to
nsA[C]String), so I think it's worth asserting.

MozReview-Commit-ID: 4R0dYuTfrFW
Attachment #8886696 - Flags: review?(erahm)
This avoids triggering the assertions added in patch 6.

MozReview-Commit-ID: 9000zY0FDId
While it would be nice to assert this in AssertValid() (added in the
previous patch), this is difficult because of what MutatePrep does:
https://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/xpcom/string/nsTSubstring.cpp#152,165,177-178

MozReview-Commit-ID: 12sQLHtJ4xc
I decided I don't actually want to land the last two patches, given how awkward it is to use the string API to do what the one caller that needed fixing wants to do.  I think we should:
 * first, make that easier, and then
 * second, try to do the stronger assertions that I mention in patch 6 (i.e., in the same place as the code in patch 4)
Comment on attachment 8886693 [details] [diff] [review]
patch 1 - Add ClassFlags::NULL_TERMINATED to strings that require null-termination

Review of attachment 8886693 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/string/nsTString.h
@@ +466,5 @@
>    // allow subclasses to initialize fields directly
>    nsTString_CharT(char_type* aData, size_type aLength, DataFlags aDataFlags,
>                    ClassFlags aClassFlags)
> +    : substring_type(aData, aLength, aDataFlags,
> +                     aClassFlags | ClassFlags::NULL_TERMINATED)

This one seems a little sketchy, it would be nice if there were a way to make sure we don't goof up and make a subclass that expects to *not* be null terminated.

I'd almost prefer pushing the flag to the subclass and asserting they set NULL_TERMINATED, but I can see how that would require a lot more changes.
Attachment #8886693 - Flags: review?(erahm) → review+
Attachment #8886694 - Flags: review?(erahm) → review+
Comment on attachment 8886695 [details] [diff] [review]
patch 3 - Encapsulate setting mData/mLength/mDataFlags in a new method

Review of attachment 8886695 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/string/nsTDependentString.cpp
@@ +27,5 @@
>      startPos = strLength;
>    }
>  
> +  char_type* newData =
> +    const_cast<char_type*>(static_cast<const char_type*>(str.Data())) + startPos;

Just curious, why do we need the static_cast? char16ptr_t weirdness? It would be nice to drop it if we don't need it anymore.
Attachment #8886695 - Flags: review?(erahm) → review+
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #9)
> This one seems a little sketchy, it would be nice if there were a way to
> make sure we don't goof up and make a subclass that expects to *not* be null
> terminated.

Any subclass that isn't null-terminated should derive from nsTSubstring rather than nsTString.  That's the key static type distinction between the two.

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #10)
> Just curious, why do we need the static_cast? char16ptr_t weirdness? It
> would be nice to drop it if we don't need it anymore.

I noticed that too, and concluded from https://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/mfbt/Char16.h#20-21 that we probably still needed it.
Comment on attachment 8886696 [details] [diff] [review]
patch 4 - Assert that strings whose static type requires a null-terminated buffer aren't assign a non-null-terminated buffer

Review of attachment 8886696 [details] [diff] [review]:
-----------------------------------------------------------------

This seems okay, a few notes below.

::: xpcom/string/nsTSubstring.h
@@ -304,5 @@
>  
>  protected:
>    nsTStringRepr_CharT() = delete; // Never instantiate directly
>  
> -  constexpr

This could still be constexpr in release builds right?

@@ +1093,5 @@
>    {
>      mData = char_traits::sEmptyBuffer;
>      mLength = 0;
>      mDataFlags = DataFlags::TERMINATED;
> +    AssertValid();

Given the assertion we know this is always true right?
Attachment #8886696 - Flags: review?(erahm) → review+
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #12)
> > -  constexpr
> 
> This could still be constexpr in release builds right?

I guess it could.  Is there a reason it's valuable for it to be constexpr?

> >      mData = char_traits::sEmptyBuffer;
> >      mLength = 0;
> >      mDataFlags = DataFlags::TERMINATED;
> > +    AssertValid();
> 
> Given the assertion we know this is always true right?

Yeah, but I decided I wanted it there anyway for future additions to AssertValid().
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #11)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #9)
> > This one seems a little sketchy, it would be nice if there were a way to
> > make sure we don't goof up and make a subclass that expects to *not* be null
> > terminated.
> 
> Any subclass that isn't null-terminated should derive from nsTSubstring
> rather than nsTString.  That's the key static type distinction between the
> two.

Yeah I understand, but my thought was it would be nice if we could statically enforce / assert that fact to avoid future issues. I guess that can just be "you need to understand this stuff" and code review from string person should catch it.

> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #10)
> > Just curious, why do we need the static_cast? char16ptr_t weirdness? It
> > would be nice to drop it if we don't need it anymore.
> 
> I noticed that too, and concluded from
> https://searchfox.org/mozilla-central/rev/
> cbd628b085ac809bf5a536109e6288aa91cbdff0/mfbt/Char16.h#20-21 that we
> probably still needed it.

I was thinking the implicit conversion to |const char16_t*| [1] might handle it for us, but maybe not inside a const_cast?

[1] https://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/mfbt/Char16.h#51-54
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #13)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #12)
> > > -  constexpr
> > 
> > This could still be constexpr in release builds right?
> 
> I guess it could.  Is there a reason it's valuable for it to be constexpr?

David, you added a constexpr ctor in your nsTStringRepr class patch [1], is their a compelling reason to keep it constexpr? My guess is yes because (vaguely) it gives the compiler a chance to do some optimizations, but it would be nice to hear your reasoning.

[1] https://reviewboard.mozilla.org/r/119210/diff/3#index_header
Flags: needinfo?(dmajor)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #15)
> David, you added a constexpr ctor in your nsTStringRepr class patch [1], is
> their a compelling reason to keep it constexpr? My guess is yes because
> (vaguely) it gives the compiler a chance to do some optimizations, but it
> would be nice to hear your reasoning.
> 
> [1] https://reviewboard.mozilla.org/r/119210/diff/3#index_header

My first thought is that I must have added it to be extra-super-duper sure that global nsTLiteralStrings can be set up as just plain static data, without having to run constructors. But apparently I didn't add a constexpr constructor on nsTLiteralString itself, so I guess it's not a concern.

My second guess would be that I added it as a safety net so that we error out if you add a new field and accidentally omit it from the ctor, since constexpr requires all members to be initialized.
Flags: needinfo?(dmajor)
...and/or possibly to reinforce the fact that nsTStringRepr is (was) just a dumb POD with no logic.
OK, maybe it's best to move the AssertValid() into nsTString (and just skip adding an equivalent to nsTLiteralString), and post a separate patch to add the constexpr to the nsTLiteralString constructor?
I actually couldn't figure out a way to trigger this assertion with the
current string code without doing invalid casts, but there are things we
may want to add to the string code in the future that might risk hitting
this (e.g., move constructors, promoting various rebind methods to
nsA[C]String), so I think it's worth asserting.

MozReview-Commit-ID: 4R0dYuTfrFW
Attachment #8887721 - Flags: review?(erahm)
Attachment #8886696 - Attachment is obsolete: true
Attachment #8887721 - Flags: review?(erahm) → review+
Attachment #8887722 - Flags: review?(erahm) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/79bc740e42aaeb256202eef44335a563594a7b04
Bug 1381080 patch 1 - Add ClassFlags::NULL_TERMINATED to strings that require null-termination.  r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e88978e73540cdd60f7a0374ac74b8fcfe0925
Bug 1381080 patch 2 - Encapsulate setting to empty buffer in a new method.  r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/235ac09dfdb2412295ac98834fb111e9198b0474
Bug 1381080 patch 3 - Encapsulate setting mData/mLength/mDataFlags in a new method.  r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b76213c99cfcb05a53d381edd272cbc29e6fadf
Bug 1381080 patch 4 - Assert that strings whose static type requires a null-terminated buffer aren't assign a non-null-terminated buffer.  r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a663e876c3c2d76703368581b033f28495c10e1
Bug 1381080 patch 7 - Mark nsLiteral[C]String constructor as constexpr.  r=erahm
You need to log in before you can comment on or make changes to this bug.