[PATCH] g10: Fix a race condition initially creating trustdb
NIIBE Yutaka
gniibe at fsij.org
Thu May 21 13:23:08 CEST 2015
Hello,
I updated the patch again.
In this version, helper functions take_write_lock and
release_write_lock, so that the code is easier to read and to be
maintained. The intention is to express that we do serialization of
write accesses to trustdb.gpg from multiple processes of gpg.
Besides, preparation of the lock (calling dotlock_create function) is
in the take_write_lock function now, deferring the preparation as
possible.
In tdbio_set_dbname, I added a quick check for existing trustdb.gpg,
so that we can avoid lock operations for most likely case.
With this change against the one yesterday, if it's read-only accesses
to trustdb.gpg, no write access to the file system is done. So, it's
friendly to the situation of read-only file system.
My idea is applying this to 2.1. Later, I intend to apply to 1.4 and
2.0.
diff --git a/g10/tdbio.c b/g10/tdbio.c
index 69438b4..6d071a8 100644
--- a/g10/tdbio.c
+++ b/g10/tdbio.c
@@ -100,7 +100,33 @@ static int in_transaction;
static void open_db(void);
+static int
+take_write_lock (void)
+{
+ if (!lockhandle)
+ lockhandle = dotlock_create (db_name, 0);
+ if (!lockhandle)
+ log_fatal ( _("can't create lock for '%s'\n"), db_name );
+
+ if (!is_locked)
+ {
+ if (dotlock_take (lockhandle, -1) )
+ log_fatal ( _("can't lock '%s'\n"), db_name );
+ else
+ is_locked = 1;
+ return 0;
+ }
+ else
+ return 1;
+}
+static void
+release_write_lock (void)
+{
+ if (!opt.lock_once)
+ if (!dotlock_release (lockhandle))
+ is_locked = 0;
+}
/*************************************
************* record cache **********
@@ -256,12 +282,7 @@ put_record_into_cache( ulong recno, const char *data )
int n = dirty_count / 5; /* discard some dirty entries */
if( !n )
n = 1;
- if( !is_locked ) {
- if( dotlock_take( lockhandle, -1 ) )
- log_fatal("can't acquire lock - giving up\n");
- else
- is_locked = 1;
- }
+ take_write_lock ();
for( unused = NULL, r = cache_list; r; r = r->next ) {
if( r->flags.used && r->flags.dirty ) {
int rc = write_cache_item( r );
@@ -275,10 +296,7 @@ put_record_into_cache( ulong recno, const char *data )
break;
}
}
- if( !opt.lock_once ) {
- if( !dotlock_release( lockhandle ) )
- is_locked = 0;
- }
+ release_write_lock ();
assert( unused );
r = unused;
r->flags.used = 1;
@@ -317,13 +335,9 @@ tdbio_sync()
if( !cache_is_dirty )
return 0;
- if( !is_locked ) {
- if( dotlock_take( lockhandle, -1 ) )
- log_fatal("can't acquire lock - giving up\n");
- else
- is_locked = 1;
- did_lock = 1;
- }
+ if (!take_write_lock ())
+ did_lock = 1;
+
for( r = cache_list; r; r = r->next ) {
if( r->flags.used && r->flags.dirty ) {
int rc = write_cache_item( r );
@@ -332,10 +346,8 @@ tdbio_sync()
}
}
cache_is_dirty = 0;
- if( did_lock && !opt.lock_once ) {
- if( !dotlock_release (lockhandle) )
- is_locked = 0;
- }
+ if (did_lock)
+ release_write_lock ();
return 0;
}
@@ -372,20 +384,12 @@ tdbio_end_transaction()
if( !in_transaction )
log_bug("tdbio: no active transaction\n");
- if( !is_locked ) {
- if( dotlock_take( lockhandle, -1 ) )
- log_fatal("can't acquire lock - giving up\n");
- else
- is_locked = 1;
- }
+ take_write_lock ();
block_all_signals();
in_transaction = 0;
rc = tdbio_sync();
unblock_all_signals();
- if( !opt.lock_once ) {
- if( !dotlock_release (lockhandle) )
- is_locked = 0;
- }
+ release_write_lock ();
return rc;
}
@@ -483,6 +487,7 @@ int
tdbio_set_dbname( const char *new_dbname, int create, int *r_nofile)
{
char *fname;
+ struct stat statbuf;
static int initialized = 0;
if( !initialized ) {
@@ -504,6 +509,21 @@ tdbio_set_dbname( const char *new_dbname, int create, int *r_nofile)
else
fname = xstrdup (new_dbname);
+ xfree (db_name);
+ db_name = fname;
+
+ /*
+ * Quick check for (likely) case where there is trustdb.gpg
+ * already. This check is not required in theory, but it helps in
+ * practice, avoiding costly operations of preparing and taking
+ * the lock.
+ */
+ if (stat (fname, &statbuf) == 0 && statbuf.st_size > 0)
+ /* OK, we have the valid trustdb.gpg already. */
+ return 0;
+
+ take_write_lock ();
+
if( access( fname, R_OK ) ) {
#ifdef HAVE_W32CE_SYSTEM
/* We know how the cegcc implementation of access works ;-). */
@@ -512,11 +532,9 @@ tdbio_set_dbname( const char *new_dbname, int create, int *r_nofile)
else
gpg_err_set_errno (EIO);
#endif /*HAVE_W32CE_SYSTEM*/
- if( errno != ENOENT ) {
- log_error( _("can't access '%s': %s\n"), fname, strerror(errno) );
- xfree(fname);
- return GPG_ERR_TRUSTDB;
- }
+ if( errno != ENOENT )
+ log_fatal( _("can't access '%s': %s\n"), fname, strerror(errno) );
+
if (!create)
*r_nofile = 1;
else {
@@ -546,16 +564,6 @@ tdbio_set_dbname( const char *new_dbname, int create, int *r_nofile)
}
*p = save_slash;
- xfree(db_name);
- db_name = fname;
-#ifdef __riscos__
- if( !lockhandle )
- lockhandle = dotlock_create (db_name, 0);
- if( !lockhandle )
- log_fatal( _("can't create lock for '%s'\n"), db_name );
- if( dotlock_make (lockhandle, -1) )
- log_fatal( _("can't lock '%s'\n"), db_name );
-#endif /* __riscos__ */
oldmask=umask(077);
if (is_secured_filename (fname)) {
fp = NULL;
@@ -573,13 +581,6 @@ tdbio_set_dbname( const char *new_dbname, int create, int *r_nofile)
log_fatal (_("can't open '%s': %s\n"),
db_name, strerror (errno));
-#ifndef __riscos__
- if( !lockhandle )
- lockhandle = dotlock_create (db_name, 0);
- if( !lockhandle )
- log_fatal( _("can't create lock for '%s'\n"), db_name );
-#endif /* !__riscos__ */
-
rc = create_version_record ();
if( rc )
log_fatal( _("%s: failed to create version record: %s"),
@@ -590,12 +591,10 @@ tdbio_set_dbname( const char *new_dbname, int create, int *r_nofile)
if( !opt.quiet )
log_info(_("%s: trustdb created\n"), db_name);
-
- return 0;
}
}
- xfree(db_name);
- db_name = fname;
+
+ release_write_lock ();
return 0;
}
@@ -615,14 +614,6 @@ open_db()
assert( db_fd == -1 );
- if (!lockhandle )
- lockhandle = dotlock_create (db_name, 0);
- if (!lockhandle )
- log_fatal( _("can't create lock for '%s'\n"), db_name );
-#ifdef __riscos__
- if (dotlock_take (lockhandle, -1) )
- log_fatal( _("can't lock '%s'\n"), db_name );
-#endif /* __riscos__ */
#ifdef HAVE_W32CE_SYSTEM
{
DWORD prevrc = 0;
--
More information about the Gnupg-devel
mailing list