Closed
Bug 1340577
Opened 7 years ago
Closed 7 years ago
release-assert |begin| <= |end| when passed to Substring and similar APIs
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
Attachments
(1 file, 2 obsolete files)
13.97 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
Been discovered recently that constructing e.g. nsDependeCSubstring(begin, end) will happily accept begin > end. That leads to overflow of the length property constructing a string that starts at an arbitrary position having a ~2^32 length and allowing consumers, probably even content, to access any memory after the |end| marker. This probably doesn't happen over the code base, but better to add a proper hard-failing check we never pass begin > end to any APIs that accept it and may construct such broken strings.
Assignee | ||
Comment 1•7 years ago
|
||
All I could find in /xpcom/*String*.h https://treeherder.mozilla.org/#/jobs?repo=try&revision=9943fdadf45b8c383e0b6c77b2278d71d36fccc2
Assignee | ||
Comment 2•7 years ago
|
||
Bit better try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05437466e352c84e0dafe43685d4a9536037aa02
Comment 3•7 years ago
|
||
Comment on attachment 8838641 [details] [diff] [review] v1 Review of attachment 8838641 [details] [diff] [review]: ----------------------------------------------------------------- The commit message probably needs to be tweaked, since the fix here is a little more general than just modifying Substring. Some changes I think we should make are noted below. ::: xpcom/glue/nsStringAPI.h @@ +1442,5 @@ > > inline const nsDependentSubstring > Substring(const char16_t* aStart, const char16_t* aEnd) > { > + MOZ_RELEASE_ASSERT(aStart <= aEnd, "Overflow!"); If we're going to add release assertions to these Substring overloads, we should move them out of line into nsStringAPI.cpp. ::: xpcom/string/nsTDependentString.h @@ +31,5 @@ > nsTDependentString_CharT(const char_type* aStart, const char_type* aEnd) > : string_type(const_cast<char_type*>(aStart), > uint32_t(aEnd - aStart), F_TERMINATED) > { > + MOZ_RELEASE_ASSERT(aStart <= aEnd, "Overflow!"); Likewise, these changes should go in nsTDependentString.cpp. ::: xpcom/string/nsTDependentSubstring.h @@ +30,5 @@ > void Rebind(const char_type* aData, size_type aLength); > > void Rebind(const char_type* aStart, const char_type* aEnd) > { > + MOZ_RELEASE_ASSERT(aStart <= aEnd, "Overflow!"); Likewise, these changes should go in nsTDependentSubstring.cpp. ::: xpcom/string/nsTSubstring.h @@ +272,5 @@ > } > > char_type First() const > { > + MOZ_RELEASE_ASSERT(mLength > 0, "|First()| called on an empty string"); Likewise, these functions should be out-of-lined.
Attachment #8838641 -
Flags: review?(nfroyd) → review-
Assignee | ||
Comment 4•7 years ago
|
||
Everything carrying release asserts moved to respective cpps. already on top of one found blocking bug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e072bfb14d1d64c3f14ddfb8d003144868131a39
Attachment #8838641 -
Attachment is obsolete: true
Attachment #8838871 -
Flags: review?(nfroyd)
Assignee | ||
Comment 5•7 years ago
|
||
And I'll update the commit message before landing.
Assignee | ||
Comment 6•7 years ago
|
||
Try based on a greener changeset: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67e09780abf6702ede43165133bc4d9fe066247d
Comment 7•7 years ago
|
||
Comment on attachment 8838871 [details] [diff] [review] v2 Review of attachment 8838871 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8838871 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8838871 -
Attachment is obsolete: true
Attachment #8839628 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e552501cd1f Add release-grade assertions to various XPCOM string API implementations to avoid input causing an overflow. r=froydnj
Keywords: checkin-needed
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3e552501cd1f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•