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 dotlock_create.

--

GnuPG-bug-id: 1675

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 ) {
+        take_write_lock ();
-	    if( dotlock_take( lockhandle, -1 ) )
-		log_fatal("can't acquire lock - giving up\n");
-	    else
-		is_locked = 1;
-	}
 	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 ) {
+        release_write_lock ();
-	    if( !dotlock_release( lockhandle ) )
-		is_locked = 0;
-	}
 	assert( unused );
 	r = unused;
 	r->flags.used = 1;
@@ -317,13 +335,9 @@ tdbio_sync()
     if( !cache_is_dirty )
 	return 0;
 
-    if( !is_locked ) {
+    if (!take_write_lock ())
-	if( dotlock_take( lockhandle, -1 ) )
-	    log_fatal("can't acquire lock - giving up\n");
-	else
-	    is_locked = 1;
         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 (did_lock)
-	if( !dotlock_release (lockhandle) )
+        release_write_lock ();
-	    is_locked = 0;
-    }
 
     return 0;
 }
@@ -372,20 +384,12 @@ tdbio_end_transaction()
 
     if( !in_transaction )
 	log_bug("tdbio: no active transaction\n");
-    if( !is_locked ) {
+    take_write_lock ();
-	if( dotlock_take( lockhandle, -1 ) )
-	    log_fatal("can't acquire lock - giving up\n");
-	else
-	    is_locked = 1;
-    }
     gnupg_block_all_signals();
     in_transaction = 0;
     rc = tdbio_sync();
     gnupg_unblock_all_signals();
-    if( !opt.lock_once ) {
+    release_write_lock ();
-	if( !dotlock_release (lockhandle) )
-	    is_locked = 0;
-    }
     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 ) {
+        if( errno != ENOENT )
-	    log_error( _("can't access '%s': %s\n"), fname, strerror(errno) );
+            log_fatal( _("can't access '%s': %s\n"), fname, strerror(errno) );
-	    xfree(fname);
+
-	    return GPG_ERR_TRUSTDB;
-	}
 	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;
+    release_write_lock ();
-	}
-    }
-    xfree(db_name);
-    db_name = fname;
     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;