[PATCH] g10: Fix a race condition initially creating trustdb

[NIIBE Yutaka]

Hello,

This is a fix for the issue 1675:
    https://bugs.gnupg.org/gnupg/issue1675

The commit b73ae3ca36547939c9aaf54c0d05fbc93d47c096 in Sep 23, 2011
changed make_dotlock into dotlock_take, but there was a mistake that
it changed to dotlock_make (for riscos).

The commit acde3f8ea660ced34ebe34f7d31185c9fdea8295 in Sep 22, 2011
said "Remove support for RISCOS from dotlock.c", so, I think that
"ifdef __riscos__ in dotlock.c" is dead code.

Following patch (only) implements the mutual exclusion between:

  * initial creation of trustdb.gpg (write access) in tdbio_set_dbname

  * reading from trustdb.gpg in tdbio_read_record (read access)

I read through the code for trustdb.gpg.  If I read the code
correctly, it implements mutual exclusions between write accesses to
trustdb.gpg, but there are still races between:

  * writing to trustdb.gpg

  * reading from trustdb.gpg

I'll try to make reproducible case(s) and to consider fixes for those.

This patch is tested against the minimal reproducible case.  It would
be better to implement multiple-readers-single-writer-lock, but
assuming the write access to trustdb is rare, simple mutual exclusion
would be just acceptable.

Meanwhile, please review following patch.

diff --git a/g10/tdbio.c b/g10/tdbio.c
index 69438b4..08e2ab9 100644
--- a/g10/tdbio.c
+++ b/g10/tdbio.c
@@ -548,14 +548,18 @@ tdbio_set_dbname( const char *new_dbname, int create, int *r_nofile)

 	    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__ */
+            if( !is_locked  ) {
+                if( dotlock_take (lockhandle, -1) )
+                    log_fatal( _("can't lock '%s'\n"), db_name );
+            else
+                is_locked = 1;
+            }
+
 	    oldmask=umask(077);
             if (is_secured_filename (fname)) {
                 fp = NULL;
@@ -573,13 +577,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"),
@@ -591,6 +588,10 @@ tdbio_set_dbname( const char *new_dbname, int create, int *r_nofile)
 	    if( !opt.quiet )
 		log_info(_("%s: trustdb created\n"), db_name);

+            if( !opt.lock_once ) {
+                if( !dotlock_release( lockhandle ) )
+                    is_locked = 0;
+            }
 	    return 0;
 	}
     }
@@ -619,10 +620,13 @@ open_db()
     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__ */
+  if( !is_locked ) {
+      if( dotlock_take( lockhandle, -1 ) )
+          log_fatal("can't acquire lock - giving up\n");
+      else
+          is_locked = 1;
+  }
+
 #ifdef HAVE_W32CE_SYSTEM
   {
     DWORD prevrc = 0;
@@ -659,6 +663,11 @@ open_db()
   /* Read the version record. */
   if (tdbio_read_record (0, &rec, RECTYPE_VER ) )
     log_fatal( _("%s: invalid trustdb\n"), db_name );
+
+  if( !opt.lock_once ) {
+      if( !dotlock_release( lockhandle ) )
+          is_locked = 0;
+  }
 }


--