Fix unicode errors

Changes `write()` method of native pipe and terminal to accept a
PoolByteArray rather than String. This means that `get_string_from_utf8()`
is no longer called on data coming from PTY and being sent to Terminal.

The terminal state machine already has a UTF8 parser which maintains
its state across calls to `write()`. This means that we can write half
the bytes of a single unicode character in one call and the remaining half
in the next call and the state machine will parse it correctly.

On the other hand, the `get_string_from_utf8()` method of Godot's
PoolByteArray requires that the array contains completely valid UTF8,
otherwise we get errors such as "Unicode error: invalid skip".

The data coming from PTY can be arbitrarily split in the middle of a
unicode character meaning that we will sometimes get errors when calling
`get_string_from_utf8()` on it. This is more likely to occur when there
is a large amount of output (i.e. it's more likely to be split). In other
cases, the data might intentionally contain invalid unicode such as when
printing binary files or random data (e.g. `cat /bin/sh`, `cat /dev/random`).

We avoid these errors by passing the PoolByteArray data directly to the terminal
state machine.

In addition to fixing unicode errors, this commit:
- Prevents repeated calls to pipes `_read_cb()` method that would block Godot
  and result in a crash with the message "ERROR: All memory pool allocations
  are in use" that resulted from writing data to an ever-increasing number of
  PoolByteArrays before any of them could be freed. This could be triggered by
  running the `cat /dev/urandom` command after making the change to `write()`
  mentioned above.
- Prevents memory leaks by freeing libuv buffers after they have been copied
  to PoolByteArrays.

Fixes #55.

addons/godot_xterm/native/src/pipe.cpp

@@ -66,20 +66,18 @@ void Pipe::close() {
   uv_run(uv_default_loop(), UV_RUN_NOWAIT);
 }
 
-godot_error Pipe::write(String p_data) {
+godot_error Pipe::write(PoolByteArray data) {
-  char *s = p_data.alloc_c_string();
+  char *s = (char *)data.read().ptr();
-  ULONG len = strlen(s);
+  ULONG len = data.size();
 
-  uv_buf_t bufs[1];
+  uv_buf_t buf;
-  bufs[0].base = s;
+  uv_write_t *req = (uv_write_t *)malloc(sizeof(uv_write_t));
-  bufs[0].len = len;
 
-  uv_write_t req;
+  buf.base = s;
-
+  buf.len = len;
-  req.data = s;
+  req->data = (void *)buf.base;
-
-  uv_write(&req, (uv_stream_t *)&handle, bufs, 1, _write_cb);
 
+  uv_write(req, (uv_stream_t *)&handle, &buf, 1, _write_cb);
   uv_run(uv_default_loop(), UV_RUN_NOWAIT);
 
   return GODOT_OK;
@@ -92,10 +90,16 @@ int Pipe::get_status() {
   return status;
 }
 
-void Pipe::_poll_connection() { uv_run(uv_default_loop(), UV_RUN_NOWAIT); }
+void Pipe::_poll_connection() {
+  if (status == 1 && !uv_is_active((uv_handle_t *)&handle))
+    uv_read_start((uv_stream_t *)&handle, _alloc_buffer, _read_cb);
+
+  uv_run(uv_default_loop(), UV_RUN_NOWAIT);
+}
 
 void _read_cb(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf) {
   Pipe *pipe = static_cast<Pipe *>(handle->data);
+
   if (nread < 0) {
     switch (nread) {
     case UV_EOF:
@@ -104,6 +108,7 @@ void _read_cb(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf) {
       // Can happen when the process exits.
       // As long as PTY has caught it, we should be fine.
       uv_read_stop(handle);
+      pipe->status = 0;
       return;
     default:
       UV_ERR_PRINT(nread);
@@ -113,12 +118,19 @@ void _read_cb(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf) {
 
   PoolByteArray data;
   data.resize(nread);
-  memcpy(data.write().ptr(), buf->base, nread);
+  { memcpy(data.write().ptr(), buf->base, nread); }
+  std::free((char *)buf->base);
 
   pipe->emit_signal("data_received", data);
+
+  // Stop reading until the next poll, otherwise _read_cb could be called
+  // repeatedly, blocking Godot, and eventually resulting in a memory pool
+  // allocation error. This can be triggered with the command `cat /dev/urandom`
+  // if reading is not stopped.
+  uv_read_stop(handle);
 }
 
-void _write_cb(uv_write_t *req, int status) {}
+void _write_cb(uv_write_t *req, int status) { std::free(req); }
 
 void _alloc_buffer(uv_handle_t *handle, size_t suggested_size, uv_buf_t *buf) {
   buf->base = (char *)malloc(suggested_size);
@@ -128,4 +140,4 @@ void _alloc_buffer(uv_handle_t *handle, size_t suggested_size, uv_buf_t *buf) {
 void _close_cb(uv_handle_t *handle) {
   Pipe *pipe = static_cast<Pipe *>(handle->data);
   pipe->status = 0;
-}
+}

addons/godot_xterm/native/src/pipe.h

@@ -26,7 +26,7 @@ public:
   void close();
   int get_status();
 
-  godot_error write(String p_data);
+  godot_error write(PoolByteArray data);
 
   void pause();
   void resume();

addons/godot_xterm/native/src/terminal.cpp

@@ -224,19 +224,17 @@ static void term_output(const char *s, size_t len, void *user) {}
 
 static void write_cb(struct tsm_vte *vte, const char *u8, size_t len,
                      void *data) {
-
   Terminal *term = static_cast<Terminal *>(data);
 
   PoolByteArray bytes = PoolByteArray();
-
+  bytes.resize(len);
-  for (int i = 0; i < len; i++)
+  { memcpy(bytes.write().ptr(), u8, len); }
-    bytes.append(u8[i]);
 
   if (len > 0) {
     if (term->input_event_key.is_valid()) {
       // The callback was fired from a key press event so emit the "key_pressed"
       // signal.
-      term->emit_signal("key_pressed", String(u8), term->input_event_key);
+      term->emit_signal("key_pressed", bytes, term->input_event_key);
       term->input_event_key.unref();
     }
 
@@ -263,7 +261,7 @@ static int text_draw_cb(struct tsm_screen *con, uint64_t id, const uint32_t *ch,
   if (len < 1) // No foreground to draw.
     return 0;
 
-  size_t ulen;
+  size_t ulen = 0;
   char buf[5] = {0};
 
   char *utf8 = tsm_ucs4_to_utf8_alloc(ch, len, &ulen);
@@ -310,8 +308,9 @@ void Terminal::_register_methods() {
 
   register_signal<Terminal>("data_sent", "data",
                             GODOT_VARIANT_TYPE_POOL_BYTE_ARRAY);
-  register_signal<Terminal>("key_pressed", "data", GODOT_VARIANT_TYPE_STRING,
+  register_signal<Terminal>("key_pressed", "data",
-                            "event", GODOT_VARIANT_TYPE_OBJECT);
+                            GODOT_VARIANT_TYPE_POOL_BYTE_ARRAY, "event",
+                            GODOT_VARIANT_TYPE_OBJECT);
   register_signal<Terminal>("size_changed", "new_size",
                             GODOT_VARIANT_TYPE_VECTOR2);
   register_signal<Terminal>("bell", Dictionary());
@@ -609,11 +608,8 @@ void Terminal::update_size() {
   emit_signal("size_changed", Vector2(cols, rows));
 }
 
-void Terminal::write(String data) {
+void Terminal::write(PoolByteArray data) {
-  const char *u8 = data.alloc_c_string();
+  tsm_vte_input(vte, (char *)data.read().ptr(), data.size());
-  size_t len = strlen(u8);
-
-  tsm_vte_input(vte, u8, len);
 }
 
 void Terminal::sb_up(int num) {

addons/godot_xterm/native/src/terminal.h

@@ -53,7 +53,7 @@ public:
   void _gui_input(Variant event);
   void _draw();
 
-  void write(String data);
+  void write(PoolByteArray data);
 
   void sb_up(int num);
   void sb_down(int num);

addons/godot_xterm/nodes/pty/unix/pty_unix.gd

@@ -68,13 +68,12 @@ var _exit_cb: FuncRef
 # Writes data to the socket.
 # data: The data to write.
 func write(data) -> void:
-	assert(data is String or data is PoolByteArray)
+	assert(
-
+		data is PoolByteArray or data is String,
-	if data is PoolByteArray:
+		"Invalid type for argument 'data'. Should be of type PoolByteArray or String"
-		data = data.get_string_from_utf8()
+	)
-
 	if _pipe:
-		_pipe.write(data)
+		_pipe.write(data if data is PoolByteArray else data.to_utf8())
 
 
 func resize(cols: int, rows: int) -> void:

addons/godot_xterm/terminal.gd

@@ -78,7 +78,10 @@ func get_cols() -> int:
 
 
 func write(data) -> void:
-	assert(data is String or data is PoolByteArray)
+	assert(
+		data is PoolByteArray or data is String,
+		"Invalid type for argument 'data'. Should be of type PoolByteArray or String."
+	)
 
 	# Will be cleared when _flush() is called after VisualServer emits the "frame_pre_draw" signal.
 	_buffer.push_back(data)
@@ -90,7 +93,7 @@ func write(data) -> void:
 
 func _flush():
 	for data in _buffer:
-		_native_terminal.write(data if data is String else data.get_string_from_utf8())
+		_native_terminal.write(data if data is PoolByteArray else data.to_utf8())
 		_native_terminal.update()
 	_buffer.clear()
 
@@ -269,8 +272,8 @@ func _on_data_sent(data: PoolByteArray):
 	emit_signal("data_sent", data)
 
 
-func _on_key_pressed(data: String, event: InputEventKey):
+func _on_key_pressed(data: PoolByteArray, event: InputEventKey):
-	emit_signal("key_pressed", data, event)
+	emit_signal("key_pressed", data.get_string_from_utf8(), event)
 
 
 func _on_size_changed(new_size: Vector2):

test/integration/terminal.test.gd

@@ -0,0 +1,9 @@
+extends "res://addons/gut/test.gd"
+
+const Terminal := preload("res://addons/godot_xterm/terminal.gd")
+
+
+func test_writing_random_data_to_terminal_does_not_crash_application():
+	add_child_autofree(preload("res://test/scenes/write_random.tscn").instance())
+	yield(yield_frames(5, "Writing random data to terminal"), YIELD)
+	assert_true(true, "Expected no crash when writing random data to terminal.")

test/scenes/write_random.tscn

@@ -0,0 +1,21 @@
+[gd_scene load_steps=2 format=2]
+
+[sub_resource type="GDScript" id=1]
+script/source = "extends \"res://addons/godot_xterm/terminal.gd\"
+
+var rng := RandomNumberGenerator.new()
+
+
+func _ready():
+	rng.seed = 0
+
+
+func _process(_delta):
+	for _i in range(4096):
+		write(PoolByteArray([rng.randi() % 256]))
+"
+
+[node name="Terminal" type="Control"]
+anchor_right = 1.0
+anchor_bottom = 1.0
+script = SubResource( 1 )