From 3ad0e105e7fe9b7f51d4bde6963b5f0fed42d543 Mon Sep 17 00:00:00 2001 From: Leroy Hopson Date: Sun, 14 Aug 2022 19:45:17 +1200 Subject: [PATCH] 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 | 42 ++++++++++++------- addons/godot_xterm/native/src/pipe.h | 2 +- addons/godot_xterm/native/src/terminal.cpp | 22 ++++------ addons/godot_xterm/native/src/terminal.h | 2 +- addons/godot_xterm/nodes/pty/unix/pty_unix.gd | 11 +++-- addons/godot_xterm/terminal.gd | 11 +++-- test/integration/terminal.test.gd | 9 ++++ test/scenes/write_random.tscn | 21 ++++++++++ 8 files changed, 80 insertions(+), 40 deletions(-) create mode 100644 test/integration/terminal.test.gd create mode 100644 test/scenes/write_random.tscn diff --git a/addons/godot_xterm/native/src/pipe.cpp b/addons/godot_xterm/native/src/pipe.cpp index d313c1d..403f1db 100644 --- a/addons/godot_xterm/native/src/pipe.cpp +++ b/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) { - char *s = p_data.alloc_c_string(); - ULONG len = strlen(s); +godot_error Pipe::write(PoolByteArray data) { + char *s = (char *)data.read().ptr(); + ULONG len = data.size(); - uv_buf_t bufs[1]; - bufs[0].base = s; - bufs[0].len = len; + uv_buf_t buf; + uv_write_t *req = (uv_write_t *)malloc(sizeof(uv_write_t)); - uv_write_t req; - - req.data = s; - - uv_write(&req, (uv_stream_t *)&handle, bufs, 1, _write_cb); + buf.base = s; + buf.len = len; + req->data = (void *)buf.base; + 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(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(handle->data); pipe->status = 0; -} \ No newline at end of file +} diff --git a/addons/godot_xterm/native/src/pipe.h b/addons/godot_xterm/native/src/pipe.h index 0b7c09a..19ebb5a 100644 --- a/addons/godot_xterm/native/src/pipe.h +++ b/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(); diff --git a/addons/godot_xterm/native/src/terminal.cpp b/addons/godot_xterm/native/src/terminal.cpp index 0d67f81..7084e8d 100644 --- a/addons/godot_xterm/native/src/terminal.cpp +++ b/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(data); PoolByteArray bytes = PoolByteArray(); - - for (int i = 0; i < len; i++) - bytes.append(u8[i]); + bytes.resize(len); + { memcpy(bytes.write().ptr(), u8, len); } 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("data_sent", "data", GODOT_VARIANT_TYPE_POOL_BYTE_ARRAY); - register_signal("key_pressed", "data", GODOT_VARIANT_TYPE_STRING, - "event", GODOT_VARIANT_TYPE_OBJECT); + register_signal("key_pressed", "data", + GODOT_VARIANT_TYPE_POOL_BYTE_ARRAY, "event", + GODOT_VARIANT_TYPE_OBJECT); register_signal("size_changed", "new_size", GODOT_VARIANT_TYPE_VECTOR2); register_signal("bell", Dictionary()); @@ -609,11 +608,8 @@ void Terminal::update_size() { emit_signal("size_changed", Vector2(cols, rows)); } -void Terminal::write(String data) { - const char *u8 = data.alloc_c_string(); - size_t len = strlen(u8); - - tsm_vte_input(vte, u8, len); +void Terminal::write(PoolByteArray data) { + tsm_vte_input(vte, (char *)data.read().ptr(), data.size()); } void Terminal::sb_up(int num) { diff --git a/addons/godot_xterm/native/src/terminal.h b/addons/godot_xterm/native/src/terminal.h index 7757bd3..47c364a 100644 --- a/addons/godot_xterm/native/src/terminal.h +++ b/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); diff --git a/addons/godot_xterm/nodes/pty/unix/pty_unix.gd b/addons/godot_xterm/nodes/pty/unix/pty_unix.gd index 3f12859..789c143 100644 --- a/addons/godot_xterm/nodes/pty/unix/pty_unix.gd +++ b/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) - - if data is PoolByteArray: - data = data.get_string_from_utf8() - + assert( + data is PoolByteArray or data is String, + "Invalid type for argument 'data'. Should be of type PoolByteArray or String" + ) if _pipe: - _pipe.write(data) + _pipe.write(data if data is PoolByteArray else data.to_utf8()) func resize(cols: int, rows: int) -> void: diff --git a/addons/godot_xterm/terminal.gd b/addons/godot_xterm/terminal.gd index 6f7c14f..3b789d4 100644 --- a/addons/godot_xterm/terminal.gd +++ b/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): - emit_signal("key_pressed", data, event) +func _on_key_pressed(data: PoolByteArray, event: InputEventKey): + emit_signal("key_pressed", data.get_string_from_utf8(), event) func _on_size_changed(new_size: Vector2): diff --git a/test/integration/terminal.test.gd b/test/integration/terminal.test.gd new file mode 100644 index 0000000..dabcbe6 --- /dev/null +++ b/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.") diff --git a/test/scenes/write_random.tscn b/test/scenes/write_random.tscn new file mode 100644 index 0000000..32827f2 --- /dev/null +++ b/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 )