Skip to content

Commit

Permalink
REGRESSION(255600@main): [ iOS ] ASSERTION FAILED: threadLikeAssertio…
Browse files Browse the repository at this point in the history
…n.isCurrent() hit by TestWebKitAPI.WebKit.RequestRectForFoundTextRange every time

https://bugs.webkit.org/show_bug.cgi?id=246656
rdar://101266728

Reviewed by Darin Adler.

The deleted value of a `WebFoundTextRange` is currently constructed using copy
assignment. This is incorrect as the deleted value is intended to be written
into an uninitialized storage buffer. The use of copy assignment results in
the destruction of an uninitialized StringImpl, followed by an OOB write in
`StringImpl::deref()`.

Note that 255600@main is not the root cause of the issue. However, after
255600@main, the OOB write results in the address of the `completionHandler`
passed into `WebFoundTextRangeController::requestRectForFoundTextRange` being
modified. Consequently, `completionHandler.m_callThread.m_uid` is set to a
garbage value, and the assertion is hit as there is now a mismatch between the
calling thread and stored initialization thread.

To fix, use operator new to construct the deleted value. Additionally, use
`HashTableDeletedValue` when contructed the deleted AtomString for correctness.

* Source/WebKit/Shared/WebFoundTextRange.cpp:
(WebKit::WebFoundTextRange::operator== const):
(WebKit::WebFoundTextRange::decode):
* Source/WebKit/Shared/WebFoundTextRange.h:
(WTF::HashTraits<WebKit::WebFoundTextRange>::emptyValue):
(WTF::HashTraits<WebKit::WebFoundTextRange>::constructDeletedValue):
(WTF::HashTraits<WebKit::WebFoundTextRange>::isDeletedValue):
(WTF::HashTraits<WebKit::WebFoundTextRange>::deletedValue): Deleted.

Canonical link: https://commits.webkit.org/255832@main
  • Loading branch information
pxlcoder committed Oct 21, 2022
1 parent 8fc38a3 commit 9c64727
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 3 deletions.
8 changes: 8 additions & 0 deletions Source/WebKit/Shared/WebFoundTextRange.cpp
Expand Up @@ -32,6 +32,11 @@ namespace WebKit {

bool WebFoundTextRange::operator==(const WebFoundTextRange& other) const
{
if (frameIdentifier.isHashTableDeletedValue())
return other.frameIdentifier.isHashTableDeletedValue();
if (other.frameIdentifier.isHashTableDeletedValue())
return false;

return location == other.location
&& length == other.length
&& frameIdentifier == other.frameIdentifier
Expand Down Expand Up @@ -59,6 +64,9 @@ std::optional<WebFoundTextRange> WebFoundTextRange::decode(IPC::Decoder& decoder
if (!decoder.decode(result.frameIdentifier))
return std::nullopt;

if (result.frameIdentifier.isHashTableDeletedValue())

This comment has been minimized.

Copy link
@darinadler

darinadler Oct 21, 2022

Member

After you landed this, I realized that we don’t ever decode hash table deleted values when decoding an atom string, so this check is unnecessary dead code.

This comment has been minimized.

Copy link
@pxlcoder

pxlcoder Oct 21, 2022

Author Member

Will remove in a follow-up.

This comment has been minimized.

Copy link
@pxlcoder

pxlcoder Oct 21, 2022

Author Member
return std::nullopt;

if (!decoder.decode(result.order))
return std::nullopt;

Expand Down
5 changes: 2 additions & 3 deletions Source/WebKit/Shared/WebFoundTextRange.h
Expand Up @@ -55,10 +55,9 @@ struct WebFoundTextRangeHash {

template<> struct HashTraits<WebKit::WebFoundTextRange> : GenericHashTraits<WebKit::WebFoundTextRange> {
static WebKit::WebFoundTextRange emptyValue() { return { }; }
static WebKit::WebFoundTextRange deletedValue() { return { std::numeric_limits<uint64_t>::max(), 0, { }, 0 }; }

static void constructDeletedValue(WebKit::WebFoundTextRange& range) { range = deletedValue(); }
static bool isDeletedValue(const WebKit::WebFoundTextRange& range) { return range == deletedValue(); }
static void constructDeletedValue(WebKit::WebFoundTextRange& slot) { new (NotNull, &slot.frameIdentifier) AtomString { HashTableDeletedValue }; }
static bool isDeletedValue(const WebKit::WebFoundTextRange& range) { return range.frameIdentifier.isHashTableDeletedValue(); }
};

template<> struct DefaultHash<WebKit::WebFoundTextRange> : WebFoundTextRangeHash { };
Expand Down

0 comments on commit 9c64727

Please sign in to comment.