Guarantee null-termination for strings. (#2316)

include/toit/toit.h

@@ -62,6 +62,8 @@ typedef toit_err_t (*toit_msg_on_created_cb_t)(void* user_data, toit_msg_context
  * service) to this service.
  *
  * The data is owned by the receiver and must be freed.
+ * If the Toit side sent a string, then the data is guaranteed to be 0-terminated. The
+ * length does *not* include the 0-terminator.
  *
  * @param user_data The user data passed to `toit_msg_add_handler`.
  * @param sender The PID of the sender.
@@ -85,6 +87,8 @@ typedef toit_err_t (*toit_msg_on_message_cb_t)(void* user_data, int sender, uint
  * It is an error to not reply to the request, or to reply more than once.
  *
  * The data is owned by the receiver and must be freed.
+ * If the Toit side sent a string, then the data is guaranteed to be 0-terminated. The
+ * length does *not* include the 0-terminator.
  *
  * @param user_data The user data passed to `toit_msg_add_handler`.
  * @param sender The PID of the sender.

lib/system/external.toit

@@ -64,19 +64,25 @@ class Client:
   is-closed -> bool:
     return is-closed_
 
-  /** Helper to convert the $message to a ByteArray. */
-  encode-message_ message/io.Data --copy/bool -> ByteArray:
+  /** Helper to convert the $message to a string or ByteArray. */
+  encode-message_ message/io.Data --copy/bool -> io.Data:
     bytes/ByteArray := ?
-    if copy or message is not ByteArray:
-      bytes = ByteArray message.byte-size
-      message.write-to-byte-array bytes --at=0 0 message.byte-size
-    else:
-      bytes = message as ByteArray
+    if message is string:
+      return message
+    if not copy and message is ByteArray:
+      return message
+    bytes = ByteArray message.byte-size
+    message.write-to-byte-array bytes --at=0 0 message.byte-size
     return bytes
 
   /**
   Sends a notification message to the external process.
 
+  If the $message is a string, then the receiver is guaranteed to receive
+    the string data with an additional 0-terminator. The external process can
+    safely interpret it as a C string. The length argument to the receiver
+    does *not* include the 0-terminator.
+
   If $copy is true (the default) copies the given $message before sending it.
   If $copy is false, attempts to transfer ownership of the $message to the
     external process. This is only possible for $ByteArray instances that
@@ -94,6 +100,11 @@ class Client:
   The $function id is an integer that the external process receives as
     argument. External processes are free to interpret this id as they see fit.
 
+  If the $message is a string, then the receiver is guaranteed to receive
+    the string data with an additional 0-terminator. The external process can
+    safely interpret it as a C string. The length argument to the receiver
+    does *not* include the 0-terminator.
+
   If $copy is true (the default), copies the given $message before sending it.
   If $copy is false, attempts to transfer ownership of the $message to the
     external process. This is only possible for $ByteArray instances that

src/messaging.cc

@@ -709,7 +709,7 @@ Object* MessageDecoder::decode_byte_array(bool inlined) {
   return result;
 }
 
-bool MessageDecoder::decode_byte_array_external(void** data, word* length) {
+bool MessageDecoder::decode_external_data(void** data, word* length) {
   if (decoding_tison()) return false;
   int tag = read_uint8();
   if (tag == TAG_BYTE_ARRAY) {
@@ -729,6 +729,22 @@ bool MessageDecoder::decode_byte_array_external(void** data, word* length) {
     memcpy(copy, &buffer_[cursor_], encoded_length);
     *data = copy;
     return true;
+  } else if (tag == TAG_STRING) {
+    *length = read_cardinal();
+    *data = read_pointer();
+    return true;
+  } else if (tag == TAG_STRING_INLINE) {
+    int encoded_length = *length = read_cardinal();
+    char* copy = unvoid_cast<char*>(malloc(encoded_length + 1));
+    if (copy == null) {
+      mark_allocation_failed();
+      return false;
+    }
+    memcpy(copy, &buffer_[cursor_], encoded_length);
+    copy[encoded_length] = '\0';
+    *length = encoded_length;  // Exclude the '\0'.
+    *data = copy;
+    return true;
   }
   return false;
 }
@@ -749,7 +765,7 @@ bool MessageDecoder::decode_rpc_request_external(int* id, int* name, void** data
   if (tag != TAG_POSITIVE_SMI) return false;
   *name = read_cardinal();
   if (overflown()) return false;
-  return decode_byte_array_external(data, length);
+  return decode_external_data(data, length);
 }
 
 Object* MessageDecoder::decode_double() {
@@ -926,7 +942,7 @@ Interpreter::Result ExternalSystemMessageHandler::run() {
       if (type == SYSTEM_RPC_REQUEST && supports_rpc_requests()) {
         success = decoder.decode_rpc_request_external(&id, &name, &data, &length);
       } else {
-        success = decoder.decode_byte_array_external(&data, &length);
+        success = decoder.decode_external_data(&data, &length);
       }
       if (success && length > INT_MAX) {
         abort();

src/messaging.h

@@ -297,7 +297,7 @@ class MessageDecoder {
   void remove_disposing_finalizers();
 
   Object* decode() { ASSERT(!decoding_tison()); return decode_any(); }
-  bool decode_byte_array_external(void** data, word* length);
+  bool decode_external_data(void** data, word* length);
   bool decode_rpc_request_external(int* id, int* name, void** data, word* length);
 
   // Encoded messages may contain pointers to external areas allocated using

tests/ctest/external-messaging2-toit-run-input.toit

@@ -3,6 +3,7 @@
 // be found in the tests/LICENSE file.
 
 import expect show *
+import io
 import monitor
 import system.external
 
@@ -26,6 +27,7 @@ main:
   test-id clients
 
   test-rpc clients #[42]
+  test-rpc clients "foobar"
   test-rpc-fail clients
   test-gc clients
 
@@ -36,6 +38,10 @@ main:
   test-notification clients (ByteArray 319: it)
   test-notification clients (ByteArray 3197: it)
   test-notification clients (ByteArray 31971: it)
+  test-notification clients ""
+  test-notification clients "foobar"
+  test-notification clients "foobar"[3..]
+  test-notification clients "foobar" * 100
   test-notification clients #[99, 99]
 
   clients.do: it.close
@@ -48,10 +54,11 @@ test-id clients/List:
     expect-equals 1 response.size
     expect-equals id response[0]
 
-test-rpc clients/List data/ByteArray:
+test-rpc clients/List data/io.Data:
   clients.do: | client/external.Client |
     response := client.request 0 data
-    expect-bytes-equal data response
+    bytes := ByteArray.from data
+    expect-equals bytes response
 
 test-rpc-fail clients/List:
   clients.do: | client/external.Client |
@@ -65,9 +72,10 @@ test-gc clients/List:
     expect-equals 1 response.size
     expect-equals 0 response[0]
 
-test-notification clients/List data/ByteArray:
+test-notification clients/List data/io.Data:
   clients.size.repeat: | i |
     client/external.Client := clients[i]
     client.notify data
     result := incoming-notifications[i].receive
-    expect-bytes-equal data result
+    bytes := ByteArray.from data
+    expect-equals bytes result

tests/ctest/external-messaging3-toit-run-input.toit

@@ -0,0 +1,30 @@
+// Copyright (C) 2024 Toitware ApS.
+// Use of this source code is governed by a Zero-Clause BSD license that can
+// be found in the tests/LICENSE file.
+
+import expect show *
+import io
+import monitor
+import system.external
+
+EXTERNAL-ID ::= "toit.io/external-test"
+
+// Test that strings are 0-terminated on the C side.
+main:
+  client := external.Client.open EXTERNAL-ID
+
+  strings := [
+    "",
+    "foo",
+    "bar",
+    "foobar",
+    "foobar" * 100,
+    "foobar" * 1000,
+    "foo"[3..],
+    "foobar"[3..],
+    ("foobar" * 1000)[1..],
+  ]
+  strings.do: | str/string |
+    response/ByteArray := client.request 0 str
+    expect-equals (str.size + 1) response.size
+    expect-equals 0 response.last

tests/ctest/external-messaging3-toit-run-test.c

@@ -0,0 +1,23 @@
+// Copyright (C) 2024 Toitware ApS.
+// Use of this source code is governed by a Zero-Clause BSD license that can
+// be found in the tests/LICENSE file.
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+
+#include "../../include/toit/toit.h"
+
+static toit_err_t on_rpc_request(void* user_data, int sender, int function, toit_msg_request_handle_t handle, uint8_t* data, int length) {
+  if (toit_msg_request_reply(handle, data, length + 1, true) != TOIT_OK) {
+    printf("unable to reply\n");
+  }
+  return TOIT_OK;
+}
+
+static void __attribute__((constructor)) init() {
+  printf("registering external handler 1\n");
+  toit_msg_cbs_t cbs = TOIT_MSG_EMPTY_CBS();
+  cbs.on_rpc_request = on_rpc_request;
+  toit_msg_add_handler("toit.io/external-test", NULL, cbs);
+}