Skip to content

Commit 4e73539

Browse files
matthargettmatthargett
committed
Run the macOS NodeApiTests under sanitizers, which found another bug -- a use-after-free. Will check sanitizers under Android next
1 parent 300d73d commit 4e73539

13 files changed

Lines changed: 72 additions & 64 deletions

File tree

‎CMakeLists.txt‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ FetchContent_Declare(arcana.cpp
1313
GIT_REPOSITORY https://github.com/microsoft/arcana.cpp.git
1414
GIT_TAG 1a8a5d6e95413ed14b38a6ac9419048f9a9c8009)
1515
FetchContent_Declare(AndroidExtensions
16-
GIT_REPOSITORY https://github.com/bghgary/AndroidExtensions.git
17-
GIT_TAG 7d88a601fda9892791e7b4e994e375e049615688)
16+
GIT_REPOSITORY https://github.com/matthargett/AndroidExtensions.git
17+
GIT_TAG 4ffcc4ab149a02b9ee72622eee708b2753fd2093)
1818
FetchContent_Declare(asio
1919
GIT_REPOSITORY https://github.com/chriskohlhoff/asio.git
2020
GIT_TAG f693a3eb7fe72a5f19b975289afc4f437d373d9c)
@@ -72,6 +72,28 @@ option(JSRUNTIMEHOST_POLYFILL_URL "Include JsRuntimeHost Polyfill URL and URLSea
7272
option(JSRUNTIMEHOST_POLYFILL_ABORT_CONTROLLER "Include JsRuntimeHost Polyfills AbortController and AbortSignal." ON)
7373
option(JSRUNTIMEHOST_POLYFILL_WEBSOCKET "Include JsRuntimeHost Polyfill WebSocket." ON)
7474
option(JSRUNTIMEHOST_POLYFILL_BLOB "Include JsRuntimeHost Polyfill Blob." ON)
75+
option(JSR_ENABLE_ASAN "Enable AddressSanitizer support." OFF)
76+
77+
if(JSR_ENABLE_ASAN)
78+
message(STATUS "JSR_ENABLE_ASAN=ON (appending address sanitizer flags)")
79+
set(JSR_ASAN_COMPILE_FLAGS "-fsanitize=address -fno-omit-frame-pointer")
80+
foreach(var CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
81+
if(NOT "${${var}}" MATCHES "-fsanitize=address")
82+
set(${var} "${${var}} ${JSR_ASAN_COMPILE_FLAGS}")
83+
endif()
84+
set(${var} "${${var}}" CACHE STRING "" FORCE)
85+
endforeach()
86+
87+
set(JSR_ASAN_LINK_FLAGS "-fsanitize=address")
88+
foreach(var CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
89+
if(NOT "${${var}}" MATCHES "-fsanitize=address")
90+
set(${var} "${${var}} ${JSR_ASAN_LINK_FLAGS}")
91+
endif()
92+
set(${var} "${${var}}" CACHE STRING "" FORCE)
93+
endforeach()
94+
message(STATUS "ASan compile flags: C=${CMAKE_C_FLAGS} CXX=${CMAKE_CXX_FLAGS}")
95+
message(STATUS "ASan link flags: EXE=${CMAKE_EXE_LINKER_FLAGS} SHARED=${CMAKE_SHARED_LINKER_FLAGS}")
96+
endif()
7597

7698
# --------------------------------------------------
7799

‎Core/Node-API/Source/js_native_api_javascriptcore.cc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -664,6 +664,9 @@ struct napi_ref__ {
664664
CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &_objectId));
665665
if (_objectId == 0) {
666666
CHECK_NAPI(ReferenceInfo::Initialize(env, _value, [value = _value](ReferenceInfo* info) {
667+
if (info->Env()->shutting_down) {
668+
return;
669+
}
667670
auto entry{info->Env()->active_ref_values.find(value)};
668671
// NOTE: The finalizer callback is actually on a "sentinel" JS object that is linked to the
669672
// actual JS object we are trying to track. This means it is possible for the tracked object

‎Core/Node-API/Source/js_native_api_javascriptcore.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ struct napi_env__ {
1414
napi_extended_error_info last_error{nullptr, nullptr, 0, napi_ok};
1515
std::unordered_map<napi_value, std::uintptr_t> active_ref_values{};
1616
std::list<napi_ref> strong_refs{};
17+
bool shutting_down{false};
1718

1819
JSValueRef constructor_info_symbol{};
1920
JSValueRef function_info_symbol{};
@@ -32,6 +33,7 @@ struct napi_env__ {
3233
}
3334

3435
~napi_env__() {
36+
shutting_down = true;
3537
deinit_refs();
3638
deinit_symbol(wrapper_info_symbol);
3739
deinit_symbol(reference_info_symbol);

‎Tests/NodeApi/CMakeLists.txt‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,17 @@ function(node_api_copy_test_sources TARGET_NAME)
2121
endfunction()
2222

2323
if(ANDROID)
24-
set(NODE_LITE_PLATFORM_SRC node_lite_android.cpp)
25-
set(NODE_LITE_CHILD_PROCESS_SRC child_process_android.cpp)
24+
set(NODE_LITE_PLATFORM_SRC node_lite_posix.cpp)
25+
set(NODE_LITE_CHILD_PROCESS_SRC child_process_posix.cpp)
2626
elseif(APPLE)
27-
set(NODE_LITE_PLATFORM_SRC node_lite_mac.cpp)
28-
set(NODE_LITE_CHILD_PROCESS_SRC child_process_mac.cpp)
27+
set(NODE_LITE_PLATFORM_SRC node_lite_posix.cpp)
28+
set(NODE_LITE_CHILD_PROCESS_SRC child_process_posix.cpp)
2929
elseif(WIN32)
3030
set(NODE_LITE_PLATFORM_SRC node_lite_windows.cpp)
3131
set(NODE_LITE_CHILD_PROCESS_SRC child_process.cpp)
3232
else()
33-
set(NODE_LITE_PLATFORM_SRC node_lite_mac.cpp)
34-
set(NODE_LITE_CHILD_PROCESS_SRC child_process_mac.cpp)
33+
set(NODE_LITE_PLATFORM_SRC node_lite_posix.cpp)
34+
set(NODE_LITE_CHILD_PROCESS_SRC child_process_posix.cpp)
3535
message(WARNING "Node-API node_lite platform not yet customized for ${CMAKE_SYSTEM_NAME}; using POSIX defaults.")
3636
endif()
3737

‎Tests/NodeApi/node_lite_jsruntimehost.cpp‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,14 @@ class JsRuntimeHostEnvHolder : public IEnvHolder {
6868
}
6969
}
7070

71+
if (context_ != nullptr) {
72+
JSGlobalContextRelease(context_);
73+
context_ = nullptr;
74+
}
75+
7176
Napi::Detach(napiEnv);
7277
env_ = nullptr;
73-
}
74-
75-
if (context_ != nullptr) {
78+
} else if (context_ != nullptr) {
7679
JSGlobalContextRelease(context_);
7780
context_ = nullptr;
7881
}

‎Tests/NodeApi/node_lite_windows.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@
22
// Licensed under the MIT License.
33

44
#include <windows.h>
5-
#include "node_lite.h"
6-
#include "string_utils.h"
5+
#include "node_lite.h"
76

87
namespace node_api_tests {
98

‎Tests/UnitTests/Android/app/build.gradle‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ if (project.hasProperty("jsEngine")) {
88
}
99

1010
def nodeApiAssetsDir = "${project.buildDir}/generated/nodeapiassets"
11+
def enableAsan = project.hasProperty("enableAsan")
1112

1213
android {
1314
namespace 'com.jsruntimehost.unittests'
@@ -28,11 +29,15 @@ android {
2829

2930
externalNativeBuild {
3031
cmake {
31-
arguments (
32+
def cmakeArgs = [
3233
"-DANDROID_STL=c++_shared",
3334
"-DNAPI_JAVASCRIPT_ENGINE=${jsEngine}",
3435
"-DJSRUNTIMEHOST_CORE_APPRUNTIME_V8_INSPECTOR=ON"
35-
)
36+
]
37+
if (enableAsan) {
38+
cmakeArgs += "-DJSR_ENABLE_ASAN=ON"
39+
}
40+
arguments(*cmakeArgs)
3641
}
3742
}
3843

@@ -64,6 +69,12 @@ android {
6469
viewBinding true
6570
}
6671

72+
packagingOptions {
73+
if (enableAsan) {
74+
doNotStrip "**/*.so"
75+
}
76+
}
77+
6778
sourceSets {
6879
main {
6980
assets.srcDirs += [nodeApiAssetsDir]

‎Tests/UnitTests/Android/app/src/androidTest/java/com/jsruntimehost/unittests/Main.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22

33
import android.content.Context;
44

5-
import java.io.File;
6-
75
import androidx.test.platform.app.InstrumentationRegistry;
86
import androidx.test.ext.junit.runners.AndroidJUnit4;
97

@@ -26,9 +24,6 @@ public void javaScriptTests() {
2624
assertEquals("com.jsruntimehost.unittests", appContext.getPackageName());
2725

2826
Context applicationContext = appContext.getApplicationContext();
29-
File baseDir = new File(applicationContext.getFilesDir(), "node_api_tests");
30-
Native.prepareNodeApiTests(applicationContext, baseDir.getAbsolutePath());
31-
3227
assertEquals(0, Native.javaScriptTests(applicationContext));
3328
}
3429
}

‎Tests/UnitTests/Android/app/src/androidTest/java/com/jsruntimehost/unittests/Native.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,5 @@ public class Native {
88
System.loadLibrary("UnitTestsJNI");
99
}
1010

11-
public static native void prepareNodeApiTests(Context context, String baseDirPath);
1211
public static native int javaScriptTests(Context context);
1312
}

‎Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,15 @@ FetchContent_MakeAvailable_With_Message(googletest)
1515

1616
npm(install --silent WORKING_DIRECTORY ${TESTS_DIR})
1717

18-
set(NODE_LITE_PLATFORM_SRC ${TESTS_DIR}/NodeApi/node_lite_mac.cpp)
19-
set(NODE_LITE_CHILD_PROCESS_SRC ${TESTS_DIR}/NodeApi/child_process_mac.cpp)
18+
set(NODE_LITE_PLATFORM_SRC ${TESTS_DIR}/NodeApi/node_lite_posix.cpp)
19+
set(NODE_LITE_CHILD_PROCESS_SRC ${TESTS_DIR}/NodeApi/child_process_posix.cpp)
2020

21-
if(ANDROID)
22-
set(NODE_LITE_PLATFORM_SRC ${TESTS_DIR}/NodeApi/node_lite_android.cpp)
23-
set(NODE_LITE_CHILD_PROCESS_SRC ${TESTS_DIR}/NodeApi/child_process_android.cpp)
24-
elseif(WIN32)
21+
if(WIN32)
2522
set(NODE_LITE_PLATFORM_SRC ${TESTS_DIR}/NodeApi/node_lite_windows.cpp)
2623
set(NODE_LITE_CHILD_PROCESS_SRC ${TESTS_DIR}/NodeApi/child_process.cpp)
24+
elseif(APPLE)
25+
set(NODE_LITE_PLATFORM_SRC ${TESTS_DIR}/NodeApi/node_lite_mac.cpp)
26+
set(NODE_LITE_CHILD_PROCESS_SRC ${TESTS_DIR}/NodeApi/child_process_mac.cpp)
2727
endif()
2828

2929
add_library(UnitTestsJNI SHARED

0 commit comments

Comments
 (0)