Minor cleanup to mime-node.c

Minor cleanup to mime-node.c

[Daniel Kahn Gillmor]

This simple 2-patch series is a bit of cleanup that i noticed while
completing the work on handling S/MIME messages.

It's not strictly part of the S/MIME series, so breaking out this
minor cleanup separately should make it easier to review.

Regards,

        --dkg

[PATCH 1/2] mime-node: rename decrypted_child to unwrapped_child

[Daniel Kahn Gillmor]

When walking the MIME tree, we might need to extract a new MIME
object.  Thus far, we've only done it when decrypting
multipart/encrypted messages, but PKCS#7 (RFC 8551, S/MIME) has
several other transformations that warrant a comparable form of
unwrapping.

Make this member re-usable for PKCS#7 unwrappings as well as
multipart/encrypted decryptions.

This change is just a naming change, it has no effect on function.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 mime-node.c      | 10 +++++-----
 notmuch-client.h |  6 ++++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index e531078c..2a823dfd 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -227,19 +227,19 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part)
     GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
     notmuch_message_t *message = NULL;
 
-    if (! node->decrypted_child) {
+    if (! node->unwrapped_child) {
 	for (mime_node_t *parent = node; parent; parent = parent->parent)
 	    if (parent->envelope_file) {
 		message = parent->envelope_file;
 		break;
 	    }
 
-	node->decrypted_child = _notmuch_crypto_decrypt (&node->decrypt_attempted,
+	node->unwrapped_child = _notmuch_crypto_decrypt (&node->decrypt_attempted,
 							 node->ctx->crypto->decrypt,
 							 message,
 							 encrypteddata, &decrypt_result, &err);
     }
-    if (! node->decrypted_child) {
+    if (! node->unwrapped_child) {
 	fprintf (stderr, "Failed to decrypt part: %s\n",
 		 err ? err->message : "no error explanation given");
 	goto DONE;
@@ -380,8 +380,8 @@ mime_node_child (mime_node_t *parent, int child)
 	return NULL;
 
     if (GMIME_IS_MULTIPART (parent->part)) {
-	if (child == GMIME_MULTIPART_ENCRYPTED_CONTENT && parent->decrypted_child)
-	    sub = parent->decrypted_child;
+	if (child == GMIME_MULTIPART_ENCRYPTED_CONTENT && parent->unwrapped_child)
+	    sub = parent->unwrapped_child;
 	else
 	    sub = g_mime_multipart_get_part (
 		GMIME_MULTIPART (parent->part), child);
diff --git a/notmuch-client.h b/notmuch-client.h
index 74690054..89e15ba6 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -395,8 +395,10 @@ struct mime_node {
     struct mime_node_context *ctx;
 
     /* Internal: For successfully decrypted multipart parts, the
-     * decrypted part to substitute for the second child. */
-    GMimeObject *decrypted_child;
+     * decrypted part to substitute for the second child; or, for
+     * PKCS#7 parts, the part returned after removing/processing the
+     * PKCS#7 transformation */
+    GMimeObject *unwrapped_child;
 
     /* Internal: The next child for depth-first traversal and the part
      * number to assign it (or -1 if unknown). */
-- 
2.25.1

Re: [PATCH 1/2] mime-node: rename decrypted_child to unwrapped_child

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> When walking the MIME tree, we might need to extract a new MIME
> object.  Thus far, we've only done it when decrypting
> multipart/encrypted messages, but PKCS#7 (RFC 8551, S/MIME) has
> several other transformations that warrant a comparable form of
> unwrapping.

pushed

d

[PATCH 2/2] mime-node: Clean up unwrapped MIME parts correctly.

[Daniel Kahn Gillmor]

Avoid a memory leak in the notmuch command line.

gmime_multipart_encrypted_decrypt returns a GMimeObject marked by
GMime as "transfer full", so we are supposed to clean up after it.

When parsing a message, notmuch would leak one GMimeObject part per
multipart/encrypted MIME layer.  We clean it up by analogy with
cleaning up the signature list associated with a MIME node.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 mime-node.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/mime-node.c b/mime-node.c
index 2a823dfd..ff6805bf 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -192,6 +192,26 @@ set_signature_list_destructor (mime_node_t *node)
     }
 }
 
+/* Unwrapped MIME part destructor */
+static int
+_unwrapped_child_free (GMimeObject **proxy)
+{
+    g_object_unref (*proxy);
+    return 0;
+}
+
+/* Set up unwrapped MIME part destructor */
+static void
+set_unwrapped_child_destructor (mime_node_t *node)
+{
+    GMimeObject **proxy = talloc (node, GMimeObject *);
+
+    if (proxy) {
+	*proxy = node->unwrapped_child;
+	talloc_set_destructor (proxy, _unwrapped_child_free);
+    }
+}
+
 /* Verify a signed mime node */
 static void
 node_verify (mime_node_t *node, GMimeObject *part)
@@ -238,6 +258,8 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part)
 							 node->ctx->crypto->decrypt,
 							 message,
 							 encrypteddata, &decrypt_result, &err);
+	if (node->unwrapped_child)
+	    set_unwrapped_child_destructor (node);
     }
     if (! node->unwrapped_child) {
 	fprintf (stderr, "Failed to decrypt part: %s\n",
-- 
2.25.1

Re: [PATCH 2/2] mime-node: Clean up unwrapped MIME parts correctly.

[Tomi Ollila]

On Thu, Mar 19 2020, Daniel Kahn Gillmor wrote:

> Avoid a memory leak in the notmuch command line.
>
> gmime_multipart_encrypted_decrypt returns a GMimeObject marked by
> GMime as "transfer full", so we are supposed to clean up after it.
>
> When parsing a message, notmuch would leak one GMimeObject part per
> multipart/encrypted MIME layer.  We clean it up by analogy with
> cleaning up the signature list associated with a MIME node.
>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>

Looks good to me.

Tomi

> ---
>  mime-node.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/mime-node.c b/mime-node.c
> index 2a823dfd..ff6805bf 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -192,6 +192,26 @@ set_signature_list_destructor (mime_node_t *node)
>      }
>  }
>  
> +/* Unwrapped MIME part destructor */
> +static int
> +_unwrapped_child_free (GMimeObject **proxy)
> +{
> +    g_object_unref (*proxy);
> +    return 0;
> +}
> +
> +/* Set up unwrapped MIME part destructor */
> +static void
> +set_unwrapped_child_destructor (mime_node_t *node)
> +{
> +    GMimeObject **proxy = talloc (node, GMimeObject *);
> +
> +    if (proxy) {
> +	*proxy = node->unwrapped_child;
> +	talloc_set_destructor (proxy, _unwrapped_child_free);
> +    }
> +}
> +
>  /* Verify a signed mime node */
>  static void
>  node_verify (mime_node_t *node, GMimeObject *part)
> @@ -238,6 +258,8 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part)
>  							 node->ctx->crypto->decrypt,
>  							 message,
>  							 encrypteddata, &decrypt_result, &err);
> +	if (node->unwrapped_child)
> +	    set_unwrapped_child_destructor (node);
>      }
>      if (! node->unwrapped_child) {
>  	fprintf (stderr, "Failed to decrypt part: %s\n",
> -- 
> 2.25.1