From d4736d17aed3ea0f856c1237986d553a6616c6ca Mon Sep 17 00:00:00 2001 From: davidot Date: Wed, 7 Sep 2022 01:14:23 +0200 Subject: [PATCH] LibJS: Allow negative pointers in Value Also ensure that all a nullptr input gives null object and you don't accidentally dereference a nullptr. --- Meta/Lagom/CMakeLists.txt | 1 + Tests/LibJS/CMakeLists.txt | 3 + Tests/LibJS/test-value-js.cpp | 113 +++++++++++++++++++++++ Userland/Libraries/LibJS/Runtime/Value.h | 23 +++-- 4 files changed, 132 insertions(+), 8 deletions(-) create mode 100644 Tests/LibJS/test-value-js.cpp diff --git a/Meta/Lagom/CMakeLists.txt b/Meta/Lagom/CMakeLists.txt index fa782c16dd1..7c7fad48fb3 100644 --- a/Meta/Lagom/CMakeLists.txt +++ b/Meta/Lagom/CMakeLists.txt @@ -759,6 +759,7 @@ if (BUILD_LAGOM) # Extra tests from Tests/LibJS lagom_test(../../Tests/LibJS/test-invalid-unicode-js.cpp LIBS LibJS) lagom_test(../../Tests/LibJS/test-bytecode-js.cpp LIBS LibJS) + lagom_test(../../Tests/LibJS/test-value-js.cpp LIBS LibJS) # Spreadsheet add_executable(test-spreadsheet_lagom diff --git a/Tests/LibJS/CMakeLists.txt b/Tests/LibJS/CMakeLists.txt index 8b62b1cfaa6..ce684ced41d 100644 --- a/Tests/LibJS/CMakeLists.txt +++ b/Tests/LibJS/CMakeLists.txt @@ -8,3 +8,6 @@ link_with_locale_data(test-invalid-unicode-js) serenity_test(test-bytecode-js.cpp LibJS LIBS LibJS) link_with_locale_data(test-bytecode-js) + +serenity_test(test-value-js.cpp LibJS LIBS LibJS) +link_with_locale_data(test-value-js) diff --git a/Tests/LibJS/test-value-js.cpp b/Tests/LibJS/test-value-js.cpp new file mode 100644 index 00000000000..2b0fbdd9599 --- /dev/null +++ b/Tests/LibJS/test-value-js.cpp @@ -0,0 +1,113 @@ +/* + * Copyright (c) 2022, David Tuin + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include + +using namespace JS; + +template +static void test_nullptr_input() +{ + Type* ptr = nullptr; + JS::Value val { ptr }; + EXPECT(val.is_null()); + EXPECT(!val.is_object()); + EXPECT(!val.is_string()); + EXPECT(!val.is_bigint()); + EXPECT(!val.is_symbol()); + EXPECT(!val.is_accessor()); + EXPECT(!val.is_cell()); + EXPECT(!val.is_number()); + EXPECT(!val.is_undefined()); +} + +#define TEST_NULLPTR_INPUT(type) \ + TEST_CASE(value_nullptr_input_##type) \ + { \ + test_nullptr_input(); \ + } + +TEST_NULLPTR_INPUT(Object); +TEST_NULLPTR_INPUT(PrimitiveString); +TEST_NULLPTR_INPUT(Symbol); +TEST_NULLPTR_INPUT(BigInt); +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(static_cast(input))); \ + EXPECT(value.is_object()); \ + EXPECT(!value.is_null()); \ + auto extracted_pointer = extract_pointer(value.encoded()); \ + EXPECT_EQ(static_cast(input), extracted_pointer); \ + } + + EXPECT_POINTER_TO_SURVIVE(0x1); + EXPECT_POINTER_TO_SURVIVE(0x10); + EXPECT_POINTER_TO_SURVIVE(0x100); + EXPECT_POINTER_TO_SURVIVE(0x00007fffffffffff); + EXPECT_POINTER_TO_SURVIVE(0x0000700000000000); + EXPECT_POINTER_TO_SURVIVE(0x0000100000000000); + EXPECT_POINTER_TO_SURVIVE(0xffff800000000000); + EXPECT_POINTER_TO_SURVIVE(0xffff800000000001); + EXPECT_POINTER_TO_SURVIVE(0xffff800000000010); + +#undef EXPECT_POINTER_TO_SURVIVE +} + +TEST_CASE(non_canon_nans) +{ +#define EXPECT_TO_BE_NAN(input) \ + { \ + Value val { bit_cast(input) }; \ + EXPECT(val.is_nan()); \ + EXPECT(val.is_number()); \ + EXPECT(!val.is_integral_number()); \ + EXPECT(!val.is_finite_number()); \ + EXPECT(!val.is_infinity()); \ + EXPECT(!val.is_empty()); \ + EXPECT(!val.is_nullish()); \ + } + + EXPECT_TO_BE_NAN(CANON_NAN_BITS | 0x1); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | 0x10); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | (NULL_TAG << TAG_SHIFT)); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | (UNDEFINED_TAG << TAG_SHIFT)); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | (INT32_TAG << TAG_SHIFT) | 0x88); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | (OBJECT_TAG << TAG_SHIFT)); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | (OBJECT_TAG << TAG_SHIFT) | 0x1230); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | (STRING_TAG << TAG_SHIFT)); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | (STRING_TAG << TAG_SHIFT) | 0x1230); + + u64 sign_bit = 1ULL << 63; + + EXPECT_TO_BE_NAN(CANON_NAN_BITS | sign_bit | 0x1); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | sign_bit | 0x10); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | sign_bit | (NULL_TAG << TAG_SHIFT)); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | sign_bit | (UNDEFINED_TAG << TAG_SHIFT)); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | sign_bit | (INT32_TAG << TAG_SHIFT) | 0x88); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | sign_bit | (OBJECT_TAG << TAG_SHIFT)); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | sign_bit | (OBJECT_TAG << TAG_SHIFT) | 0x1230); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | sign_bit | (STRING_TAG << TAG_SHIFT)); + EXPECT_TO_BE_NAN(CANON_NAN_BITS | sign_bit | (STRING_TAG << TAG_SHIFT) | 0x1230); + +#undef EXPECT_TO_BE_NAN +} diff --git a/Userland/Libraries/LibJS/Runtime/Value.h b/Userland/Libraries/LibJS/Runtime/Value.h index 0923852d3e4..d53cbf3ecbc 100644 --- a/Userland/Libraries/LibJS/Runtime/Value.h +++ b/Userland/Libraries/LibJS/Runtime/Value.h @@ -251,7 +251,7 @@ public: } Value(Object const* object) - : Value(object ? (OBJECT_TAG << TAG_SHIFT) : (NULL_TAG << TAG_SHIFT), reinterpret_cast(object)) + : Value(OBJECT_TAG << TAG_SHIFT, reinterpret_cast(object)) { } @@ -401,7 +401,6 @@ public: private: Value(u64 tag, u64 val) { - VERIFY((tag & ~TAG_EXTRACTION) == 0); VERIFY(!(tag & val)); m_value.encoded = tag | val; } @@ -409,16 +408,24 @@ private: template Value(u64 tag, PointerType const* ptr) { - VERIFY((tag & ~TAG_EXTRACTION) == 0); - VERIFY((tag & TAG_EXTRACTION) != 0); - // Cell tag bit must be set or this is a nullptr - VERIFY((tag & 0x8000000000000000ul) == 0x8000000000000000ul || !ptr); + if (!ptr) { + // Make sure all nullptrs are null + m_value.tag = NULL_TAG; + return; + } + + VERIFY((tag & 0x8000000000000000ul) == 0x8000000000000000ul); if constexpr (sizeof(PointerType*) < sizeof(u64)) { m_value.encoded = tag | reinterpret_cast(ptr); } else { - VERIFY(!(reinterpret_cast(ptr) & TAG_EXTRACTION)); - m_value.encoded = tag | reinterpret_cast(ptr); + // NOTE: Pointers in x86-64 use just 48 bits however are supposed to be + // sign extended up from the 47th bit. + // This means that all bits above the 47th should be the same as + // the 47th. When storing a pointer we thus drop the top 16 bits as + // we can recover it when extracting the pointer again. + // See also: Value::extract_pointer. + m_value.encoded = tag | (reinterpret_cast(ptr) & 0x0000ffffffffffffULL); } }