From 22e274f839f9a6c9a511648f29cae497f6492c97 Mon Sep 17 00:00:00 2001 From: Werner Koch Date: Tue, 14 May 2019 13:36:08 +0200 Subject: [PATCH] sm: Change keydb code to use the keybox locking. * kbx/keybox-init.c (keybox_lock): New arg TIMEOUT. Change all callers to pass -1 when locking. * sm/keydb.c (struct resource_item): Remove LOCKANDLE. (struct keydb_handle): Add KEEP_LOCK. (keydb_add_resource): Use keybox locking instead of a separate dotlock for testing whether we can run a compress. (keydb_release): Reset KEEP_LOCK. (keydb_lock): Set KEEP_LOCK. (unlock_all): Take care of KEEP_LOCK. (lock_all): Use keybox_lock instead of dotlock fucntions. (keydb_delete): Remove arg UNLOCK. * sm/delete.c (delete_one): Adjust keydb_delete. Due to the KEEP_LOCK the keydb_release takes care of unlocking. -- This aligns the code more with g10/keydb.c and avoids the separate calls to dotlock_take. GnuPG-bug-id: 4505 Signed-off-by: Werner Koch --- g10/keydb.c | 8 +++--- kbx/keybox-init.c | 13 ++++++--- kbx/keybox.h | 2 +- sm/delete.c | 5 ++-- sm/keydb.c | 72 +++++++++++++++++++++++------------------------ sm/keydb.h | 2 +- 6 files changed, 53 insertions(+), 49 deletions(-) diff --git a/g10/keydb.c b/g10/keydb.c index 8c067e1df..45eb4aa34 100644 --- a/g10/keydb.c +++ b/g10/keydb.c @@ -1,6 +1,6 @@ /* keydb.c - key database dispatcher * Copyright (C) 2001-2013 Free Software Foundation, Inc. - * Coyrright (C) 2001-2015 Werner Koch + * Copyright (C) 2001-2015 Werner Koch * * This file is part of GnuPG. * @@ -1076,7 +1076,7 @@ lock_all (KEYDB_HANDLE hd) rc = keyring_lock (hd->active[i].u.kr, 1); break; case KEYDB_RESOURCE_TYPE_KEYBOX: - rc = keybox_lock (hd->active[i].u.kb, 1); + rc = keybox_lock (hd->active[i].u.kb, 1, -1); break; } } @@ -1094,7 +1094,7 @@ lock_all (KEYDB_HANDLE hd) keyring_lock (hd->active[i].u.kr, 0); break; case KEYDB_RESOURCE_TYPE_KEYBOX: - keybox_lock (hd->active[i].u.kb, 0); + keybox_lock (hd->active[i].u.kb, 0, 0); break; } } @@ -1127,7 +1127,7 @@ unlock_all (KEYDB_HANDLE hd) keyring_lock (hd->active[i].u.kr, 0); break; case KEYDB_RESOURCE_TYPE_KEYBOX: - keybox_lock (hd->active[i].u.kb, 0); + keybox_lock (hd->active[i].u.kb, 0, 0); break; } } diff --git a/kbx/keybox-init.c b/kbx/keybox-init.c index 6a83f7162..9a65c1345 100644 --- a/kbx/keybox-init.c +++ b/kbx/keybox-init.c @@ -261,10 +261,12 @@ _keybox_close_file (KEYBOX_HANDLE hd) /* - * Lock the keybox at handle HD, or unlock if YES is false. + * Lock the keybox at handle HD, or unlock if YES is false. TIMEOUT + * is the value used for dotlock_take. In general -1 should be used + * when taking a lock; use 0 when releasing a lock. */ gpg_error_t -keybox_lock (KEYBOX_HANDLE hd, int yes) +keybox_lock (KEYBOX_HANDLE hd, int yes, long timeout) { gpg_error_t err = 0; KB_NAME kb = hd->kb; @@ -302,10 +304,13 @@ keybox_lock (KEYBOX_HANDLE hd, int yes) hd->fp = NULL; } #endif /*HAVE_W32_SYSTEM*/ - if (dotlock_take (kb->lockhd, -1)) + if (dotlock_take (kb->lockhd, timeout)) { err = gpg_error_from_syserror (); - log_info ("can't lock '%s'\n", kb->fname ); + if (!timeout && gpg_err_code (err) == GPG_ERR_EACCES) + ; /* No diagnostic if we only tried to lock. */ + else + log_info ("can't lock '%s'\n", kb->fname ); } else kb->is_locked = 1; diff --git a/kbx/keybox.h b/kbx/keybox.h index 665b05fc0..4d941571e 100644 --- a/kbx/keybox.h +++ b/kbx/keybox.h @@ -76,7 +76,7 @@ void keybox_pop_found_state (KEYBOX_HANDLE hd); const char *keybox_get_resource_name (KEYBOX_HANDLE hd); int keybox_set_ephemeral (KEYBOX_HANDLE hd, int yes); -gpg_error_t keybox_lock (KEYBOX_HANDLE hd, int yes); +gpg_error_t keybox_lock (KEYBOX_HANDLE hd, int yes, long timeout); /*-- keybox-file.c --*/ /* Fixme: This function does not belong here: Provide a better diff --git a/sm/delete.c b/sm/delete.c index f359cc595..b370406de 100644 --- a/sm/delete.c +++ b/sm/delete.c @@ -113,7 +113,8 @@ delete_one (ctrl_t ctrl, const char *username) goto leave; } - /* We need to search again to get back to the right position. */ + /* We need to search again to get back to the right position. Neo + * that the lock is kept until the KH is released. */ rc = keydb_lock (kh); if (rc) { @@ -132,7 +133,7 @@ delete_one (ctrl_t ctrl, const char *username) goto leave; } - rc = keydb_delete (kh, duplicates ? 0 : 1); + rc = keydb_delete (kh); if (rc) goto leave; if (opt.verbose) diff --git a/sm/keydb.c b/sm/keydb.c index f66a5766d..0144a8189 100644 --- a/sm/keydb.c +++ b/sm/keydb.c @@ -47,7 +47,6 @@ struct resource_item { KEYBOX_HANDLE kr; } u; void *token; - dotlock_t lockhandle; }; static struct resource_item all_resources[MAX_KEYDB_RESOURCES]; @@ -58,7 +57,14 @@ static int any_registered; struct keydb_handle { + + /* If this flag is set the resources is locked. */ int locked; + + /* If this flag is set a lock will only be released by + * keydb_release. */ + int keep_lock; + int found; int saved_found; int current; @@ -346,26 +352,20 @@ keydb_add_resource (ctrl_t ctrl, const char *url, int force, int *auto_created) err = gpg_error (GPG_ERR_RESOURCE_LIMIT); else { + KEYBOX_HANDLE kbxhd; + all_resources[used_resources].type = rt; all_resources[used_resources].u.kr = NULL; /* Not used here */ all_resources[used_resources].token = token; - all_resources[used_resources].lockhandle - = dotlock_create (filename, 0); - if (!all_resources[used_resources].lockhandle) - log_fatal ( _("can't create lock for '%s'\n"), filename); - - /* Do a compress run if needed and the file is not locked. */ - if (!dotlock_take (all_resources[used_resources].lockhandle, 0)) + /* Do a compress run if needed and the keybox is not locked. */ + kbxhd = keybox_new_x509 (token, 0); + if (kbxhd) { - KEYBOX_HANDLE kbxhd = keybox_new_x509 (token, 0); + if (!keybox_lock (kbxhd, 1, 0)) + keybox_compress (kbxhd); - if (kbxhd) - { - keybox_compress (kbxhd); - keybox_release (kbxhd); - } - dotlock_release (all_resources[used_resources].lockhandle); + keybox_release (kbxhd); } used_resources++; @@ -415,7 +415,6 @@ keydb_new (void) case KEYDB_RESOURCE_TYPE_KEYBOX: hd->active[j].type = all_resources[i].type; hd->active[j].token = all_resources[i].token; - hd->active[j].lockhandle = all_resources[i].lockhandle; hd->active[j].u.kr = keybox_new_x509 (all_resources[i].token, 0); if (!hd->active[j].u.kr) { @@ -442,6 +441,7 @@ keydb_release (KEYDB_HANDLE hd) assert (active_handles > 0); active_handles--; + hd->keep_lock = 0; unlock_all (hd); for (i=0; i < hd->used; i++) { @@ -526,17 +526,22 @@ keydb_set_ephemeral (KEYDB_HANDLE hd, int yes) /* If the keyring has not yet been locked, lock it now. This - operation is required before any update operation; it is optional - for an insert operation. The lock is released with - keydb_released. */ + * operation is required before any update operation; it is optional + * for an insert operation. The lock is kept until a keydb_release so + * that internal unlock_all calls have no effect. */ gpg_error_t keydb_lock (KEYDB_HANDLE hd) { + gpg_error_t err; + if (!hd) return gpg_error (GPG_ERR_INV_HANDLE); - if (hd->locked) - return 0; /* Already locked. */ - return lock_all (hd); + + err = lock_all (hd); + if (!err) + hd->keep_lock = 1; + + return err; } @@ -556,8 +561,7 @@ lock_all (KEYDB_HANDLE hd) case KEYDB_RESOURCE_TYPE_NONE: break; case KEYDB_RESOURCE_TYPE_KEYBOX: - if (hd->active[i].lockhandle) - rc = dotlock_take (hd->active[i].lockhandle, -1); + rc = keybox_lock (hd->active[i].u.kr, 1, -1); break; } if (rc) @@ -566,7 +570,7 @@ lock_all (KEYDB_HANDLE hd) if (rc) { - /* revert the already set locks */ + /* Revert the already set locks. */ for (i--; i >= 0; i--) { switch (hd->active[i].type) @@ -574,8 +578,7 @@ lock_all (KEYDB_HANDLE hd) case KEYDB_RESOURCE_TYPE_NONE: break; case KEYDB_RESOURCE_TYPE_KEYBOX: - if (hd->active[i].lockhandle) - dotlock_release (hd->active[i].lockhandle); + keybox_lock (hd->active[i].u.kr, 0, 0); break; } } @@ -583,10 +586,7 @@ lock_all (KEYDB_HANDLE hd) else hd->locked = 1; - /* make_dotlock () does not yet guarantee that errno is set, thus - we can't rely on the error reason and will simply use - EACCES. */ - return rc? gpg_error (GPG_ERR_EACCES) : 0; + return rc; } static void @@ -594,7 +594,7 @@ unlock_all (KEYDB_HANDLE hd) { int i; - if (!hd->locked) + if (!hd->locked || hd->keep_lock) return; for (i=hd->used-1; i >= 0; i--) @@ -604,8 +604,7 @@ unlock_all (KEYDB_HANDLE hd) case KEYDB_RESOURCE_TYPE_NONE: break; case KEYDB_RESOURCE_TYPE_KEYBOX: - if (hd->active[i].lockhandle) - dotlock_release (hd->active[i].lockhandle); + keybox_lock (hd->active[i].u.kr, 0, 0); break; } } @@ -840,7 +839,7 @@ keydb_update_cert (KEYDB_HANDLE hd, ksba_cert_t cert) * The current keyblock or cert will be deleted. */ int -keydb_delete (KEYDB_HANDLE hd, int unlock) +keydb_delete (KEYDB_HANDLE hd) { int rc = -1; @@ -866,8 +865,7 @@ keydb_delete (KEYDB_HANDLE hd, int unlock) break; } - if (unlock) - unlock_all (hd); + unlock_all (hd); return rc; } diff --git a/sm/keydb.h b/sm/keydb.h index 623462553..20dcdbe4d 100644 --- a/sm/keydb.h +++ b/sm/keydb.h @@ -49,7 +49,7 @@ int keydb_get_cert (KEYDB_HANDLE hd, ksba_cert_t *r_cert); int keydb_insert_cert (KEYDB_HANDLE hd, ksba_cert_t cert); int keydb_update_cert (KEYDB_HANDLE hd, ksba_cert_t cert); -int keydb_delete (KEYDB_HANDLE hd, int unlock); +int keydb_delete (KEYDB_HANDLE hd); int keydb_locate_writable (KEYDB_HANDLE hd, const char *reserved); void keydb_rebuild_caches (void);