From b05657f3fe5d8865757a8cb893b5cac75237dc44 Mon Sep 17 00:00:00 2001 From: Leonardo Francisco Date: Mon, 6 Apr 2026 01:07:22 -0400 Subject: [PATCH] validity: fix dead code, stubs, and broken logic across Iteration 6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comprehensive bugfix for issues found during code audit: 1. parse_match_result (CRITICAL): Replace dead while loop + hardcoded offsets with proper TLV dict parsing (tag LE16 + len LE16 + data) matching python-validity's parse_dict(). Extracts user_dbid (tag 1), subtype (tag 3), and hash (tag 4) from match result. 2. ENROLL_CREATE_USER (CRITICAL): Fix NULL user_id crash — g_uuid_string_random() now generates UUID for user identity instead of passing NULL to g_variant_new_string(). 3. Identify gallery matching: Match sensor result against gallery by comparing finger subtype instead of always returning first entry. 4. Field abuse: Add dedicated enroll_user_dbid field to FpiDeviceValidity instead of reusing delete_storage_dbid for enrollment state. 5. Delete SSM: Full implementation — enumerate users via get_user_storage, iterate users to find matching finger subtype, delete via cmd 0x48 (del_record). Proper error handling for missing records. 6. match_finger double allocation: Remove redundant 12-byte alloc/free, single clean 13-byte allocation per python-validity format. 7. clear_storage: Full SSM implementation — enumerate user storage, del_record for each user. Replaces NOT_SUPPORTED stub. 8. Clean stale TODO/placeholder comments. All 37 tests pass (0 fail, 2 skip — unchanged baseline). --- libfprint/drivers/validity/validity.h | 13 + libfprint/drivers/validity/validity_db.c | 21 +- libfprint/drivers/validity/validity_enroll.c | 35 +- libfprint/drivers/validity/validity_verify.c | 347 +++++++++++++++---- 4 files changed, 314 insertions(+), 102 deletions(-) diff --git a/libfprint/drivers/validity/validity.h b/libfprint/drivers/validity/validity.h index 1917cb96..74bd09e6 100644 --- a/libfprint/drivers/validity/validity.h +++ b/libfprint/drivers/validity/validity.h @@ -184,6 +184,16 @@ typedef enum { DELETE_NUM_STATES, } ValidityDeleteState; +/* Clear storage SSM states */ +typedef enum { + CLEAR_GET_STORAGE = 0, + CLEAR_GET_STORAGE_RECV, + CLEAR_DEL_USER, + CLEAR_DEL_USER_RECV, + CLEAR_DONE, + CLEAR_NUM_STATES, +} ValidityClearState; + #define FPI_TYPE_DEVICE_VALIDITY (fpi_device_validity_get_type ()) G_DECLARE_FINAL_TYPE (FpiDeviceValidity, fpi_device_validity, FPI, DEVICE_VALIDITY, FpDevice) @@ -217,6 +227,7 @@ struct _FpiDeviceValidity guint8 *enroll_template; gsize enroll_template_len; guint enroll_stage; + guint16 enroll_user_dbid; /* Verify/identify mode flag: TRUE = identify, FALSE = verify */ gboolean identify_mode; @@ -227,6 +238,8 @@ struct _FpiDeviceValidity /* Delete state */ guint16 delete_storage_dbid; + guint16 delete_finger_subtype; + guint16 delete_finger_dbid; /* Command SSM: manages the send-cmd/recv-response cycle */ FpiSsm *cmd_ssm; diff --git a/libfprint/drivers/validity/validity_db.c b/libfprint/drivers/validity/validity_db.c index 465c66df..c97cef78 100644 --- a/libfprint/drivers/validity/validity_db.c +++ b/libfprint/drivers/validity/validity_db.c @@ -327,30 +327,17 @@ validity_db_build_cmd_capture_stop (gsize *out_len) guint8 * validity_db_build_cmd_match_finger (gsize *out_len) { - guint8 *cmd = g_new0 (guint8, 12); + /* python-validity: pack('delete_storage_dbid = user_dbid; + self->enroll_user_dbid = user_dbid; } else { @@ -605,7 +614,7 @@ enroll_run_state (FpiSsm *ssm, finger = fp_print_get_finger (print); guint16 subtype = validity_finger_to_subtype (finger); - guint16 user_dbid = self->delete_storage_dbid; + guint16 user_dbid = self->enroll_user_dbid; /* Build finger data from template + tid */ gsize finger_data_len; diff --git a/libfprint/drivers/validity/validity_verify.c b/libfprint/drivers/validity/validity_verify.c index 1664d9e8..5507ea68 100644 --- a/libfprint/drivers/validity/validity_verify.c +++ b/libfprint/drivers/validity/validity_verify.c @@ -159,9 +159,12 @@ verify_start_interrupt_wait (FpiDeviceValidity *self, * Match result parsing * * cmd 0x60 response: len(2LE) | dict_data[len] - * dict_data is a sequence of tagged TLV entries: - * tag(1) | data - * tag 1 → user_id(4LE), tag 3 → subtype(2LE), tag 4 → hash + * dict_data is a TLV dictionary (python-validity parse_dict()): + * while data: tag(2LE) | len(2LE) | value[len] + * In the match response: + * tag 1 → user_dbid(4LE) + * tag 3 → subtype(2LE) + * tag 4 → hash (variable length) * ================================================================ */ typedef struct @@ -180,62 +183,93 @@ match_result_clear (MatchResult *r) memset (r, 0, sizeof (*r)); } +/** + * parse_match_result: + * @data: response payload from cmd 0x60 (after status word) + * @data_len: length of @data + * @result: (out): parsed match result + * + * Parses the TLV dictionary from a get_match_result (cmd 0x60) response. + * + * python-validity reference (sensor.py match_finger()): + * rsp = self.parse_dict(rsp) + * usrid, subtype, hsh = rsp[1], rsp[3], rsp[4] + * + * where parse_dict() is: + * while len(x) > 0: + * (t, l), x = unpack('= 4) { - /* Each entry has a variable format. - * Based on python-validity parse_dict(): - * rsp[1] = user_id(4LE) - * rsp[3] = subtype(2LE) - * rsp[4] = hash - * These are indexed by occurrence order, not by tag byte. - * - * The response format from cmd 0x60 is actually: - * len(2LE) | entries - * where entries are variable-length tagged data. - * - * For simplicity, parse the known fixed structure below. */ - break; - } + guint16 tag, entry_len; + const guint8 *entry_data; - /* For the initial implementation, we try to extract the match result - * from the raw response. The python-validity code extracts fields - * via indexed dict parsing. For now, mark as matched if we got data. */ - if (remaining >= 6) - { - result->matched = TRUE; - result->user_dbid = FP_READ_UINT32_LE (dict_data); - if (remaining >= 8) - result->subtype = FP_READ_UINT16_LE (&dict_data[4]); - if (remaining > 8) + if (!fpi_byte_reader_get_uint16_le (&dict_reader, &tag)) + break; + if (!fpi_byte_reader_get_uint16_le (&dict_reader, &entry_len)) + break; + if (!fpi_byte_reader_get_data (&dict_reader, entry_len, &entry_data)) + break; + + switch (tag) { - result->hash = g_memdup2 (&dict_data[6], remaining - 6); - result->hash_len = remaining - 6; + case 1: /* user_dbid (4 bytes LE) */ + if (entry_len >= 4) + { + result->user_dbid = FP_READ_UINT32_LE (entry_data); + result->matched = TRUE; + } + break; + + case 3: /* subtype (2 bytes LE) */ + if (entry_len >= 2) + result->subtype = FP_READ_UINT16_LE (entry_data); + break; + + case 4: /* hash (variable) */ + if (entry_len > 0) + { + result->hash = g_memdup2 (entry_data, entry_len); + result->hash_len = entry_len; + } + break; + + default: + fp_dbg ("parse_match_result: ignoring unknown tag %u (len=%u)", + tag, entry_len); + break; } } @@ -438,23 +472,40 @@ verify_ssm_done (FpiSsm *ssm, if (self->identify_mode) { - /* Identify mode: report which print matched */ if (have_match) { fp_info ("Identify matched: user_dbid=%u subtype=%u", match.user_dbid, match.subtype); - /* For identify, we'd need to match against the gallery. - * Since the sensor does the matching internally, - * we report FPI_MATCH_SUCCESS with the first gallery print - * for now. In a full implementation, we'd look up the - * user_dbid against the gallery. */ + + /* Match the sensor result against the gallery by comparing + * the finger subtype. The sensor does the actual 1:N match + * internally; we just need to find which gallery FpPrint + * corresponds to the matched subtype. */ FpPrint *gallery_match = NULL; GPtrArray *gallery = NULL; fpi_device_get_identify_data (dev, &gallery); - if (gallery && gallery->len > 0) - gallery_match = g_ptr_array_index (gallery, 0); + if (gallery) + { + gint matched_finger = validity_subtype_to_finger (match.subtype); + + for (guint i = 0; i < gallery->len; i++) + { + FpPrint *candidate = g_ptr_array_index (gallery, i); + if (fp_print_get_finger (candidate) == (FpFinger) matched_finger) + { + gallery_match = candidate; + break; + } + } + + /* If no finger match, fall back to first gallery print — + * the sensor confirmed a match even if we can't correlate + * the subtype to a specific gallery entry. */ + if (!gallery_match && gallery->len > 0) + gallery_match = g_ptr_array_index (gallery, 0); + } fpi_device_identify_report (dev, gallery_match, NULL, NULL); } @@ -701,56 +752,117 @@ delete_run_state (FpiSsm *ssm, return; } - ValidityUserStorage storage = { 0 }; + /* Parse into list_storage (shared with list SSM, not concurrent) */ + validity_user_storage_clear (&self->list_storage); if (!self->cmd_response_data || !validity_db_parse_user_storage (self->cmd_response_data, self->cmd_response_len, - &storage)) + &self->list_storage)) { fpi_ssm_mark_failed (ssm, fpi_device_error_new (FP_DEVICE_ERROR_DATA_NOT_FOUND)); return; } - self->delete_storage_dbid = storage.dbid; - validity_user_storage_clear (&storage); + self->delete_storage_dbid = self->list_storage.dbid; + /* Extract finger subtype from the print to delete */ + { + FpPrint *print = NULL; + fpi_device_get_delete_data (dev, &print); + + FpFinger finger = fp_print_get_finger (print); + self->delete_finger_subtype = validity_finger_to_subtype (finger); + } + + self->list_user_idx = 0; fpi_ssm_next_state (ssm); } break; case DELETE_LOOKUP_USER: { - /* For delete, we need to find the user matching the print. - * Since we use device-stored prints, we can use the print's - * driver-specific data to identify the record. For now, - * we delete the first user's matching finger. */ - FpPrint *print = NULL; - fpi_device_get_delete_data (dev, &print); + /* Look up the user matching the print to delete. + * Iterate users to find one with a matching finger subtype. + * python-validity: db.lookup_user(identity) */ + if (self->list_user_idx >= self->list_storage.user_count) + { + /* No matching finger found across all users */ + fp_info ("Delete: no matching finger (subtype=%u) found in DB", + self->delete_finger_subtype); + fpi_ssm_mark_failed (ssm, + fpi_device_error_new (FP_DEVICE_ERROR_DATA_NOT_FOUND)); + return; + } - /* TODO: Use print's stored user ID to look up the specific - * record. For now, skip lookup and go to delete. */ - fpi_ssm_next_state (ssm); + { + guint16 user_dbid = self->list_storage.user_dbids[self->list_user_idx]; + gsize cmd_len; + guint8 *cmd = validity_db_build_cmd_get_user (user_dbid, &cmd_len); + vcsfw_tls_cmd_send (self, ssm, cmd, cmd_len, NULL); + g_free (cmd); + } } break; case DELETE_LOOKUP_USER_RECV: - fpi_ssm_next_state (ssm); + { + /* Parse user and look for the finger to delete */ + if (self->cmd_response_status == VCSFW_STATUS_OK && + self->cmd_response_data) + { + ValidityUser user = { 0 }; + if (validity_db_parse_user (self->cmd_response_data, + self->cmd_response_len, + &user)) + { + for (guint16 i = 0; i < user.finger_count; i++) + { + if (user.fingers[i].subtype == self->delete_finger_subtype) + { + /* Found matching finger — store dbid for deletion */ + self->delete_finger_dbid = user.fingers[i].dbid; + validity_user_clear (&user); + fpi_ssm_next_state (ssm); + return; + } + } + validity_user_clear (&user); + } + } + + /* Try next user — jump back to DELETE_LOOKUP_USER */ + self->list_user_idx++; + fpi_ssm_jump_to_state (ssm, DELETE_LOOKUP_USER); + } break; case DELETE_DEL_RECORD: { - /* Without proper print-to-dbid mapping, we can't delete - * a specific record. Report success for now — a full - * implementation needs the print's dbid stored as driver data. */ - fp_info ("Delete: record deletion requires print-to-dbid mapping " - "(not yet implemented)"); - fpi_ssm_jump_to_state (ssm, DELETE_DONE); + /* Delete the finger record via cmd 0x48 + * python-validity: db.del_record(dbid) */ + gsize cmd_len; + guint8 *cmd = validity_db_build_cmd_del_record ( + self->delete_finger_dbid, &cmd_len); + vcsfw_tls_cmd_send (self, ssm, cmd, cmd_len, NULL); + g_free (cmd); } break; case DELETE_DEL_RECORD_RECV: - fpi_ssm_next_state (ssm); + { + if (self->cmd_response_status != VCSFW_STATUS_OK) + { + fp_warn ("del_record failed: status=0x%04x", + self->cmd_response_status); + fpi_ssm_mark_failed (ssm, + fpi_device_error_new (FP_DEVICE_ERROR_PROTO)); + return; + } + + fp_info ("Deleted finger record: dbid=%u", self->delete_finger_dbid); + fpi_ssm_next_state (ssm); + } break; case DELETE_DONE: @@ -764,6 +876,9 @@ delete_ssm_done (FpiSsm *ssm, FpDevice *dev, GError *error) { + FpiDeviceValidity *self = FPI_DEVICE_VALIDITY (dev); + + validity_user_storage_clear (&self->list_storage); fpi_device_delete_complete (dev, error); } @@ -778,14 +893,102 @@ validity_delete (FpDevice *device) fpi_ssm_start (ssm, delete_ssm_done); } +/* ================================================================ + * Clear storage — delete all fingerprint records from the sensor DB + * python-validity: for user in db.get_user_storage(): db.del_record(user.dbid) + * ================================================================ */ + +static void +clear_run_state (FpiSsm *ssm, + FpDevice *dev) +{ + FpiDeviceValidity *self = FPI_DEVICE_VALIDITY (dev); + + switch (fpi_ssm_get_cur_state (ssm)) + { + case CLEAR_GET_STORAGE: + { + gsize cmd_len; + guint8 *cmd = validity_db_build_cmd_get_user_storage ( + VALIDITY_STORAGE_NAME, &cmd_len); + vcsfw_tls_cmd_send (self, ssm, cmd, cmd_len, NULL); + g_free (cmd); + } + break; + + case CLEAR_GET_STORAGE_RECV: + { + validity_user_storage_clear (&self->list_storage); + + if (self->cmd_response_status != VCSFW_STATUS_OK || + !self->cmd_response_data || + !validity_db_parse_user_storage (self->cmd_response_data, + self->cmd_response_len, + &self->list_storage)) + { + /* No storage or parse error — nothing to clear */ + fpi_ssm_jump_to_state (ssm, CLEAR_DONE); + return; + } + + self->list_user_idx = 0; + fpi_ssm_next_state (ssm); + } + break; + + case CLEAR_DEL_USER: + { + if (self->list_user_idx >= self->list_storage.user_count) + { + fpi_ssm_jump_to_state (ssm, CLEAR_DONE); + return; + } + + guint16 user_dbid = self->list_storage.user_dbids[self->list_user_idx]; + + gsize cmd_len; + guint8 *cmd = validity_db_build_cmd_del_record (user_dbid, &cmd_len); + vcsfw_tls_cmd_send (self, ssm, cmd, cmd_len, NULL); + g_free (cmd); + } + break; + + case CLEAR_DEL_USER_RECV: + { + if (self->cmd_response_status != VCSFW_STATUS_OK) + fp_warn ("clear_storage: del_record(dbid=%u) failed: status=0x%04x", + self->list_storage.user_dbids[self->list_user_idx], + self->cmd_response_status); + + self->list_user_idx++; + fpi_ssm_jump_to_state (ssm, CLEAR_DEL_USER); + } + break; + + case CLEAR_DONE: + fpi_ssm_mark_completed (ssm); + break; + } +} + +static void +clear_ssm_done (FpiSsm *ssm, + FpDevice *dev, + GError *error) +{ + FpiDeviceValidity *self = FPI_DEVICE_VALIDITY (dev); + + validity_user_storage_clear (&self->list_storage); + fpi_device_clear_storage_complete (dev, error); +} + void validity_clear_storage (FpDevice *device) { - /* Clear storage would need to enumerate all records and delete each. - * For now, report not supported — a full implementation would: - * 1. Get user storage - * 2. For each user: del_record(user.dbid) - * 3. Report complete */ - fpi_device_clear_storage_complete (device, - fpi_device_error_new (FP_DEVICE_ERROR_NOT_SUPPORTED)); + FpiSsm *ssm; + + G_DEBUG_HERE (); + + ssm = fpi_ssm_new (device, clear_run_state, CLEAR_NUM_STATES); + fpi_ssm_start (ssm, clear_ssm_done); }