Repository navigation
goodixtls: fix hangs, cancel desync and failed-open cleanup - #4
Merged
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
lbssousa
marked this pull request as ready for review
November 2, 2026 15:22
This was referenced Nov 2, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes ways the
goodixtls53xddriver could hangfprintd, stall an action or lose its TLS session. Found by reading the code while investigating lock screen freezes.This is not a confirmed fix for those freezes. In the one incident still in the journal (2026-11-01), no fingerprint had been enrolled yet, so the sensor had nothing to verify. These are real bugs worth fixing either way, but this PR shouldn't be expected to end the
gdm-session-workerfloods.Changes
SSL_read()on an incomplete record blockedfprintd's main thread forever. The sockets are now non-blocking after the handshake, and handshake reads have a timeout.err_from_ssl()no longer builds aGErrorwithout a domain.dev_cancel()made that reachable, so it now waits for the idle command's ack or timeout. The FDT commands,REQUEST_TLS_CONNECTIONand handshake reads get timeouts, and the FDT re-arm can now actually run.g_assert. The TLS thread is joined, and a doubleg_source_destroyis gone.NO_DEVICE.Testing
Built and run in a Fedora 44 container matching the image's
libfprint-builder.goodixtls-servertest passes, also under ASan/UBSan with leak detection. Reverting the non-blocking change makes it hang until its time limit; restoring the olderr_from_ssl()makes it fail.egis_etu905, an unrelated driver that also fails intermittently without these changes and passes when run alone.fprintd-verifyloop interrupted at random points,fprintdRSS over a long lock screen, and a normal unlock by fingerprint.Draft until the sensor tests are done.
Not included
suspend/resume: the core already cancels an active verify when the driver has nosuspend, andresumedoesn't reach the driver with no action running, so this needs a different approach and real suspend testing.rpm/libfprint.specbump, until the sensor tests pass.🤖 Generated with Claude Code