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

[NIIBE Yutaka]

On 05/20/2015 09:19 AM, NIIBE Yutaka wrote:
> I'll post updated version, soon.

Here is the one.

Assuming write(2) is atomic, the comment I wrote (saying: multiple
race conditions) in the issue 1675 is wrong.  The race condition was
only here (nowhere than that).

Tested against the minimal reproducible test case.  The bug exists
in 1.4 and in 2.0, too.

diff --git a/g10/tdbio.c b/g10/tdbio.c
index 69438b4..74127d5 100644
--- a/g10/tdbio.c
+++ b/g10/tdbio.c
@@ -504,6 +504,21 @@ tdbio_set_dbname( const char *new_dbname, int create, int *r_nofile)
     else
       fname = xstrdup (new_dbname);

+    xfree (db_name);
+    db_name = fname;
+
+    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;
+    }
+
     if( access( fname, R_OK ) ) {
 #ifdef HAVE_W32CE_SYSTEM
       /* We know how the cegcc implementation of access works ;-). */
@@ -512,11 +527,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 +559,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 +576,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 +586,14 @@ 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;
+
+    if( !opt.lock_once ) {
+        if( !dotlock_release( lockhandle ) )
+            is_locked = 0;
+    }
+
     return 0;
 }

@@ -615,14 +613,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;
--