[PATCH] g10: Fix a race condition initially creating trustdb
NIIBE Yutaka
gniibe at fsij.org
Thu May 28 10:23:42 CEST 2015
On 05/25/2015 02:41 PM, NIIBE Yutaka wrote:
> Any other thought?
Here is again for review.
g10: Fix a race condition initially creating trustdb.
* g10/tdbio.c (take_write_lock, release_write_lock): New.
(put_record_into_cache, tdbio_sync, tdbio_end_transaction): Use
new lock functions.
(tdbio_set_dbname): Fix the race.
(open_db): Don't call dotlocck_create.
--
GnuPG-bug-id: 1675
I believe this fix is correct. Tested in 2.1. I also made
backport to 1.4 and it was also tested. Will do for 2.0.
May I push this change to master?
diff --git a/g10/tdbio.c b/g10/tdbio.c
index 98a7773..940165c 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 ();
gnupg_block_all_signals();
in_transaction = 0;
rc = tdbio_sync();
gnupg_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