Skip to content

Commit

Permalink
AK+LibJS: Handle NaN-boxing pointers on AArch64
Browse files Browse the repository at this point in the history
JS::Value stores 48 bit pointers to separately allocated objects in its
payload. On x86-64, canonical addresses have their top 16 bits set to
the same value as bit 47, effectively meaning that the value has to be
sign-extended to get the pointer. AArch64, however, expects the topmost
bits to be all zeros.

This commit gates sign extension behind `#if ARCH(X86_64)`, and adds an
`#error` for unsupported architectures, so that we do not forget to
think about pointer handling when porting to a new architecture.

Fixes SerenityOS#15290
Fixes LadybirdBrowser/ancient-history#56
  • Loading branch information
BertalanD authored and awesomekling committed Sep 21, 2022
1 parent 62fed2a commit 2b69af2
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 24 deletions.
6 changes: 6 additions & 0 deletions AK/Platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
# define AK_ARCH_AARCH64 1
#endif

#if (defined(__SIZEOF_POINTER__) && __SIZEOF_POINTER__ == 8) || defined(_WIN64)
# define AK_ARCH_64_BIT
#else
# define AK_ARCH_32_BIT
#endif

#if defined(__APPLE__) && defined(__MACH__)
# define AK_OS_MACOS
# define AK_OS_BSD_GENERIC
Expand Down
30 changes: 16 additions & 14 deletions Tests/LibJS/test-value-js.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,25 +39,18 @@ TEST_NULLPTR_INPUT(Accessor);

#undef TEST_NULLPTR_INPUT

// Unfortunately we don't have a way to get the pointer without it being dereferenced
// so we just use the same logic, this is dangerous if Value is ever changed!
static u64 extract_pointer(u64 ptr)
{
return (u64)(((i64)(ptr << 16)) >> 16);
}

TEST_CASE(valid_pointer_in_gives_same_pointer_out)
{
if (sizeof(void*) < sizeof(double))
return;

#define EXPECT_POINTER_TO_SURVIVE(input) \
{ \
JS::Value value(reinterpret_cast<Object*>(static_cast<u64>(input))); \
EXPECT(value.is_object()); \
EXPECT(!value.is_null()); \
auto extracted_pointer = extract_pointer(value.encoded()); \
EXPECT_EQ(static_cast<u64>(input), extracted_pointer); \
#define EXPECT_POINTER_TO_SURVIVE(input) \
{ \
JS::Value value(reinterpret_cast<Object*>(static_cast<u64>(input))); \
EXPECT(value.is_object()); \
EXPECT(!value.is_null()); \
auto extracted_pointer = JS::Value::extract_pointer_bits(value.encoded()); \
EXPECT_EQ(static_cast<u64>(input), extracted_pointer); \
}

EXPECT_POINTER_TO_SURVIVE(0x1);
Expand All @@ -66,9 +59,18 @@ TEST_CASE(valid_pointer_in_gives_same_pointer_out)
EXPECT_POINTER_TO_SURVIVE(0x00007fffffffffff);
EXPECT_POINTER_TO_SURVIVE(0x0000700000000000);
EXPECT_POINTER_TO_SURVIVE(0x0000100000000000);

#if ARCH(X86_64)
// On x86-64, the top 16 bits of pointers are equal to bit 47.
EXPECT_POINTER_TO_SURVIVE(0xffff800000000000);
EXPECT_POINTER_TO_SURVIVE(0xffff800000000001);
EXPECT_POINTER_TO_SURVIVE(0xffff800000000010);
#elif ARCH(AARCH64)
// ... but they should contain zeroes on AArch64.
EXPECT_POINTER_TO_SURVIVE(0x0000800000000000);
EXPECT_POINTER_TO_SURVIVE(0x0000800000000001);
EXPECT_POINTER_TO_SURVIVE(0x0000800000000010);
#endif

#undef EXPECT_POINTER_TO_SURVIVE
}
Expand Down
2 changes: 1 addition & 1 deletion Userland/Libraries/LibJS/Heap/Heap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ __attribute__((no_sanitize("address"))) void Heap::gather_conservative_roots(Has
// match any pointer-backed tag, in that case we have to extract the pointer to its
// canonical form and add that as a possible pointer.
if ((data & SHIFTED_IS_CELL_PATTERN) == SHIFTED_IS_CELL_PATTERN)
possible_pointers.set((u64)(((i64)data << 16) >> 16));
possible_pointers.set(Value::extract_pointer_bits(data));
else
possible_pointers.set(data);
} else {
Expand Down
28 changes: 19 additions & 9 deletions Userland/Libraries/LibJS/Runtime/Value.h
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,24 @@ class Value {
template<typename... Args>
[[nodiscard]] ALWAYS_INLINE ThrowCompletionOr<Value> invoke(VM&, PropertyKey const& property_key, Args... args);

static constexpr FlatPtr extract_pointer_bits(u64 encoded)
{
#ifdef AK_ARCH_32_BIT
// For 32-bit system the pointer fully fits so we can just return it directly.
static_assert(sizeof(void*) == sizeof(u32));
return static_cast<FlatPtr>(encoded & 0xffff'ffff);
#elif ARCH(X86_64)
// For x86_64 the top 16 bits should be sign extending the "real" top bit (47th).
// So first shift the top 16 bits away then using the right shift it sign extends the top 16 bits.
return static_cast<FlatPtr>((static_cast<i64>(encoded << 16)) >> 16);
#elif ARCH(AARCH64)
// For AArch64 the top 16 bits of the pointer should be zero.
return static_cast<FlatPtr>(encoded & 0xffff'ffff'ffffULL);
#else
# error "Unknown architecture. Don't know whether pointers need to be sign-extended."
#endif
}

private:
Value(u64 tag, u64 val)
{
Expand Down Expand Up @@ -444,15 +462,7 @@ class Value {
PointerType* extract_pointer() const
{
VERIFY(is_cell());

// For 32-bit system the pointer fully fits so we can just return it directly.
if constexpr (sizeof(PointerType*) < sizeof(u64))
return reinterpret_cast<PointerType*>(static_cast<u32>(m_value.encoded & 0xffffffff));

// For x86_64 the top 16 bits should be sign extending the "real" top bit (47th).
// So first shift the top 16 bits away then using the right shift it sign extends the top 16 bits.
u64 ptr_val = (u64)(((i64)(m_value.encoded << 16)) >> 16);
return reinterpret_cast<PointerType*>(ptr_val);
return reinterpret_cast<PointerType*>(extract_pointer_bits(m_value.encoded));
}

[[nodiscard]] ThrowCompletionOr<Value> invoke_internal(VM&, PropertyKey const&, Optional<MarkedVector<Value>> arguments);
Expand Down

0 comments on commit 2b69af2

Please sign in to comment.