From b0f4d081109fe2ea96019b4dd72eec95253e595d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?La=C3=A9rcio=20de=20Sousa?= Date: Mon, 2 Nov 2026 11:58:56 -0300 Subject: [PATCH 1/6] goodixtls: never block the main loop on the TLS sockets The TLS server talks to the client side of a blocking socketpair from fprintd's main thread. SSL_read() on an incomplete record therefore waited forever for bytes only this same thread could write: the whole daemon stopped answering D-Bus, with nothing to recover it. The handshake's read() of the server thread's output had the same problem. Make both sockets non-blocking once the handshake is done and report an incomplete record as an error; bound the handshake reads with poll(). Also fix err_from_ssl(), which this path now relies on. It built a GError without a domain, and would crash in strlen(NULL) if OpenSSL had no error queued. Co-Authored-By: Claude Opus 5.5 --- libfprint/drivers/goodixtls/goodix.c | 18 ++++++-- libfprint/drivers/goodixtls/goodixtls.c | 55 +++++++++++++++++++++---- libfprint/drivers/goodixtls/goodixtls.h | 7 ++++ 3 files changed, 67 insertions(+), 13 deletions(-) diff --git a/libfprint/drivers/goodixtls/goodix.c b/libfprint/drivers/goodixtls/goodix.c index d5cac78..fec0d95 100644 --- a/libfprint/drivers/goodixtls/goodix.c +++ b/libfprint/drivers/goodixtls/goodix.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -1149,8 +1150,10 @@ static void on_tls_successfully_established(FpDevice* dev, gpointer user_data, FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); + if (!error) + goodix_tls_server_set_nonblocking(priv->tls_hop, &error); ((GoodixNoneCallback) priv->tls_ready_callback->callback)( - dev, priv->tls_ready_callback->user_data, NULL); + dev, priv->tls_ready_callback->user_data, error); } static void tls_handshake_done(FpiSsm* ssm, FpDevice* dev, GError* error) { @@ -1172,7 +1175,7 @@ static void tls_handshake_run(FpiSsm* ssm, FpDevice* dev) if (stage == TLS_HANDSHAKE_STAGE_HELLO_S) { guint8 buff[1024]; int size = goodix_tls_client_recv(priv->tls_hop, buff, sizeof(buff)); - if (size < 0) { + if (size <= 0) { fpi_ssm_mark_failed(ssm, g_error_new(g_io_error_quark(), size, "failed to read tls server " "hello")); @@ -1194,7 +1197,7 @@ static void tls_handshake_run(FpiSsm* ssm, FpDevice* dev) fp_dbg("Reading to proxy back"); guint8 buff[1024]; int size = goodix_tls_client_recv(priv->tls_hop, buff, sizeof(buff)); - if (size < 0) { + if (size <= 0) { fpi_ssm_mark_failed(ssm, g_error_new(g_io_error_quark(), size, "failed to read server " "handshake")); @@ -1301,7 +1304,14 @@ static void goodix_tls_ready_image_handler(FpDevice* dev, guint8* data, FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); - goodix_tls_client_send(priv->tls_hop, data, length); + if (goodix_tls_client_send(priv->tls_hop, data, length) < 0) { + callback(dev, NULL, 0, cb_info->user_data, + g_error_new(G_IO_ERROR, g_io_error_from_errno(errno), + "failed to pass the image to the TLS server: %s", + g_strerror(errno))); + g_free(cb_info); + return; + } const guint16 size = -1; guint8* buff = malloc(size); diff --git a/libfprint/drivers/goodixtls/goodixtls.c b/libfprint/drivers/goodixtls/goodixtls.c index 3596420..a47dba6 100644 --- a/libfprint/drivers/goodixtls/goodixtls.c +++ b/libfprint/drivers/goodixtls/goodixtls.c @@ -19,6 +19,8 @@ #include #include +#include +#include #include #include #include @@ -30,6 +32,7 @@ #include #include #include +#include #include "drivers_api.h" #include "fp-device.h" @@ -38,15 +41,14 @@ #include "goodix.h" #include "goodixtls.h" -static GError* err_from_ssl(void) +// The queue can be empty, and then there is no reason string. +static GError* err_from_ssl(const char* what) { - GError* err = malloc(sizeof(GError)); unsigned long code = ERR_get_error(); - err->code = code; - const char* msg = ERR_reason_error_string(code); - err->message = malloc(strlen(msg) + 1); - strcpy(err->message, msg); - return err; + const char* reason = code ? ERR_reason_error_string(code) : NULL; + + return g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED, "%s: %s", what, + reason ? reason : "no OpenSSL error queued"); } static unsigned int tls_server_psk_server_callback(SSL *ssl, @@ -99,15 +101,32 @@ int goodix_tls_client_send(GoodixTlsServer* self, guint8* data, guint16 length) return write(self->client_fd, data, length * sizeof(guint8)); } int goodix_tls_client_recv(GoodixTlsServer* self, guint8* data, guint16 length) { + struct pollfd pfd = {.fd = self->client_fd, .events = POLLIN}; + int ready = poll(&pfd, 1, GOODIX_TLS_IO_TIMEOUT_MS); + + if (ready <= 0) { + if (ready == 0) + errno = ETIMEDOUT; + return -1; + } return read(self->client_fd, data, length * sizeof(guint8)); } int goodix_tls_server_receive(GoodixTlsServer* self, guint8* data, guint32 length, GError** error) { + ERR_clear_error(); int retr = SSL_read(self->ssl_layer, data, length * sizeof(guint8)); if (retr <= 0) { - *error = err_from_ssl(); + // Every USB packet carries whole records and nothing else feeds the + // socket, so waiting for the rest of one would never end. + int ssl_error = SSL_get_error(self->ssl_layer, retr); + if (ssl_error == SSL_ERROR_WANT_READ || + ssl_error == SSL_ERROR_WANT_WRITE) + *error = g_error_new(G_IO_ERROR, G_IO_ERROR_PARTIAL_INPUT, + "Incomplete TLS record"); + else + *error = err_from_ssl("TLS read failed"); } return retr; } @@ -128,7 +147,8 @@ static void* goodix_tls_init_serve(void* me) int retr = SSL_accept(self->ssl_layer); fp_dbg("TLS server accept done"); if (retr <= 0) { - self->connection_callback(self, err_from_ssl(), self->user_data); + self->connection_callback(self, err_from_ssl("TLS handshake failed"), + self->user_data); } else { self->connection_callback(self, NULL, self->user_data); @@ -136,6 +156,23 @@ static void* goodix_tls_init_serve(void* me) return NULL; } +static gboolean set_nonblocking(int fd) +{ + int flags = fcntl(fd, F_GETFL); + + return flags != -1 && fcntl(fd, F_SETFL, flags | O_NONBLOCK) != -1; +} + +gboolean goodix_tls_server_set_nonblocking(GoodixTlsServer* self, GError** error) +{ + if (set_nonblocking(self->sock_fd) && set_nonblocking(self->client_fd)) + return TRUE; + + g_set_error(error, G_IO_ERROR, g_io_error_from_errno(errno), + "failed to make TLS sockets non-blocking: %s", g_strerror(errno)); + return FALSE; +} + gboolean goodix_tls_server_deinit(GoodixTlsServer* self, GError** error) { SSL_shutdown(self->ssl_layer); diff --git a/libfprint/drivers/goodixtls/goodixtls.h b/libfprint/drivers/goodixtls/goodixtls.h index a7d5b94..2a74eab 100644 --- a/libfprint/drivers/goodixtls/goodixtls.h +++ b/libfprint/drivers/goodixtls/goodixtls.h @@ -25,6 +25,9 @@ #define GOODIX_TLS_SERVER_PORT 4433 +// How long the handshake waits for the TLS server thread to produce output +#define GOODIX_TLS_IO_TIMEOUT_MS 2000 + // static const guint8 goodix_511_psk_0[64] = {0}; static const guint8 goodix_511_psk_0[] = { 0xba, 0x1a, 0x86, 0x03, 0x7c, 0x1d, 0x3c, 0x71, 0xc3, 0xaf, 0x34, @@ -76,6 +79,10 @@ gboolean goodix_tls_server_init(GoodixTlsServer* self, GError** error); gboolean goodix_tls_init_cli(GoodixTlsServer* self, GError** err); +// Called once the handshake is done. The sockets are only used from the main +// loop from then on, which must never block on them. +gboolean goodix_tls_server_set_nonblocking(GoodixTlsServer* self, GError** error); + // This can be called multiple times. It is called when the device send a TLS // packet. int goodix_tls_server_receive(GoodixTlsServer* self, guint8* data, From d74c2078f50b678933353e44bbb039766e8e875e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?La=C3=A9rcio=20de=20Sousa?= Date: Mon, 2 Nov 2026 11:59:33 -0300 Subject: [PATCH 2/6] goodixtls: don't let a dropped or unanswered command stall the SSM goodix_send_protocol() dropped a command when another was still running, without calling its callback, so the SSM waiting on it never moved again. And dev_cancel() made that reachable: it finished the action right after sending a fire-and-forget idle command, leaving up to GOODIX_TIMEOUT in which the next action's first command was lost. Report the busy state through the callback, and only finish a cancel on the idle command's ack or timeout. goodix_send_nop() also "completed" whichever command was running when its own send was refused; it now only completes its own. Commands with no timeout could wait forever on a silent MCU: the FDT switches, REQUEST_TLS_CONNECTION and the handshake reads. Give them all one. That also makes the FDT re-arm in on_fdt_down_second() reachable, which it never was: it looked for G_USB_DEVICE_ERROR_TIMED_OUT, while the command timeout reports G_IO_ERROR_TIMED_OUT. goodix_receive_done() no longer leaks an error nobody is waiting for. Co-Authored-By: Claude Opus 5.5 --- libfprint/drivers/goodixtls/goodix.c | 56 +++++++++++++++--------- libfprint/drivers/goodixtls/goodix.h | 5 ++- libfprint/drivers/goodixtls/goodix53xd.c | 35 +++++++++------ 3 files changed, 62 insertions(+), 34 deletions(-) diff --git a/libfprint/drivers/goodixtls/goodix.c b/libfprint/drivers/goodixtls/goodix.c index fec0d95..d505e3e 100644 --- a/libfprint/drivers/goodixtls/goodix.c +++ b/libfprint/drivers/goodixtls/goodix.c @@ -82,12 +82,18 @@ void goodix_receive_done(FpDevice *dev, guint8 *data, guint16 length, GoodixCmdCallback callback = priv->callback; gpointer user_data = priv->user_data; - if (!(priv->ack || priv->reply)) return; + if (!(priv->ack || priv->reply)) { + g_clear_error(&error); + return; + } goodix_reset_state(dev); if (!error) fp_dbg("Completed command: 0x%02x", priv->cmd); - if (callback) callback(dev, data, length, user_data, error); + if (callback) + callback(dev, data, length, user_data, error); + else + g_clear_error(&error); } void goodix_receive_none(FpDevice *dev, guint8 *data, guint16 length, @@ -487,7 +493,7 @@ gboolean goodix_send_pack(FpDevice * dev, guint8 flags, guint8 * payload, return goodix_send_data(dev, data, data_len, g_free, error); } -void goodix_send_protocol( +gboolean goodix_send_protocol( FpDevice * dev, guint8 cmd, guint8 * payload, guint16 length, GDestroyNotify free_func, gboolean calc_checksum, guint timeout_ms, gboolean reply, GoodixCmdCallback callback, gpointer user_data) @@ -504,7 +510,12 @@ void goodix_send_protocol( fp_warn("A command is already running: 0x%02x", priv->cmd); if (free_func) free_func(payload); - return; + if (callback) + callback(dev, NULL, 0, user_data, + g_error_new(G_IO_ERROR, G_IO_ERROR_BUSY, + "A command is already running: 0x%02x", + priv->cmd)); + return FALSE; } fp_dbg("Running command: 0x%02x", cmd); @@ -526,8 +537,9 @@ void goodix_send_protocol( if (!goodix_send_pack(dev, GOODIX_FLAGS_MSG_PROTOCOL, data, data_len, g_free, &error)) { goodix_receive_done(dev, NULL, 0, error); - return; + return FALSE; }; + return TRUE; } void goodix_send_nop(FpDevice *dev, GoodixNoneCallback callback, gpointer user_data) { @@ -540,16 +552,17 @@ void goodix_send_nop(FpDevice *dev, GoodixNoneCallback callback, cb_info->callback = G_CALLBACK(callback); cb_info->user_data = user_data; - goodix_send_protocol(dev, GOODIX_CMD_NOP, (guint8 *)&payload, - sizeof(payload), NULL, FALSE, GOODIX_TIMEOUT, FALSE, - goodix_receive_none, cb_info); - goodix_receive_done(dev, NULL, 0, NULL); + if (goodix_send_protocol(dev, GOODIX_CMD_NOP, (guint8 *)&payload, + sizeof(payload), NULL, FALSE, GOODIX_TIMEOUT, FALSE, + goodix_receive_none, cb_info)) + goodix_receive_done(dev, NULL, 0, NULL); return; } - goodix_send_protocol(dev, GOODIX_CMD_NOP, (guint8 *)&payload, sizeof(payload), - NULL, FALSE, GOODIX_TIMEOUT, FALSE, NULL, NULL); - goodix_receive_done(dev, NULL, 0, NULL); + if (goodix_send_protocol(dev, GOODIX_CMD_NOP, (guint8 *)&payload, + sizeof(payload), NULL, FALSE, GOODIX_TIMEOUT, FALSE, + NULL, NULL)) + goodix_receive_done(dev, NULL, 0, NULL); } void goodix_send_mcu_get_image(FpDevice* dev, guint8* payload, guint16 length, GoodixImageCallback callback, @@ -590,13 +603,13 @@ void goodix_send_mcu_switch_to_fdt_down(FpDevice *dev, guint8 *mode, cb_info->user_data = user_data; goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_DOWN, mode, length, - free_func, TRUE, 0, reply, goodix_receive_default, + free_func, TRUE, GOODIX_TIMEOUT, reply, goodix_receive_default, cb_info); return; } goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_DOWN, mode, length, - free_func, TRUE, 0, reply, NULL, NULL); + free_func, TRUE, GOODIX_TIMEOUT, reply, NULL, NULL); } void goodix_send_mcu_switch_to_fdt_up(FpDevice *dev, guint8 *mode, @@ -612,13 +625,13 @@ void goodix_send_mcu_switch_to_fdt_up(FpDevice *dev, guint8 *mode, cb_info->user_data = user_data; goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_UP, mode, length, - free_func, TRUE, 0, reply, goodix_receive_default, + free_func, TRUE, GOODIX_TIMEOUT, reply, goodix_receive_default, cb_info); return; } goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_UP, mode, length, - free_func, TRUE, 0, reply, NULL, NULL); + free_func, TRUE, GOODIX_TIMEOUT, reply, NULL, NULL); } void goodix_send_mcu_switch_to_fdt_mode(FpDevice *dev, guint8 *mode, @@ -635,13 +648,13 @@ void goodix_send_mcu_switch_to_fdt_mode(FpDevice *dev, guint8 *mode, cb_info->user_data = user_data; goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_MODE, mode, length, - free_func, TRUE, 0, reply, goodix_receive_default, + free_func, TRUE, GOODIX_TIMEOUT, reply, goodix_receive_default, cb_info); return; } goodix_send_protocol(dev, GOODIX_CMD_MCU_SWITCH_TO_FDT_MODE, mode, length, - free_func, TRUE, 0, reply, NULL, NULL); + free_func, TRUE, GOODIX_TIMEOUT, reply, NULL, NULL); } void goodix_send_nav_0(FpDevice *dev, GoodixDefaultCallback callback, @@ -898,8 +911,9 @@ void goodix_send_request_tls_connection(FpDevice* dev, cb_info->user_data = user_data; goodix_send_protocol(dev, GOODIX_CMD_REQUEST_TLS_CONNECTION, - (guint8*) &payload, sizeof(payload), NULL, TRUE, 0, - TRUE, goodix_receive_default, cb_info); + (guint8*) &payload, sizeof(payload), NULL, TRUE, + GOODIX_TLS_IO_TIMEOUT_MS, TRUE, + goodix_receive_default, cb_info); return; } @@ -1085,6 +1099,8 @@ void goodix_read_tls(FpDevice* dev, GoodixTlsCallback callback, FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); + priv->timeout = fpi_device_add_timeout(dev, GOODIX_TLS_IO_TIMEOUT_MS, + goodix_receive_timeout_cb, NULL, NULL); priv->callback = callback; priv->user_data = user_data; priv->reply = TRUE; diff --git a/libfprint/drivers/goodixtls/goodix.h b/libfprint/drivers/goodixtls/goodix.h index 1b75997..6052651 100644 --- a/libfprint/drivers/goodixtls/goodix.h +++ b/libfprint/drivers/goodixtls/goodix.h @@ -128,7 +128,10 @@ gboolean goodix_send_pack(FpDevice *dev, guint8 flags, guint8 *payload, guint16 length, GDestroyNotify free_func, GError **error); -void goodix_send_protocol(FpDevice *dev, guint8 cmd, guint8 *payload, +// Returns TRUE if the command went out. Otherwise, the callback has already +// been called with the error: a command that doesn't start must not leave its +// caller waiting. +gboolean goodix_send_protocol(FpDevice *dev, guint8 cmd, guint8 *payload, guint16 length, GDestroyNotify free_func, gboolean calc_checksum, guint timeout_ms, gboolean reply, GoodixCmdCallback callback, diff --git a/libfprint/drivers/goodixtls/goodix53xd.c b/libfprint/drivers/goodixtls/goodix53xd.c index ac0486b..b7d9b5d 100644 --- a/libfprint/drivers/goodixtls/goodix53xd.c +++ b/libfprint/drivers/goodixtls/goodix53xd.c @@ -585,11 +585,10 @@ static void on_fdt_down_second(FpDevice* dev, guint8* data, guint16 len, gpointer ssm, GError* err) { if (err) { - // Treat a transfer timeout as "no finger yet" and re-arm so the user - // gets more than one USB timeout window to press. (Needs hardware - // tuning; a real finger-up/finger-down FDT event would be cleaner.) - if (g_error_matches(err, G_USB_DEVICE_ERROR, - G_USB_DEVICE_ERROR_TIMED_OUT)) { + // Treat a command timeout as "no finger yet" and re-arm so the user + // gets more than one timeout window to press. (Needs hardware tuning; + // a real finger-up/finger-down FDT event would be cleaner.) + if (g_error_matches(err, G_IO_ERROR, G_IO_ERROR_TIMED_OUT)) { g_clear_error(&err); capture_fdt_down_arm(dev, ssm); return; @@ -1207,6 +1206,20 @@ static void dev_verify(FpDevice* dev) fpi_ssm_start(self->task_ssm, verify_ssm_done); } +static void on_cancel_idle_done(FpDevice* dev, gpointer user_data, GError* error) +{ + FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); + + if (error) { + fp_dbg("MCU didn't confirm idle mode on cancel: %s", error->message); + g_error_free(error); + } + if (self->task_ssm) + fpi_ssm_mark_failed(self->task_ssm, + g_error_new(G_IO_ERROR, G_IO_ERROR_CANCELLED, + "Cancelled")); +} + static void dev_cancel(FpDevice* dev) { FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); @@ -1233,14 +1246,10 @@ static void dev_cancel(FpDevice* dev) // fprintd process's life (confirmed in the field: only a full // fprintd restart recovers). Explicitly switching the MCU to idle // closes out any pending FDT sequence on its side before the next - // verify attempt starts. Fire-and-forget: priv state is already - // clear so it can't collide with the next command, and it - // self-resolves via ack-or-GOODIX_TIMEOUT either way. - goodix_send_mcu_switch_to_idle_mode(dev, 20, NULL, NULL); - - fpi_ssm_mark_failed(self->task_ssm, - g_error_new(G_IO_ERROR, G_IO_ERROR_CANCELLED, - "Cancelled")); + // verify attempt starts. The action only ends on its ack or + // GOODIX_TIMEOUT: the next action's first command would otherwise + // find this one still running. + goodix_send_mcu_switch_to_idle_mode(dev, 20, on_cancel_idle_done, NULL); } } From 7aa0960170be87f7247cdffcf27ad545e3205f59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?La=C3=A9rcio=20de=20Sousa?= Date: Mon, 2 Nov 2026 12:00:32 -0300 Subject: [PATCH 3/6] goodixtls: report TLS failures and clean up after a failed open Most TLS failures were only logged: the open SSM then waited forever (server init, REQUEST_TLS_CONNECTION), or went on as if nothing had happened (failed handshake). goodix_tls() now always reports back to its caller, exactly once. A failed open didn't release anything, as dev_close() isn't called for it: the interface stayed claimed, the read loop running and tls_hop set, and the next open aborted fprintd on g_assert(priv->tls_hop == NULL). Clean up in open_ssm_done(). goodix_dev_deinit() destroyed a pending timeout twice, the second time through a pointer that may have been freed by the first. With the read loop's token replaced on reopen, a cancelled transfer could also be mistaken for a failed one and start a second loop: check its error instead. The TLS server thread was never joined, leaking its stack on each open, and SSL_free() could run under an SSL_accept() in progress. Stop it with end of file on the socket, and join it before freeing anything it uses. Co-Authored-By: Claude Opus 5.5 --- libfprint/drivers/goodixtls/goodix.c | 79 +++++++++++++++--------- libfprint/drivers/goodixtls/goodix53xd.c | 9 +++ libfprint/drivers/goodixtls/goodixtls.c | 55 +++++++++++++---- libfprint/drivers/goodixtls/goodixtls.h | 4 +- 4 files changed, 105 insertions(+), 42 deletions(-) diff --git a/libfprint/drivers/goodixtls/goodix.c b/libfprint/drivers/goodixtls/goodix.c index d505e3e..96c8eab 100644 --- a/libfprint/drivers/goodixtls/goodix.c +++ b/libfprint/drivers/goodixtls/goodix.c @@ -378,8 +378,11 @@ void goodix_receive_data_cb(FpiUsbTransfer *transfer, FpDevice *dev, FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); - if (g_cancellable_is_cancelled(priv->transfer_cancel_tkn)) { + // Match the error rather than priv->transfer_cancel_tkn: by now that may + // be gone, or a new one if the device was reopened. + if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { fp_dbg("transfer cancelled, aborting read loop..."); + g_error_free(error); return; } if (error) { @@ -1048,6 +1051,7 @@ gboolean goodix_dev_init(FpDevice *dev, GError **error) { priv->user_data = NULL; priv->data = NULL; priv->length = 0; + g_clear_object(&priv->transfer_cancel_tkn); priv->transfer_cancel_tkn = g_cancellable_new(); return g_usb_device_claim_interface(fpi_device_get_usb_device(dev), @@ -1073,10 +1077,12 @@ gboolean goodix_dev_deinit(FpDevice *dev, GError **error) { FpiDeviceGoodixTlsPrivate *priv = fpi_device_goodixtls_get_instance_private(self); - if (priv->timeout) g_source_destroy(priv->timeout); - g_free(priv->data); + g_clear_pointer(&priv->data, g_free); + priv->length = 0; g_cancellable_cancel(priv->transfer_cancel_tkn); + g_clear_object(&priv->transfer_cancel_tkn); goodix_shutdown_tls(dev, error); + g_clear_pointer(&priv->tls_ready_callback, g_free); goodix_reset_state(dev); priv->inited = FALSE; @@ -1116,8 +1122,12 @@ enum tls_states { static void on_goodix_tls_server_ready(GoodixTlsServer* server, GError* err, gpointer dev) { + // Runs on the server thread. A failure here is also seen by the main loop, + // as a handshake step that errors out or times out; and it's expected when + // the server is torn down in the middle of a handshake. if (err) { - fp_err("server ready failed: %s", err->message); + fp_dbg("TLS server stopped: %s", err->message); + g_error_free(err); return; } fp_dbg("TLS connection ready"); @@ -1159,6 +1169,22 @@ enum goodix_tls_handshake_stages { TLS_HANDSHAKE_STAGE_NUM, }; +// Tell goodix_tls()'s caller how it went: exactly once, whichever way it fails. +static void goodix_tls_complete(FpDevice* dev, GError* error) +{ + FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); + FpiDeviceGoodixTlsPrivate* priv = + fpi_device_goodixtls_get_instance_private(self); + g_autofree GoodixCallbackInfo* info = + g_steal_pointer(&priv->tls_ready_callback); + + if (!info) { + g_clear_error(&error); + return; + } + ((GoodixNoneCallback) info->callback)(dev, info->user_data, error); +} + static void on_tls_successfully_established(FpDevice* dev, gpointer user_data, GError* error) { @@ -1168,14 +1194,15 @@ static void on_tls_successfully_established(FpDevice* dev, gpointer user_data, fpi_device_goodixtls_get_instance_private(self); if (!error) goodix_tls_server_set_nonblocking(priv->tls_hop, &error); - ((GoodixNoneCallback) priv->tls_ready_callback->callback)( - dev, priv->tls_ready_callback->user_data, error); + goodix_tls_complete(dev, error); } static void tls_handshake_done(FpiSsm* ssm, FpDevice* dev, GError* error) { if (error) { fp_dbg("failed to do tls handshake: %s (code: %d)", error->message, error->code); + goodix_tls_complete(dev, error); + return; } goodix_send_tls_successfully_established( dev, on_tls_successfully_established, NULL); @@ -1240,28 +1267,22 @@ static void on_goodix_request_tls_connection(FpDevice* dev, guint8* data, GError* error) { if (error) { - fp_err("failed to get tls handshake: %s", error->message); - goodix_send_tls_successfully_established(FP_DEVICE(dev), NULL, NULL); + goodix_tls_complete(dev, error); return; } FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(user_data); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); - goodix_tls_client_send(priv->tls_hop, data, length); - - do_tls_handshake(dev); -} - -static void goodix_tls_ready(GoodixTlsServer* server, GError* err, gpointer dev) -{ - if (err) { - fp_err("failed to init tls server: %s, code: %d", err->message, - err->code); + if (goodix_tls_client_send(priv->tls_hop, data, length) < 0) { + goodix_tls_complete(dev, + g_error_new(G_IO_ERROR, g_io_error_from_errno(errno), + "failed to pass the client hello to the " + "TLS server: %s", g_strerror(errno))); return; } - goodix_send_request_tls_connection(FP_DEVICE(dev), - on_goodix_request_tls_connection, dev); + + do_tls_handshake(dev); } void goodix_tls(FpDevice* dev, GoodixNoneCallback callback, gpointer user_data) @@ -1270,12 +1291,14 @@ void goodix_tls(FpDevice* dev, GoodixNoneCallback callback, gpointer user_data) FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); FpiDeviceGoodixTlsPrivate* priv = fpi_device_goodixtls_get_instance_private(self); - g_assert(priv->tls_hop == NULL); - priv->tls_hop = malloc(sizeof(GoodixTlsServer)); - - if (!priv->tls_ready_callback) { - priv->tls_ready_callback = malloc(sizeof(GoodixCallbackInfo)); + if (priv->tls_hop) { + fp_warn("A TLS server is still around, replacing it"); + goodix_shutdown_tls(dev, NULL); } + priv->tls_hop = g_new0(GoodixTlsServer, 1); + + g_clear_pointer(&priv->tls_ready_callback, g_free); + priv->tls_ready_callback = g_new0(GoodixCallbackInfo, 1); priv->tls_ready_callback->callback = G_CALLBACK(callback); priv->tls_ready_callback->user_data = user_data; GoodixTlsServer* s = priv->tls_hop; @@ -1283,12 +1306,12 @@ void goodix_tls(FpDevice* dev, GoodixNoneCallback callback, gpointer user_data) s->user_data = self; GError* err = NULL; if (!goodix_tls_server_init(priv->tls_hop, &err)) { - fp_err("failed to init tls server, error: %s, code: %d", err->message, - err->code); + goodix_tls_complete(dev, err); return; } - goodix_tls_ready(s, err, self); + goodix_send_request_tls_connection(dev, on_goodix_request_tls_connection, + self); } gboolean goodix_shutdown_tls(FpDevice* dev, GError** error) diff --git a/libfprint/drivers/goodixtls/goodix53xd.c b/libfprint/drivers/goodixtls/goodix53xd.c index b7d9b5d..12bde1c 100644 --- a/libfprint/drivers/goodixtls/goodix53xd.c +++ b/libfprint/drivers/goodixtls/goodix53xd.c @@ -403,6 +403,15 @@ static void open_ssm_done(FpiSsm* ssm, FpDevice* dev, GError* error) FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); self->task_ssm = NULL; + if (error) { + // A failed open doesn't get a dev_close(). + g_autoptr(GError) deinit_error = NULL; + + g_clear_pointer(&self->otp, g_free); + g_clear_pointer(&self->calib, g_array_unref); + if (!goodix_dev_deinit(dev, &deinit_error)) + fp_dbg("cleanup after failed open: %s", deinit_error->message); + } fpi_device_open_complete(dev, error); } diff --git a/libfprint/drivers/goodixtls/goodixtls.c b/libfprint/drivers/goodixtls/goodixtls.c index a47dba6..ad363ba 100644 --- a/libfprint/drivers/goodixtls/goodixtls.c +++ b/libfprint/drivers/goodixtls/goodixtls.c @@ -175,13 +175,28 @@ gboolean goodix_tls_server_set_nonblocking(GoodixTlsServer* self, GError** error gboolean goodix_tls_server_deinit(GoodixTlsServer* self, GError** error) { - SSL_shutdown(self->ssl_layer); - SSL_free(self->ssl_layer); + // End of file gets a handshake still waiting for its client out of + // SSL_accept(), and then it can't use ssl_layer anymore. + if (self->client_fd >= 0) + shutdown(self->client_fd, SHUT_WR); + if (self->serve_thread_started) + pthread_join(self->serve_thread, NULL); + + if (self->ssl_layer) { + // Nobody reads the close_notify, don't wait for room for it. + set_nonblocking(self->sock_fd); + SSL_shutdown(self->ssl_layer); + SSL_free(self->ssl_layer); + ERR_clear_error(); + } - close(self->client_fd); - close(self->sock_fd); + if (self->client_fd >= 0) + close(self->client_fd); + if (self->sock_fd >= 0) + close(self->sock_fd); - SSL_CTX_free(self->ssl_ctx); + if (self->ssl_ctx) + SSL_CTX_free(self->ssl_ctx); return TRUE; } @@ -189,10 +204,20 @@ gboolean goodix_tls_server_deinit(GoodixTlsServer* self, GError** error) gboolean goodix_tls_server_init(GoodixTlsServer* self, GError** error) { g_assert(self->connection_callback); + self->sock_fd = -1; + self->client_fd = -1; SSL_load_error_strings(); OpenSSL_add_ssl_algorithms(); SSL_library_init(); self->ssl_ctx = tls_server_create_ctx(); + if (self->ssl_ctx == NULL) { + fp_dbg("Unable to create TLS server context\n"); + *error = fpi_device_error_new_msg(FP_DEVICE_ERROR_GENERAL, "Unable to " + "create TLS " + "server " + "context"); + return FALSE; + } tls_server_config_ctx(self->ssl_ctx); int socks[2] = {0, 0}; @@ -204,19 +229,23 @@ gboolean goodix_tls_server_init(GoodixTlsServer* self, GError** error) self->sock_fd = socks[0]; self->client_fd = socks[1]; - if (self->ssl_ctx == NULL) { - fp_dbg("Unable to create TLS server context\n"); - *error = fpi_device_error_new_msg(FP_DEVICE_ERROR_GENERAL, "Unable to " - "create TLS " - "server " - "context"); + self->ssl_layer = SSL_new(self->ssl_ctx); + if (self->ssl_layer == NULL) { + *error = err_from_ssl("failed to create the TLS connection"); return FALSE; } - self->ssl_layer = SSL_new(self->ssl_ctx); tls_config_ssl(self->ssl_layer); SSL_set_fd(self->ssl_layer, self->sock_fd); - pthread_create(&self->serve_thread, 0, goodix_tls_init_serve, self); + int thread_error = + pthread_create(&self->serve_thread, 0, goodix_tls_init_serve, self); + if (thread_error != 0) { + g_set_error(error, G_IO_ERROR, g_io_error_from_errno(thread_error), + "failed to start the TLS server thread: %s", + g_strerror(thread_error)); + return FALSE; + } + self->serve_thread_started = TRUE; return TRUE; } diff --git a/libfprint/drivers/goodixtls/goodixtls.h b/libfprint/drivers/goodixtls/goodixtls.h index 2a74eab..710b3de 100644 --- a/libfprint/drivers/goodixtls/goodixtls.h +++ b/libfprint/drivers/goodixtls/goodixtls.h @@ -71,6 +71,7 @@ typedef struct _GoodixTlsServer { // SSL* cli_ssl_layer; int client_fd; pthread_t serve_thread; + gboolean serve_thread_started; } GoodixTlsServer; // This is called only once to init the TLS server. @@ -92,6 +93,7 @@ int goodix_tls_client_send(GoodixTlsServer* self, guint8* data, guint16 length); int goodix_tls_client_recv(GoodixTlsServer* self, guint8* data, guint16 length); -// This is called only once to deinit the TLS server. +// This is called only once to deinit the TLS server, even if init failed or +// the handshake is still running. // Return TRUE on success, FALSE otherwise and error should be set. gboolean goodix_tls_server_deinit(GoodixTlsServer* self, GError** error); From 8e0c5017136980a86ec027c8313de0c6736b00a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?La=C3=A9rcio=20de=20Sousa?= Date: Mon, 2 Nov 2026 12:01:31 -0300 Subject: [PATCH 4/6] goodixtls53xd: keep the TLS session in sync across a cancel Each image is a TLS record, and the records are numbered. A cancel with an image on its way made the driver forget about the command, so the record was dropped instead of reaching OpenSSL; every capture after that failed on a bad MAC until the device was reopened. Polling for a finger captures all the time, so this was the likely case for a cancel rather than a rare one. When an image is in flight, dev_cancel() now lets on_capture_image() consume it and finishes the cancel from there, which the command's timeout guarantees. If an image reply arrives after its command timed out, it could also be handed to the next command, because TLS data wasn't checked against the current one. Ignore it. Partially assembled packets are also dropped when their command ends, and a TLS data message too short for its header is an error instead of a read past the end of the buffer. Co-Authored-By: Claude Opus 5.5 --- libfprint/drivers/goodixtls/goodix.c | 25 +++++++++ libfprint/drivers/goodixtls/goodix.h | 3 ++ libfprint/drivers/goodixtls/goodix53xd.c | 64 +++++++++++++++++------- 3 files changed, 75 insertions(+), 17 deletions(-) diff --git a/libfprint/drivers/goodixtls/goodix.c b/libfprint/drivers/goodixtls/goodix.c index 96c8eab..e4070d3 100644 --- a/libfprint/drivers/goodixtls/goodix.c +++ b/libfprint/drivers/goodixtls/goodix.c @@ -359,7 +359,20 @@ void goodix_receive_pack(FpDevice *dev, guint8 *data, guint32 length) { case GOODIX_FLAGS_TLS_DATA: fp_dbg("Got TLS data msg"); + // A reply left over from a command that timed out must not be taken + // for the answer to the current one. + if (priv->cmd != GOODIX_CMD_MCU_GET_IMAGE) { + fp_warn("Unexpected TLS data during command: 0x%02x", priv->cmd); + break; + } // GOODIX 52xd: Remove first 9 to get valid TLS content + if (payload_len < 9) { + goodix_receive_done(dev, NULL, 0, + g_error_new(G_IO_ERROR, G_IO_ERROR_INVALID_DATA, + "Short TLS data message: %d", + payload_len)); + break; + } goodix_receive_done(dev, payload+9, payload_len-9, NULL); break; @@ -1069,6 +1082,18 @@ void goodix_reset_state(FpDevice* dev) priv->reply = FALSE; priv->callback = NULL; priv->user_data = NULL; + // Whatever was being assembled belongs to the command that just ended. + g_clear_pointer(&priv->data, g_free); + priv->length = 0; +} + +gboolean goodix_command_in_flight(FpDevice* dev, guint8 cmd) +{ + FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); + FpiDeviceGoodixTlsPrivate* priv = + fpi_device_goodixtls_get_instance_private(self); + + return priv->cmd == cmd && (priv->ack || priv->reply); } gboolean goodix_dev_deinit(FpDevice *dev, GError **error) { diff --git a/libfprint/drivers/goodixtls/goodix.h b/libfprint/drivers/goodixtls/goodix.h index 6052651..24c70eb 100644 --- a/libfprint/drivers/goodixtls/goodix.h +++ b/libfprint/drivers/goodixtls/goodix.h @@ -232,6 +232,9 @@ gboolean goodix_dev_deinit(FpDevice *dev, GError **error); void goodix_reset_state(FpDevice* dev); +// Whether cmd was sent and its ack or reply are still awaited. +gboolean goodix_command_in_flight(FpDevice* dev, guint8 cmd); + // ---- DEV SECTION END ---- // ----------------------------------------------------------------------------- diff --git a/libfprint/drivers/goodixtls/goodix53xd.c b/libfprint/drivers/goodixtls/goodix53xd.c index 12bde1c..e01d849 100644 --- a/libfprint/drivers/goodixtls/goodix53xd.c +++ b/libfprint/drivers/goodixtls/goodix53xd.c @@ -120,6 +120,9 @@ struct _FpiDeviceGoodixTls53XD { // before calling fpi_ssm_mark_failed(), which asserts none is pending. gboolean poll_timeout_pending; + // TRUE from dev_cancel() until the action has actually been failed. + gboolean cancel_pending; + // Deferred verify/identify result reporting gboolean verify_wait_finger_up; gboolean pending_result_report; @@ -508,9 +511,18 @@ static void dump_empty_sample(FpiDeviceGoodixTls53XD* self) dump_sample(self, "empty"); } +static void finish_cancel(FpDevice* dev); + static void on_capture_image(FpDevice* dev, guint8* data, guint16 len, gpointer ssm, GError* err) { + FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); + + if (self->cancel_pending) { + g_clear_error(&err); + finish_cancel(dev); + return; + } if (err) { fpi_ssm_mark_failed(ssm, err); return; @@ -522,7 +534,6 @@ static void on_capture_image(FpDevice* dev, guint8* data, guint16 len, return; } - FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); Goodix53xdPix* frame = self->captured_raw; decode_frame(frame, data); @@ -1131,6 +1142,8 @@ static void dev_open(FpDevice* dev) FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); GError* error = NULL; + self->cancel_pending = FALSE; + g_autofree char* path = calib_path(); g_autoptr(GError) calib_error = NULL; g_clear_pointer(&self->calib, g_array_unref); @@ -1174,6 +1187,7 @@ static void dev_enroll(FpDevice* dev) { FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); + self->cancel_pending = FALSE; self->enroll_stage = 0; g_clear_pointer(&self->enroll_images, g_ptr_array_unref); self->enroll_images = g_ptr_array_new_with_free_func(g_free); @@ -1210,6 +1224,7 @@ static void dev_verify(FpDevice* dev) clear_pending_result(self); self->action_result_reported = FALSE; self->verify_wait_finger_up = FALSE; + self->cancel_pending = FALSE; self->task_ssm = fpi_ssm_new(dev, verify_run_state, VERIFY_NUM_STATES); fpi_ssm_start(self->task_ssm, verify_ssm_done); @@ -1223,12 +1238,33 @@ static void on_cancel_idle_done(FpDevice* dev, gpointer user_data, GError* error fp_dbg("MCU didn't confirm idle mode on cancel: %s", error->message); g_error_free(error); } + self->cancel_pending = FALSE; if (self->task_ssm) fpi_ssm_mark_failed(self->task_ssm, g_error_new(G_IO_ERROR, G_IO_ERROR_CANCELLED, "Cancelled")); } +static void finish_cancel(FpDevice* dev) +{ + goodix_reset_state(dev); + + // A FDT_DOWN "wait for finger" (0x32) may still be outstanding on + // the MCU itself when we get here: goodix_reset_state() only clears + // our own ack/reply/timeout bookkeeping, it can't un-send bytes + // already on the wire. Left alone, the MCU eventually replies to + // that stale FDT_DOWN once a *new* command is already in flight, and + // its cmd byte (0x32) no longer matches priv->cmd -> permanent + // "Invalid protocol/ACK command" desync for the rest of this + // fprintd process's life (confirmed in the field: only a full + // fprintd restart recovers). Explicitly switching the MCU to idle + // closes out any pending FDT sequence on its side before the next + // verify attempt starts. The action only ends on its ack or + // GOODIX_TIMEOUT: the next action's first command would otherwise + // find this one still running. + goodix_send_mcu_switch_to_idle_mode(dev, 20, on_cancel_idle_done, NULL); +} + static void dev_cancel(FpDevice* dev) { FpiDeviceGoodixTls53XD* self = FPI_DEVICE_GOODIXTLS53XD(dev); @@ -1243,22 +1279,16 @@ static void dev_cancel(FpDevice* dev) fpi_ssm_cancel_delayed_state_change(self->task_ssm); self->poll_timeout_pending = FALSE; } - goodix_reset_state(dev); - - // A FDT_DOWN "wait for finger" (0x32) may still be outstanding on - // the MCU itself when we get here: goodix_reset_state() only clears - // our own ack/reply/timeout bookkeeping, it can't un-send bytes - // already on the wire. Left alone, the MCU eventually replies to - // that stale FDT_DOWN once a *new* command is already in flight, and - // its cmd byte (0x32) no longer matches priv->cmd -> permanent - // "Invalid protocol/ACK command" desync for the rest of this - // fprintd process's life (confirmed in the field: only a full - // fprintd restart recovers). Explicitly switching the MCU to idle - // closes out any pending FDT sequence on its side before the next - // verify attempt starts. The action only ends on its ack or - // GOODIX_TIMEOUT: the next action's first command would otherwise - // find this one still running. - goodix_send_mcu_switch_to_idle_mode(dev, 20, on_cancel_idle_done, NULL); + self->cancel_pending = TRUE; + + // Every image is a TLS record, numbered in sequence: dropping the one + // on its way would fail all the captures after it, until the device + // is reopened. Let on_capture_image() take it to the TLS server and + // carry on from there; the command's timeout guarantees it gets called. + if (goodix_command_in_flight(dev, GOODIX_CMD_MCU_GET_IMAGE)) + return; + + finish_cancel(dev); } } From 3f5be4a6b879cbfc94be1444d64dde927f9d20f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?La=C3=A9rcio=20de=20Sousa?= Date: Mon, 2 Nov 2026 12:01:31 -0300 Subject: [PATCH 5/6] goodixtls: fix a per-frame leak and the read loop spinning on errors goodix_tls_ready_image_handler() never freed its 64 KiB buffer. Polling for a finger runs this for every frame, for as long as a verify is active, and with fprintd --no-timeout the memory never came back. goodix_receive_data_cb() resubmitted a failed transfer at once, forever. Pause between attempts, and stop on G_USB_DEVICE_ERROR_NO_DEVICE, failing the command waiting for a reply since none can arrive. Co-Authored-By: Claude Opus 5.5 --- libfprint/drivers/goodixtls/goodix.c | 26 ++++++++++++++++++++++---- libfprint/drivers/goodixtls/goodix.h | 3 +++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/libfprint/drivers/goodixtls/goodix.c b/libfprint/drivers/goodixtls/goodix.c index e4070d3..c70169d 100644 --- a/libfprint/drivers/goodixtls/goodix.c +++ b/libfprint/drivers/goodixtls/goodix.c @@ -55,6 +55,7 @@ typedef struct { GoodixCallbackInfo* tls_ready_callback; GCancellable* transfer_cancel_tkn; + GSource* read_retry; gboolean inited; } FpiDeviceGoodixTlsPrivate; @@ -385,6 +386,15 @@ void goodix_receive_pack(FpDevice *dev, guint8 *data, guint32 length) { priv->length = 0; } +static void goodix_receive_retry_cb(FpDevice *dev, gpointer user_data) { + FpiDeviceGoodixTls *self = FPI_DEVICE_GOODIXTLS(dev); + FpiDeviceGoodixTlsPrivate *priv = + fpi_device_goodixtls_get_instance_private(self); + + priv->read_retry = NULL; + goodix_receive_data(dev); +} + void goodix_receive_data_cb(FpiUsbTransfer *transfer, FpDevice *dev, gpointer user_data, GError *error) { FpiDeviceGoodixTls* self = FPI_DEVICE_GOODIXTLS(dev); @@ -399,12 +409,19 @@ void goodix_receive_data_cb(FpiUsbTransfer *transfer, FpDevice *dev, return; } if (error) { - // Warn about error and free it. fp_warn("Receive data error: %s", error->message); + + // There will be nothing else to read. Fail what's waiting for a reply. + if (g_error_matches(error, G_USB_DEVICE_ERROR, G_USB_DEVICE_ERROR_NO_DEVICE)) { + priv->inited = FALSE; + goodix_receive_done(dev, NULL, 0, error); + return; + } g_error_free(error); - // Retry receiving data and return. - goodix_receive_data(dev); + // Retry receiving data, but don't spin on an error that persists. + priv->read_retry = fpi_device_add_timeout( + dev, GOODIX_READ_RETRY_MS, goodix_receive_retry_cb, NULL, NULL); return; } @@ -1102,6 +1119,7 @@ gboolean goodix_dev_deinit(FpDevice *dev, GError **error) { FpiDeviceGoodixTlsPrivate *priv = fpi_device_goodixtls_get_instance_private(self); + g_clear_pointer(&priv->read_retry, g_source_destroy); g_clear_pointer(&priv->data, g_free); priv->length = 0; g_cancellable_cancel(priv->transfer_cancel_tkn); @@ -1378,7 +1396,7 @@ static void goodix_tls_ready_image_handler(FpDevice* dev, guint8* data, } const guint16 size = -1; - guint8* buff = malloc(size); + g_autofree guint8* buff = g_malloc(size); GError* err = NULL; int read_size = goodix_tls_server_receive(priv->tls_hop, buff, size, &err); if (read_size <= 0) { diff --git a/libfprint/drivers/goodixtls/goodix.h b/libfprint/drivers/goodixtls/goodix.h index 24c70eb..0345535 100644 --- a/libfprint/drivers/goodixtls/goodix.h +++ b/libfprint/drivers/goodixtls/goodix.h @@ -22,6 +22,9 @@ // 1 seconds USB timeout #define GOODIX_TIMEOUT (1000) +// Pause before reading again after a failed USB transfer +#define GOODIX_READ_RETRY_MS (100) + G_DECLARE_DERIVABLE_TYPE(FpiDeviceGoodixTls, fpi_device_goodixtls, FPI, DEVICE_GOODIXTLS, FpDevice) From 6de0f6643bc5e10583541955b8aec16c4092f39c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?La=C3=A9rcio=20de=20Sousa?= Date: Mon, 2 Nov 2026 12:05:25 -0300 Subject: [PATCH 6/6] tests: cover the goodixtls TLS server's failure paths Incomplete, corrupt and missing records must come back as errors instead of blocking the main loop, and the server must be able to go away at any point of a handshake. These run without a sensor; the command and cancel logic in the driver still needs one. Co-Authored-By: Claude Opus 5.5 --- tests/meson.build | 23 ++++ tests/test-goodixtls-server.c | 235 ++++++++++++++++++++++++++++++++++ 2 files changed, 258 insertions(+) create mode 100644 tests/test-goodixtls-server.c diff --git a/tests/meson.build b/tests/meson.build index aad0675..c8f8f5a 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -321,6 +321,29 @@ if 'virtual_image' in drivers ] endif +if 'goodixtls53xd' in drivers + # Built from the driver's source: the drivers aren't in a library that + # tests can link to, and this part doesn't need a device. + goodixtls_server_test = executable('test-goodixtls-server', + sources: [ + 'test-goodixtls-server.c', + '../libfprint/drivers/goodixtls/goodixtls.c', + ], + include_directories: include_directories( + '../libfprint/drivers', + '../libfprint/drivers/goodixtls', + ), + dependencies: [ libfprint_private_dep, openssl_dep, threads_dep ], + c_args: common_cflags, + install: false, + ) + test('goodixtls-server', + goodixtls_server_test, + suite: ['unit-tests'], + env: envs, + ) +endif + unit_tests_deps = { 'fpi-assembling' : [cairo_dep] } foreach test_name: unit_tests diff --git a/tests/test-goodixtls-server.c b/tests/test-goodixtls-server.c new file mode 100644 index 0000000..cad1ba3 --- /dev/null +++ b/tests/test-goodixtls-server.c @@ -0,0 +1,235 @@ +/* + * Unit tests for the goodixtls TLS server: what the main loop gets when the + * data coming from the sensor is not what it should be. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "goodixtls.h" + +/* A test that blocks on a socket would otherwise hang instead of failing. */ +#define HANG_LIMIT_SECONDS 30 + +typedef struct +{ + GoodixTlsServer server; + SSL_CTX *client_ctx; + SSL *client; +} Fixture; + +static void +on_server_ready (GoodixTlsServer *server, GError *error, gpointer user_data) +{ + g_clear_error (&error); +} + +static unsigned int +client_psk_cb (SSL *ssl, const char *hint, char *identity, + unsigned int max_identity_len, unsigned char *psk, + unsigned int max_psk_len) +{ + g_assert_cmpuint (max_psk_len, >=, 32); + memset (psk, 0, 32); + g_strlcpy (identity, "test", max_identity_len); + return 32; +} + +static void +fixture_init_server (Fixture *f) +{ + g_autoptr(GError) error = NULL; + + memset (f, 0, sizeof (*f)); + f->server.connection_callback = on_server_ready; + g_assert_true (goodix_tls_server_init (&f->server, &error)); + g_assert_no_error (error); +} + +static void +fixture_setup_connected (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + + fixture_init_server (f); + + f->client_ctx = SSL_CTX_new (TLS_client_method ()); + g_assert_nonnull (f->client_ctx); + SSL_CTX_set_min_proto_version (f->client_ctx, TLS1_2_VERSION); + SSL_CTX_set_max_proto_version (f->client_ctx, TLS1_2_VERSION); + g_assert_cmpint (SSL_CTX_set_cipher_list (f->client_ctx, "PSK"), ==, 1); + SSL_CTX_set_psk_client_callback (f->client_ctx, client_psk_cb); + + f->client = SSL_new (f->client_ctx); + SSL_set_fd (f->client, f->server.client_fd); + g_assert_cmpint (SSL_connect (f->client), ==, 1); + + g_assert_true (goodix_tls_server_set_nonblocking (&f->server, &error)); + g_assert_no_error (error); +} + +static void +fixture_teardown (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + + g_assert_true (goodix_tls_server_deinit (&f->server, &error)); + g_assert_no_error (error); + g_clear_pointer (&f->client, SSL_free); + g_clear_pointer (&f->client_ctx, SSL_CTX_free); +} + +static void +test_whole_record (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + const char message[] = "image"; + guint8 buffer[64]; + + g_assert_cmpint (SSL_write (f->client, message, sizeof (message)), ==, sizeof (message)); + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + ==, sizeof (message)); + g_assert_no_error (error); + g_assert_cmpstr ((const char *) buffer, ==, message); +} + +/* This used to block forever, and fprintd with it. */ +static void +test_incomplete_record (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + const guint8 start_of_record[] = { 0x17, 0x03, 0x03, 0x10, 0x00, 0xaa }; + guint8 buffer[64]; + + g_assert_cmpint (goodix_tls_client_send (&f->server, (guint8 *) start_of_record, + sizeof (start_of_record)), + ==, sizeof (start_of_record)); + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + <=, 0); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_PARTIAL_INPUT); +} + +static void +test_nothing_to_read (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + guint8 buffer[64]; + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + <=, 0); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_PARTIAL_INPUT); +} + +static void +test_end_of_file (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + guint8 buffer[64]; + + g_assert_cmpint (shutdown (f->server.client_fd, SHUT_WR), ==, 0); + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + <=, 0); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED); + g_assert_nonnull (error->message); +} + +/* A record the TLS session can't authenticate, as after one was dropped. */ +static void +test_corrupt_record (Fixture *f, gconstpointer data) +{ + g_autoptr(GError) error = NULL; + guint8 record[5 + 64] = { 0x17, 0x03, 0x03, 0x00, 64 }; + guint8 buffer[64]; + + g_assert_cmpint (goodix_tls_client_send (&f->server, record, sizeof (record)), + ==, sizeof (record)); + + g_assert_cmpint (goodix_tls_server_receive (&f->server, buffer, sizeof (buffer), &error), + <=, 0); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED); +} + +static void +test_client_recv_timeout (Fixture *f, gconstpointer data) +{ + guint8 buffer[64]; + + errno = 0; + g_assert_cmpint (goodix_tls_client_recv (&f->server, buffer, sizeof (buffer)), ==, -1); + g_assert_cmpint (errno, ==, ETIMEDOUT); +} + +/* The server thread is waiting for a client hello that will never come. */ +static void +test_deinit_during_handshake (void) +{ + g_autoptr(GError) error = NULL; + Fixture f; + + fixture_init_server (&f); + + g_assert_true (goodix_tls_server_deinit (&f.server, &error)); + g_assert_no_error (error); +} + +static void +test_deinit_mid_handshake (void) +{ + g_autoptr(GError) error = NULL; + const guint8 start_of_hello[] = { 0x16, 0x03, 0x01, 0x10, 0x00, 0x01 }; + Fixture f; + + fixture_init_server (&f); + g_assert_cmpint (goodix_tls_client_send (&f.server, (guint8 *) start_of_hello, + sizeof (start_of_hello)), + ==, sizeof (start_of_hello)); + + g_assert_true (goodix_tls_server_deinit (&f.server, &error)); + g_assert_no_error (error); +} + +#define CONNECTED_TEST(path, func) \ + g_test_add (path, Fixture, NULL, fixture_setup_connected, func, fixture_teardown) + +int +main (int argc, char *argv[]) +{ + g_test_init (&argc, &argv, NULL); + + alarm (HANG_LIMIT_SECONDS); + + CONNECTED_TEST ("/goodixtls/receive/whole-record", test_whole_record); + CONNECTED_TEST ("/goodixtls/receive/incomplete-record", test_incomplete_record); + CONNECTED_TEST ("/goodixtls/receive/nothing-to-read", test_nothing_to_read); + CONNECTED_TEST ("/goodixtls/receive/end-of-file", test_end_of_file); + CONNECTED_TEST ("/goodixtls/receive/corrupt-record", test_corrupt_record); + CONNECTED_TEST ("/goodixtls/client-recv/timeout", test_client_recv_timeout); + g_test_add_func ("/goodixtls/deinit/during-handshake", test_deinit_during_handshake); + g_test_add_func ("/goodixtls/deinit/mid-handshake", test_deinit_mid_handshake); + + return g_test_run (); +}