g10: Fix a race condition initially creating trustdb.
authorNIIBE Yutaka <gniibe@fsij.org>
Thu, 28 May 2015 08:08:37 +0000 (17:08 +0900)
committerNIIBE Yutaka <gniibe@fsij.org>
Thu, 28 May 2015 08:18:35 +0000 (17:18 +0900)
* 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

index 98a7773..940165c 100644 (file)
@@ -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;
+}
 \f
 /*************************************
  ************* 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;