[evolution-patches] Re: [Evolution-hackers] Deadlock in camel-gpg-context.c

[Jeffrey Stedfast <fejj ximian com>]

Attached is a patch to clean up the camel-gpg-context code to deal with
this scenerio.

We don't even need the seen_eof1 bit in gpg_verify() for determining if
we've gotten a TRUST_* status message since we can easily tell by
checking gpg->trust != GPG_TRUST_UNKNOWN.

Jeff

On Mon, 2003-04-21 at 15:05, Jeffrey Stedfast wrote:
> On Mon, 2003-04-21 at 13:55, Christophe Saout wrote:
> > Am Mon, 2003-04-21 um 19.41 schrieb Jeffrey Stedfast:
> > 
> > > > In camel/camel-gpg-context.c (evo 1.3 cvs) there is a problem:
> > > > 
> > > > In gpg_ctx_op_step the file descriptors stdout_fd, stderr_fd and
> > > > status_fd are polled for input.
> > > > 
> > > > But stdout_fd is not read when there is no gpg->ostream to get the data
> > > > (if (FD_ISSET (gpg->stdout_fd, &rdset) && gpg->ostream)).
> > > > 
> > > > So the end of file state cannot be detected and thus gpg->seen_eof1
> > > > won't be set.
> > > > 
> > > > If that happens gpg_ctx_op_complete fails to detect when gpg has
> > > > finished and the users of these functions run in an endless loop.
> > > 
> > > When can this condition ever happen?
> > 
> > When gpg_ctx_op_step is called with gpg->ostream being NULL.
> > gpg_verify and gpg_import_keys don't set them (it actually happened to
> > me when trying to verify a signature).
> 
> a few things:
> 
> 1) gpg_import_keys() is not yet in use, so this isn't a real concern.
> 
> 2) seen_eof1 is used differently in the case of gpg_verify(). Instead of
> being used as a eof marker, it is used to denote that we have gotten a
> trust metric for the data being verified. ie, we parsed a
> TRUST_[NEVER,MARGINAL,FULLY,ULTIMATE] status line from gpg's --status-fd
> 
> apparently you have discovered a bug in gpg, although the
> camel-gpg-context code is a bit dodgy in this case as well.
> 
> Jeff
-- 
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
fejj ximian com  - www.ximian.com

Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
retrieving revision 1.1798
diff -u -r1.1798 ChangeLog
--- ChangeLog	19 Apr 2003 03:15:49 -0000	1.1798
+++ ChangeLog	21 Apr 2003 19:17:26 -0000
@@ -1,3 +1,14 @@
+2003-04-21  Jeffrey Stedfast  <fejj ximian com>
+
+	* camel-gpg-context.c (gpg_ctx_parse_status): Don't set seen_eof1
+	here anymore once we get a trust metric.
+	(gpg_ctx_new): Init seen_eof1 to TRUE here.
+	(gpg_ctx_set_ostream): Change seen_eof1 to FALSE here this way we
+	only ever have to set this if we are expecting output.
+	(gpg_ctx_parse_status): Don't set seen_eof1 for importing either.
+	(gpg_ctx_op_step): Only FD_SET() those fd's that we have not yet
+	finished reading.
+
 2003-04-16  Jeffrey Stedfast  <fejj ximian com>
 
 	* camel-url-scanner.c (camel_url_web_end): Urls are unlikely to
Index: camel-gpg-context.c
===================================================================
RCS file: /cvs/gnome/evolution/camel/camel-gpg-context.c,v
retrieving revision 1.33
diff -u -r1.33 camel-gpg-context.c
--- camel-gpg-context.c	16 Apr 2003 17:02:10 -0000	1.33
+++ camel-gpg-context.c	21 Apr 2003 19:17:26 -0000
@@ -300,7 +300,7 @@
 	camel_object_ref (CAMEL_OBJECT (session));
 	gpg->userid_hint = g_hash_table_new (g_str_hash, g_str_equal);
 	gpg->complete = FALSE;
-	gpg->seen_eof1 = FALSE;
+	gpg->seen_eof1 = TRUE;
 	gpg->seen_eof2 = FALSE;
 	gpg->pid = (pid_t) -1;
 	gpg->exit_status = 0;
@@ -424,6 +424,7 @@
 	if (gpg->ostream)
 		camel_object_unref (CAMEL_OBJECT (gpg->ostream));
 	gpg->ostream = ostream;
+	gpg->seen_eof1 = FALSE;
 }
 
 static const char *
@@ -875,11 +876,6 @@
 				} else if (!strncmp (status, "ULTIMATE", 8)) {
 					gpg->trust = GPG_TRUST_ULTIMATE;
 				}
-				
-				/* Since verifying a signature will never produce output
-				   on gpg's stdout descriptor, we use this EOF bit for
-				   making sure that we get a TRUST metric. */
-				gpg->seen_eof1 = TRUE;
 			} else if (!strncmp (status, "VALIDSIG", 8)) {
 				gpg->validsig = TRUE;
 			} else if (!strncmp (status, "BADSIG", 6)) {
@@ -908,11 +904,7 @@
 			}
 			break;
 		case GPG_CTX_MODE_IMPORT:
-			/* hack to work around the fact that gpg
-                           doesn't write anything to stdout when
-                           importing keys */
-			if (!strncmp (status, "IMPORT_RES", 10))
-				gpg->seen_eof1 = TRUE;
+			/* noop */
 			break;
 		case GPG_CTX_MODE_EXPORT:
 			/* noop */
@@ -969,12 +961,21 @@
 	
  retry:
 	FD_ZERO (&rdset);
-	FD_SET (gpg->stdout_fd, &rdset);
-	FD_SET (gpg->stderr_fd, &rdset);
-	FD_SET (gpg->status_fd, &rdset);
 	
-	maxfd = MAX (gpg->stdout_fd, gpg->stderr_fd);
-	maxfd = MAX (maxfd, gpg->status_fd);
+	if (!gpg->seen_eof1) {
+		FD_SET (gpg->stdout_fd, &rdset);
+		maxfd = MAX (maxfd, gpg->stdout_fd);
+	}
+	
+	if (!gpg->seen_eof2) {
+		FD_SET (gpg->stderr_fd, &rdset);
+		maxfd = MAX (maxfd, gpg->stderr_fd);
+	}
+	
+	if (!gpg->complete) {
+		FD_SET (gpg->status_fd, &rdset);
+		maxfd = MAX (maxfd, gpg->status_fd);
+	}
 	
 	if (gpg->stdin_fd != -1 || gpg->passwd_fd != -1) {
 		FD_ZERO (&wrset);
@@ -990,6 +991,8 @@
 		wrsetp = &wrset;
 	}
 	
+	g_assert (maxfd > 0);
+	
 	timeout.tv_sec = 10; /* timeout in seconds */
 	timeout.tv_usec = 0;
 	
@@ -1184,6 +1187,7 @@
 	return gpg->complete && gpg->seen_eof1 && gpg->seen_eof2;
 }
 
+#if 0
 static gboolean
 gpg_ctx_op_exited (struct _GpgCtx *gpg)
 {
@@ -1202,6 +1206,7 @@
 	
 	return FALSE;
 }
+#endif
 
 static void
 gpg_ctx_op_cancel (struct _GpgCtx *gpg)