Fix various issues in libpkl (#1805)

* Fix possible SIGSEGV from host process.
   - Require that the same `pkl_exec_t` be used in the same OS thread; returning meaningful error if this fails
* Remove `System.exitProcess(1)` logic in NativeTransport; this would kill the host process too and isn't an appropriate action for a received ProtocolException
* Allow multiple calls to pkl_init without pkl_close; this limitation doesn't really make any sense
* Prefer calling methods defined in graal_isolate.h, instead of creating the same method via CEntryPoint
* Add a CMakeLists.txt so that CLion can follow the code and provide proper diagnostics
* Improve doc comments; describe params as either in or out

CMakeLists.txt

@@ -0,0 +1,24 @@
+# This file is only used to configure CLion with proper include paths.
+# Pkl builds everything through Gradle, not CMake.
+cmake_minimum_required(VERSION 4.2)
+project(pkl C)
+
+set(CMAKE_C_STANDARD 11)
+
+file(GLOB NATIVE_LIB_INCLUDE_DIRS CONFIGURE_DEPENDS
+  libpkl/build/native-libs/*/include)
+file(GLOB NATIVE_IMAGE_BUILD_DIRS CONFIGURE_DEPENDS
+  libpkl/build/tmp/native-image-build/*)
+
+include_directories(${NATIVE_LIB_INCLUDE_DIRS})
+include_directories(${NATIVE_IMAGE_BUILD_DIRS})
+include_directories(libpkl/src/main/c/include)
+
+file(GLOB_RECURSE GENERATED_HEADER_FILES CONFIGURE_DEPENDS
+  libpkl/build/tmp/native-libs/*/*.h)
+
+add_executable(pkl
+        ${GENERATED_HEADER_FILES}
+        libpkl/src/main/c/include/pkl.h
+        libpkl/src/main/c/pkl.c
+        libpkl/src/nativeTest/c/test_pkl.c)

DEVELOPMENT.adoc

@@ -93,6 +93,14 @@ There is an IntelliJ plugin meant for development on the Pkl project itself loca
 
 See https://github.com/apple/pkl-project-commons?tab=readme-ov-file#internal-intellij-plugin[its readme] for instructions on how to set it up.
 
+== Working on libpkl
+
+libpkl is a native library written in C, and is best written in CLion, or VSCode.
+
+IntelliJ has limited support for editing C sources.
+
+There is a CMakeLists.txt file whose only job is to configure CLion with proper include paths.
+
 == Resources
 
 For automated build setup examples see our https://github.com/apple/pkl/blob/main/.github/[GitHub Actions] jobs like our https://github.com/apple/pkl/blob/main/.github/jobs/BuildNativeJob.pkl[BuildNativeJob.pkl], where we build Pkl automatically.

libpkl/libpkl.gradle.kts

@@ -511,7 +511,7 @@ spotless {
       rootProject.file("build-logic/src/main/resources/license-header.star-block.txt"),
       "// ",
     )
-    target("src/*/c/*.c", "src/*/c/*.h")
+    target("src/*/c/**/*.c", "src/*/c/**/*.h")
     eclipseCdt(libs.versions.eclipseCdtFormat.get())
   }
   shell {

libpkl/src/main/c/include/pkl.h

@@ -27,7 +27,7 @@ extern "C" {
 #define PKL_EXPORT __attribute__((visibility("default")))
 #endif
 
-#define PKL_ERR_LOCK      1     /* Failed to create a mutex, or acquire a lock on a mutex */
+#define PKL_ERR_THREAD    1     /* Called using the same pexec_t but from a different thread */
 #define PKL_ERR_PROTOCOL  2     /* Failed to decode a message */
 
 /** Error details that occurred during a method call */
@@ -38,34 +38,36 @@ typedef struct {
 /**
  * Pkl executor instance that manages communication with the Pkl runtime.
  *
- * Instances should be created via `pkl_init()` and destroyed via `pkl_close().`
+ * Instances should be created via `pkl_init` and destroyed via `pkl_close`.
  *
- * All operations on this struct are considered thread-safe and are synchronized via a mutex.
+ * All calls using this executor should be synchronized in the same thread.
  */
 typedef struct __pkl_exec_t pkl_exec_t;
 
 /**
  * The callback that gets called when a message is received from Pkl.
  *
- * Messages must be deserialized to Pkl's Message Passing API: 
+ * Messages must be deserialized to Pkl's Message Passing API:
  * https://pkl-lang.org/main/current/bindings-specification/message-passing-api.html
  *
- * @param length    The length of the message bytes
- * @param message   The message itself
- * @param userData  User-defined data passed in from pkl_init.
+ * @param[in] length    The length of the message bytes
+ * @param[in] message   The message itself
+ * @param[in] userData  User-defined data passed in from pkl_init.
  */
 typedef void (*pkl_message_response_handler)(unsigned int length, char *message,
 		void *userData);
 
 /**
- * Initialises and allocates a Pkl executor, writing it to the slot pointed by `exec`.
- * Only one executor can exist at one time.
- * Calling `pkl_init` multiple times without calling `pkl_close` in between results in an error.
+ * Initializes and allocates a Pkl executor, writing it to the slot pointed by `exec`.
  *
- * @param handler   The callback that gets called when a message is received from Pkl.
- * @param userData  User-defined data that gets passed to handler.
- * @param exec      The pointer to write the created pkl_exec_t to.
- * @param error     The pointer to write error details to.
+ * To clean up resources allocated by the executor, use `pkl_close()`.
+ *
+ * All calls using this executor should come from the same thread.
+ *
+ * @param[in] handler   The callback that gets called when a message is received from Pkl.
+ * @param[in] userData  User-defined data that gets passed to handler.
+ * @param[out] exec      The pointer to write the created pkl_exec_t to.
+ * @param[out] error     The pointer to write error details to. Can optionally be `NULL`.
  *
  * @return 0 on success, non-zero on failure.
  */
@@ -75,25 +77,34 @@ PKL_EXPORT int pkl_init(pkl_message_response_handler handler, void *userData,
 /**
  * Send a message to Pkl, providing the length and a pointer to the first byte.
  *
- * Messages must be serialized to Pkl's Message Passing API:
+ * Messages must be serialized according to Pkl's Message Passing API:
  * https://pkl-lang.org/main/current/bindings-specification/message-passing-api.html
  *
- * @param pexec     The Pkl executor instance.
- * @param length    The length of the message, in bytes.
- * @param message   The message to send to Pkl.
+ * If a message is incorrectly serialized, returns `PKL_ERR_PROTOCOL`.
+ * If called from a different thread than `pkl_exec_t`'s originating thread, returns
+ * `PKL_ERR_THREAD`.
+ *
+ * @param[in] pexec     The Pkl executor instance.
+ * @param[in] length    The length of the message, in bytes.
+ * @param[in] message   The message to send to Pkl.
+ * @param[out] error    The pointer to write error details to. Can optionally be `NULL`.
  *
  * @return 0 on success, and non-zero otherwise.
  */
-PKL_EXPORT int pkl_send_message(pkl_exec_t *pexec, unsigned int length, char *message,
-		pkl_error_t *error);
+PKL_EXPORT int pkl_send_message(const pkl_exec_t *pexec, unsigned int length,
+		char *message, pkl_error_t *error);
 
 /**
  * Cleans up any resources that were created as part of the `pkl_init` process
  * for our `pkl_exec_t` instance.
  *
- * @param pexec     The Pkl executor instance.
+ * If called from a different thread than `pkl_exec_t`'s originating thread, returns
+ * `PKL_ERR_THREAD`.
  *
- * @return 0 on success, -1 if `pexec` is NULL, and an error code otherwise.
+ * @param[in] pexec     The Pkl executor instance.
+ * @param[out] error    The pointer to write error details to. Can optionally be `NULL`.
+ *
+ * @return 0 on success, -1 if `pexec` is `NULL`, and an error code otherwise.
  */
 PKL_EXPORT int pkl_close(pkl_exec_t *pexec, pkl_error_t *error);
 

libpkl/src/main/c/pkl.c

@@ -32,19 +32,14 @@
 #define PKL_VERSION "0.0.0"
 #endif
 
+// ReSharper disable once CppClassNeverUsed
 struct __pkl_exec_t {
-#ifdef _WIN32
-  CRITICAL_SECTION mutex;
-#else
-	pthread_mutex_t mutex;
-#endif
-	graal_isolatethread_t *graal_isolatethread;
 	graal_isolate_t *isolate;
+	graal_isolatethread_t *isolateThread;
 
-	/**
-	 * The caller-supplied handler/userData from pkl_init, invoked only from `queue_thread`.
-	 */
+	/** The caller-supplied handler/userData from pkl_init, invoked only from `queue_thread`. */
 	pkl_message_response_handler handler;
+
 	void *userData;
 
 #ifdef _WIN32
@@ -54,12 +49,10 @@ struct __pkl_exec_t {
 #endif
 };
 
-static int pkl_is_initialized = 0;
-
 /**
  * Polls for messages coming from Pkl and sends them back to the handler.
  *
- * Ensures thats calls into Pkl are wrapped with graal_attach_thread/graal_detach_thread, and
+ * Ensures that calls into Pkl are wrapped with graal_attach_thread/graal_detach_thread, and
  * calls back to the handler are _not_ inside the GraalVM thread context.
  */
 #ifdef _WIN32
@@ -67,7 +60,7 @@ static DWORD WINAPI pkl_dispatch_worker(LPVOID arg) {
 #else
 static void* pkl_dispatch_worker(void *arg) {
 #endif
-	pkl_exec_t *pexec = (pkl_exec_t*) arg;
+	const pkl_exec_t *pexec = (pkl_exec_t*) arg;
 
 	for (;;) {
 		graal_isolatethread_t *thread;
@@ -78,7 +71,7 @@ static void* pkl_dispatch_worker(void *arg) {
 		}
 
 		char *message = NULL;
-		int length = pkl_internal_poll_response(thread, &message);
+		const int length = pkl_internal_poll_response(thread, &message);
 
 		if (graal_detach_thread(thread) != 0) {
 			fprintf(stderr,
@@ -128,40 +121,6 @@ static void pkl_stop_dispatch_worker(pkl_exec_t *pexec) {
 #endif
 }
 
-static void pkl_runtime_cleanup(pkl_exec_t *pexec) {
-	// pkl_internal_server_stop unblocks queue_thread's pending/next poll call, so the join
-	// below is guaranteed to return; only then is it safe to tear down the isolate.
-	pkl_internal_server_stop(pexec->graal_isolatethread);
-	pkl_stop_dispatch_worker(pexec);
-	pkl_internal_close(pexec->graal_isolatethread);
-	pexec->graal_isolatethread = NULL;
-}
-
-static void pkl_unlock_mutex(pkl_exec_t *pexec) {
-#ifdef _WIN32
-  LeaveCriticalSection(&pexec->mutex);
-#else
-	if (pthread_mutex_unlock(&pexec->mutex) != 0) {
-		fprintf(stderr, "fatal: failed to unlock mutex.\n");
-		abort();
-	}
-#endif
-}
-
-static int pkl_lock_mutex(pkl_exec_t *pexec, pkl_error_t *error) {
-#ifdef _WIN32
-  EnterCriticalSection(&pexec->mutex);
-#else
-	if (pthread_mutex_lock(&pexec->mutex) != 0) {
-		if (error != NULL) {
-			error->message = "failed to lock mutex";
-		}
-		return PKL_ERR_LOCK;
-	}
-#endif
-	return 0;
-}
-
 int pkl_init(pkl_message_response_handler handler, void *userData,
 		pkl_exec_t **exec, pkl_error_t *error) {
 	if (handler == NULL) {
@@ -176,92 +135,38 @@ int pkl_init(pkl_message_response_handler handler, void *userData,
 		}
 		return -1;
 	}
-	if (pkl_is_initialized) {
-		if (error != NULL) {
-			error->message =
-					"pkl_init called multiple times without calling pkl_close";
-		}
-		return -1;
-	}
 	pkl_exec_t *pexec = calloc(1, sizeof(pkl_exec_t));
 	if (pexec == NULL) {
 		fprintf(stderr, "failed to allocate pkl_exec_t\n");
 		abort();
 	}
 
-#ifdef _WIN32
-  InitializeCriticalSection(&pexec->mutex);
-#else
-	if (pthread_mutex_init(&pexec->mutex, NULL) != 0) {
-		if (error != NULL) {
-			error->message = "Failed to initialize mutex";
-		}
-		free(pexec);
-		*exec = NULL;
-		return -1;
-	}
-#endif
-
 	pexec->handler = handler;
 	pexec->userData = userData;
 
-	pexec->graal_isolatethread = pkl_internal_init();
+	graal_isolate_t *isolate;
+	graal_isolatethread_t *isolateThread;
 
-	if (pexec->graal_isolatethread == NULL) {
+	if (graal_create_isolate(NULL, &isolate, &isolateThread) != 0) {
 		if (error != NULL) {
-			error->message = "Failed to allocate graal_isolatethread";
+			error->message = "Failed to create graal isolate thread";
 		}
-#ifdef _WIN32
-    DeleteCriticalSection(&pexec->mutex);
-#else
-		if (pthread_mutex_destroy(&pexec->mutex) != 0) {
-			fprintf(stderr, "fatal: failed to destroy mutex.\n");
-			abort();
-		}
-#endif
-		free(pexec);
-		*exec = NULL;
-		return -1;
-	}
-
-	pexec->isolate = graal_get_isolate(pexec->graal_isolatethread);
-	if (pexec->isolate == NULL) {
-		if (error != NULL) {
-			error->message =
-					"Failed to resolve isolate from graal_isolatethread";
-		}
-		pkl_internal_close(pexec->graal_isolatethread);
-#ifdef _WIN32
-    DeleteCriticalSection(&pexec->mutex);
-#else
-		if (pthread_mutex_destroy(&pexec->mutex) != 0) {
-			fprintf(stderr, "fatal: failed to destroy mutex.\n");
-			abort();
-		}
-#endif
 		free(pexec);
 		*exec = NULL;
 		return -1;
 	}
+	pexec->isolate = isolate;
+	pexec->isolateThread = isolateThread;
 
 	if (pkl_start_dispatch_worker(pexec, error) != 0) {
-		pkl_internal_close(pexec->graal_isolatethread);
-#ifdef _WIN32
-    DeleteCriticalSection(&pexec->mutex);
-#else
-		if (pthread_mutex_destroy(&pexec->mutex) != 0) {
-			fprintf(stderr, "fatal: failed to destroy mutex.\n");
-			abort();
-		}
-#endif
+		graal_tear_down_isolate(isolateThread);
 		free(pexec);
 		*exec = NULL;
 		return -1;
 	}
 
-	pkl_internal_server_start(pexec->graal_isolatethread);
+	pkl_internal_server_start(isolateThread);
 
-	pkl_is_initialized = 1;
 	*exec = pexec;
 	if (error != NULL) {
 		error->message = NULL;
@@ -269,8 +174,8 @@ int pkl_init(pkl_message_response_handler handler, void *userData,
 	return 0;
 }
 
-int pkl_send_message(pkl_exec_t *pexec, unsigned int length, char *message,
-		pkl_error_t *error) {
+int pkl_send_message(const pkl_exec_t *pexec, const unsigned int length,
+		char *message, pkl_error_t *error) {
 	if (pexec == NULL) {
 		if (error != NULL) {
 			error->message = "pexec is null";
@@ -284,14 +189,19 @@ int pkl_send_message(pkl_exec_t *pexec, unsigned int length, char *message,
 		return -1;
 	}
 
-	int lock_response = pkl_lock_mutex(pexec, error);
-	if (lock_response != 0) {
-		return lock_response;
+	graal_isolatethread_t *thread = graal_get_current_thread(pexec->isolate);
+	if (thread != pexec->isolateThread) {
+		if (error != NULL) {
+			error->message =
+					"called into pkl_send_message from different thread";
+		}
+		return PKL_ERR_THREAD;
 	}
+
 	char *errormessage = NULL;
-	int resp = pkl_internal_send_message(pexec->graal_isolatethread, length,
-			message, &errormessage);
-	pkl_unlock_mutex(pexec);
+	const int resp = pkl_internal_send_message(thread, (int) length, message,
+			&errormessage);
+
 	if (resp != 0) {
 		if (error != NULL) {
 			error->message = errormessage;
@@ -312,35 +222,24 @@ int pkl_close(pkl_exec_t *pexec, pkl_error_t *error) {
 		return -1;
 	}
 
-#ifdef _WIN32
-  EnterCriticalSection(&pexec->mutex);
-#else
-	if (pthread_mutex_lock(&pexec->mutex) != 0) {
+	graal_isolatethread_t *thread = graal_get_current_thread(pexec->isolate);
+	if (thread != pexec->isolateThread) {
 		if (error != NULL) {
-			error->message = "failed to lock mutex";
+			error->message = "called into pkl_close from different thread";
 		}
-		return PKL_ERR_LOCK;
+		return PKL_ERR_THREAD;
 	}
-#endif
 
-	pkl_runtime_cleanup(pexec);
+	// pkl_internal_server_stop unblocks queue_thread's pending/next poll call, so the join
+	// below is guaranteed to return; only then is it safe to tear down the isolate.
+	pkl_internal_server_stop(thread);
+	pkl_stop_dispatch_worker(pexec);
 
-#ifdef _WIN32
-  LeaveCriticalSection(&pexec->mutex);
-  DeleteCriticalSection(&pexec->mutex);
-#else
-	if (pthread_mutex_unlock(&pexec->mutex) != 0) {
-		fprintf(stderr, "fatal: failed to unlock mutex.\n");
+	if (graal_tear_down_isolate(thread) != 0) {
+		fprintf(stderr, "fatal: failed to tear down graal isolate.\n");
 		abort();
 	}
 
-	if (pthread_mutex_destroy(&pexec->mutex) != 0) {
-		fprintf(stderr, "fatal: failed to destroy mutex.\n");
-		abort();
-	}
-#endif
-
-	pkl_is_initialized = 0;
 	free(pexec);
 	if (error != NULL) {
 		error->message = NULL;

libpkl/src/main/java/org/pkl/libpkl/LibPklInternal.java

@@ -46,9 +46,6 @@ public class LibPklInternal {
 
   private LibPklInternal() {}
 
-  @CEntryPoint(name = "pkl_internal_init", builtin = CEntryPoint.Builtin.CREATE_ISOLATE)
-  static native IsolateThread pklInternalInit();
-
   @CEntryPoint(name = "pkl_internal_send_message")
   public static int pklInternalSendMessage(
       IsolateThread thread, int length, CCharPointer ptr, CCharPointerPointer errorMessage) {
@@ -64,9 +61,6 @@ public class LibPklInternal {
     }
   }
 
-  @CEntryPoint(name = "pkl_internal_close", builtin = CEntryPoint.Builtin.TEAR_DOWN_ISOLATE)
-  public static native void pklInternalClose(IsolateThread thread);
-
   @CEntryPoint(name = "pkl_internal_server_start")
   public static void pklInternalServerStart(IsolateThread thread) {
     server = new Server(transport);

libpkl/src/main/java/org/pkl/libpkl/NativeTransport.java

@@ -43,7 +43,7 @@ public class NativeTransport extends AbstractMessageTransport {
   protected void doClose() {}
 
   @Override
-  protected void doSend(Message message) {
+  protected void doSend(Message message) throws ProtocolException {
     try (var os = new ByteArrayOutputStream();
         var packer = MessagePack.newDefaultPacker(os)) {
       var encoder = new ServerMessagePackEncoder(packer);
@@ -53,9 +53,9 @@ public class NativeTransport extends AbstractMessageTransport {
       // impossible; no IO happens during packing
       throw PklBugException.unreachableCode();
     } catch (ProtocolException e) {
-      System.err.println("Received unexpected ProtocolException when encoding a message, aborting");
-      e.printStackTrace(System.err);
-      System.exit(1);
+      // should never happen; messages coming from Pkl should always be well-formed.
+      log("Unexpected protocol exception: " + e);
+      throw e;
     }
   }
 

libpkl/src/nativeTest/c/test_pkl.c

@@ -77,6 +77,20 @@ void test_empty_message() {
 	printf("✓ Empty message rejected\n");
 }
 
+void test_garbage_send_message() {
+	pkl_error_t err = { 0 };
+	pkl_exec_t *exec = NULL;
+	assert(pkl_init(test_message_handler, NULL, &exec, &err) == 0);
+
+	char *message = (char*) (unsigned char[] ) { 0x41, 0x42, 0x43 };
+	int result = pkl_send_message(exec, 3, message, &err);
+	assert(result == PKL_ERR_PROTOCOL);
+
+	result = pkl_close(exec, &err);
+	assert(result == 0);
+	printf("✓ Garbage message becomes protocol exception\n");
+}
+
 int main() {
 	printf("Running libpkl C tests...\n\n");
 
@@ -84,6 +98,7 @@ int main() {
 	test_init_close();
 	test_null_send_message();
 	test_empty_message();
+	test_garbage_send_message();
 
 	printf("\nAll tests passed!\n");
 	return 0;

libpkl/src/nativeTest/kotlin/org/pkl/libpkl/LibPklTest.kt

@@ -13,17 +13,20 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+@file:Suppress("FunctionName")
+
 package org.pkl.libpkl
 
 import com.sun.jna.Pointer
 import com.sun.jna.ptr.PointerByReference
+import java.util.concurrent.Executors
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.Test
 import org.pkl.core.Release
 
 class LibPklTest {
   @Test
-  fun testMalformedMessage() {
+  fun `malformed message`() {
     val messageResponseHandler =
       object : LibPklJNA.PklMessageResponseHandler {
         override fun invoke(length: Int, message: Pointer, userData: Pointer?) {}
@@ -45,7 +48,44 @@ class LibPklTest {
   }
 
   @Test
-  fun testEmptyMessageIsRejected() {
+  fun `empty message is rejected`() {
+    val exec = init()
+    val error = LibPklJNA.PklError()
+    try {
+      val result = LibPklJNA.INSTANCE.pkl_send_message(exec, 0, byteArrayOf(0), error)
+      assertThat(result).isEqualTo(2)
+      assertThat(error.message).contains("Unexpected end of input")
+    } finally {
+      assertThat(LibPklJNA.INSTANCE.pkl_close(exec, error)).isEqualTo(0)
+    }
+  }
+
+  @Test
+  fun `version string matches current version`() {
+    val currentVersion = Release.current().version.toString()
+    assertThat(LibPklJNA.INSTANCE.pkl_version()).isEqualTo(currentVersion)
+  }
+
+  @Test
+  fun `calling into libpkl from different thread using same pkl_exec_t instance fails`() {
+    val exec = init()
+    val error = LibPklJNA.PklError()
+    val executor = Executors.newSingleThreadExecutor()
+    try {
+      executor
+        .submit {
+          val resp = LibPklJNA.INSTANCE.pkl_close(exec, error)
+          assertThat(resp).isEqualTo(1)
+          assertThat(error.message).isEqualTo("called into pkl_close from different thread")
+        }
+        .get()
+    } finally {
+      executor.shutdown()
+      assertThat(LibPklJNA.INSTANCE.pkl_close(exec, error)).isEqualTo(0)
+    }
+  }
+
+  private fun init(): Pointer {
     val execRef = PointerByReference()
     val error = LibPklJNA.PklError()
     val messageResponseHandler =
@@ -55,18 +95,6 @@ class LibPklTest {
     assertThat(LibPklJNA.INSTANCE.pkl_init(messageResponseHandler, Pointer.NULL, execRef, error))
       .`as` { "Failed to call pkl_init: ${error.message}" }
       .isEqualTo(0)
-    try {
-      val result = LibPklJNA.INSTANCE.pkl_send_message(execRef.value, 0, byteArrayOf(0), error)
-      assertThat(result).isEqualTo(2)
-      assertThat(error.message).contains("Unexpected end of input")
-    } finally {
-      assertThat(LibPklJNA.INSTANCE.pkl_close(execRef.value, error)).isEqualTo(0)
-    }
-  }
-
-  @Test
-  fun testVersionString() {
-    val currentVersion = Release.current().version.toString()
-    assertThat(LibPklJNA.INSTANCE.pkl_version()).isEqualTo(currentVersion)
+    return execRef.value
   }
 }

pkl-server/src/main/kotlin/org/pkl/server/Server.kt

@@ -231,7 +231,9 @@ class Server(private val transport: MessageTransport) : AutoCloseable {
         build()
       }
     } catch (e: IllegalArgumentException) {
-      throw ProtocolException(e.message ?: "Failed to create an evalutor. $e", e)
+      throw ProtocolException(e.message ?: "Failed to create an evaluator. $e", e)
+    } catch (e: IllegalStateException) {
+      throw ProtocolException(e.message ?: "Failed to create an evaluator. $e", e)
     }
   }