Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Core/Node-API/Source/js_native_api_chakra.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2244,7 +2244,13 @@ napi_status napi_create_dataview(napi_env env,
&unused,
&bufferLength));

if (byte_length + byte_offset > bufferLength) {
// bufferLength is 32-bit; byte_offset and byte_length are caller-supplied
// size_t values. Validate each against the buffer size without adding them
// (byte_offset + byte_length could overflow size_t and wrap past the check,
// after which the values would be truncated to 32-bit for JsCreateDataView
// while the original 64-bit values get stored in DataViewInfo and later
// handed back by napi_get_dataview_info, enabling an out-of-bounds access).
if (byte_offset > bufferLength || byte_length > bufferLength - byte_offset) {
napi_throw_range_error(
env,
"ERR_NAPI_INVALID_DATAVIEW_ARGS",
Expand Down
8 changes: 7 additions & 1 deletion Core/Node-API/Source/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3185,7 +3185,13 @@ napi_status NAPI_CDECL napi_create_dataview(napi_env env,
RETURN_STATUS_IF_FALSE(env, value->IsArrayBuffer(), napi_invalid_arg);

v8::Local<v8::ArrayBuffer> buffer = value.As<v8::ArrayBuffer>();
if (byte_length + byte_offset > buffer->ByteLength()) {
// [BABYLON-NATIVE-ADDITION]: overflow-safe bounds check; not in Node.js upstream
// byte_offset and byte_length are caller-supplied size_t values; computing
Comment thread
bkaradzic-microsoft marked this conversation as resolved.
// byte_length + byte_offset can overflow and wrap past the buffer size, which
// would slip an out-of-range request through and trip a fatal CHECK inside
// v8::DataView::New. Validate each against the buffer size without adding them.
if (byte_offset > buffer->ByteLength() ||
byte_length > buffer->ByteLength() - byte_offset) {
napi_throw_range_error(env,
"ERR_NAPI_INVALID_DATAVIEW_ARGS",
"byte_offset + byte_length should be less than or "
Expand Down
6 changes: 6 additions & 0 deletions Tests/UnitTests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ endif()
add_executable(UnitTests ${SOURCES} ${SCRIPTS} ${TYPE_SCRIPTS} ${ASSETS})
target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}")

# The V8JSI Node-API shim does not implement napi_create_dataview, so the
# CreateDataViewRejectsOverflowingRange test is compiled out on that backend.
if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI")
target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI)
endif()

target_link_libraries(UnitTests
PRIVATE AppRuntime
PRIVATE Console
Expand Down
73 changes: 73 additions & 0 deletions Tests/UnitTests/Shared/Shared.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <arcana/threading/blocking_concurrent_queue.h>
#include <atomic>
#include <chrono>
#include <cstdint>
#include <future>
#include <iostream>
#include <thread>
Expand Down Expand Up @@ -276,6 +277,78 @@ TEST(AppRuntime, DestroyDoesNotDeadlock)
testThread.join();
}

// The V8JSI Node-API shim does not implement napi_create_dataview /
// napi_get_dataview_info (its DataView::New throws "TODO"), so this native test
// only builds on the Chakra, V8, and JavaScriptCore backends. The size_t-width
// guard is required because the overflow scenario below needs a 64-bit size_t.
#if (SIZE_MAX > 0xFFFFFFFFu) && !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI)
TEST(NodeApi, CreateDataViewRejectsOverflowingRange)
{
// Regression: napi_create_dataview must reject a (byte_offset, byte_length)
// pair whose sum overflows size_t. The pre-fix code performed an unchecked
// `byte_offset + byte_length > bufferLength` comparison; with the inputs
// below the 64-bit sum wraps to 8 and slips past it. It then truncated the
// values to 32-bit (offset -> 0, length -> 8) and created a valid 8-byte
// DataView, but stored the ORIGINAL 64-bit offset/length in DataViewInfo,
// which napi_get_dataview_info hands back alongside the small real buffer --
// an out-of-bounds access primitive. This path is not reachable from JS
// `new DataView`, so it is covered natively here. The scenario requires a
// 64-bit size_t (where the 32-bit truncation diverged from the stored value),
// hence the size_t-width guard.
Babylon::AppRuntime runtime{};

std::promise<bool> overflowSafe;
std::promise<bool> validAccepted;

runtime.Dispatch([&overflowSafe, &validAccepted](Napi::Env env) {
napi_env nenv{env};

Napi::ArrayBuffer arrayBuffer{Napi::ArrayBuffer::New(env, 16)};
napi_value arrayBufferValue{arrayBuffer};

// Low 32 bits are individually valid for the 16-byte buffer (offset 0,
// length 8), but the full 64-bit values are enormous and their sum wraps
// around size_t to 8.
const size_t hugeOffset{0xFFFFFFFF00000000ull};
const size_t hugeLength{0x0000000100000008ull};

napi_value result{nullptr};
napi_status status{napi_create_dataview(nenv, hugeLength, arrayBufferValue, hugeOffset, &result)};

bool safe;
if (status != napi_ok || result == nullptr)
{
// Fixed path: the out-of-range request is rejected outright.
safe = true;
}
else
{
// If creation unexpectedly succeeds, the reported extents must still
// lie within the 16-byte backing buffer (i.e. not the raw 64-bit
// inputs). The pre-fix code reported the huge stored values here.
size_t reportedLength{0};
size_t reportedOffset{0};
void* data{nullptr};
napi_get_dataview_info(nenv, result, &reportedLength, &data, nullptr, &reportedOffset);
safe = reportedOffset <= 16 && reportedLength <= 16 && reportedOffset + reportedLength <= 16;
}

// Clear any pending range error so it doesn't surface as an unhandled error.
napi_value pendingException{nullptr};
napi_get_and_clear_last_exception(nenv, &pendingException);
overflowSafe.set_value(safe);

// A legitimate offset/length pair must still succeed.
napi_value validResult{nullptr};
napi_status validStatus{napi_create_dataview(nenv, 8, arrayBufferValue, 4, &validResult)};
validAccepted.set_value(validStatus == napi_ok && validResult != nullptr);
});

EXPECT_TRUE(overflowSafe.get_future().get());
EXPECT_TRUE(validAccepted.get_future().get());
}
#endif

int RunTests()
{
testing::InitGoogleTest();
Expand Down
Loading