From: Jani Nikula <jani@nikula.org>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/4] Introduce a generic tree-like abstraction for MIME
	traversal.
In-Reply-To: <1322446871-14986-3-git-send-email-amdragon@mit.edu>
References: <1322446871-14986-1-git-send-email-amdragon@mit.edu>
	<1322446871-14986-3-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.10+51~gef3ae74 (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Tue, 29 Nov 2011 21:11:49 +0200
Message-ID: <8739d6u4ju.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Cc: dkg@fifthhorseman.net
Precedence: list


Hi, generally looks good to me, but please find a few comments below.

BR,
Jani.

On Sun, 27 Nov 2011 21:21:09 -0500, Austin Clements <amdragon@MIT.EDU> wrot=
e:
> This wraps all of the complex MIME part handling in a single, simple
> function that gets part N from *any* MIME object, so traversing a MIME
> part tree becomes a two-line for loop.  Furthermore, the MIME node
> structure provides easy access to envelopes for message parts as well
> as cryptographic information.
>=20
> This code is directly derived from the current show_message_body code
> (much of it is identical), but the control relation is inverted:
> instead of show_message_body controlling the traversal of the MIME
> structure and invoking callbacks, the caller controls the traversal of
> the MIME structure.
> ---
>  Makefile.local   |    1 +
>  mime-node.c      |  234 ++++++++++++++++++++++++++++++++++++++++++++++++=
++++++
>  notmuch-client.h |   80 ++++++++++++++++++
>  3 files changed, 315 insertions(+), 0 deletions(-)
>  create mode 100644 mime-node.c
>=20
> diff --git a/Makefile.local b/Makefile.local
> index c94402b..c46ed26 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -312,6 +312,7 @@ notmuch_client_srcs =3D		\
>  	notmuch-time.c		\
>  	query-string.c		\
>  	show-message.c		\
> +	mime-node.c		\
>  	json.c
>=20=20
>  notmuch_client_modules =3D $(notmuch_client_srcs:.c=3D.o)
> diff --git a/mime-node.c b/mime-node.c
> new file mode 100644
> index 0000000..942738b
> --- /dev/null
> +++ b/mime-node.c
> @@ -0,0 +1,234 @@
> +/* notmuch - Not much of an email program, (just index and search)
> + *
> + * Copyright =C2=A9 2009 Carl Worth
> + * Copyright =C2=A9 2009 Keith Packard
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Authors: Carl Worth <cworth@cworth.org>
> + *          Keith Packard <keithp@keithp.com>
> + *          Austin Clements <aclements@csail.mit.edu>
> + */
> +
> +#include "notmuch-client.h"
> +
> +/* Context that gets inherited from the root node. */
> +typedef struct mime_node_context {
> +    /* Per-message resources.  These are allocated internally and must
> +     * be destroyed. */
> +    FILE *file;
> +    GMimeStream *stream;
> +    GMimeParser *parser;
> +    GMimeMessage *mime_message;
> +=20=20=20=20

Leftover indentation spaces above.

> +    /* Context provided by the caller. */
> +    GMimeCipherContext *cryptoctx;
> +    notmuch_bool_t decrypt;
> +} mime_node_context_t;
> +
> +static int
> +_mime_node_context_free (mime_node_context_t *res)
> +{
> +    if (res->mime_message)
> +	g_object_unref (res->mime_message);
> +
> +    if (res->parser)
> +	g_object_unref (res->parser);
> +
> +    if (res->stream)
> +	g_object_unref (res->stream);
> +
> +    if (res->file)
> +	fclose (res->file);
> +
> +    return 0;
> +}
> +
> +notmuch_status_t
> +mime_node_open (const void *ctx, notmuch_message_t *message,
> +		GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,

The style here seems to be * next to the variable name, not type.

> +		mime_node_t **root_out)
> +{
> +    const char *filename =3D notmuch_message_get_filename (message);
> +    mime_node_context_t *mctx;
> +    mime_node_t *root =3D NULL;

No need to initialize as it's initialized right away below.

> +    notmuch_status_t status;
> +
> +    root =3D talloc_zero (ctx, mime_node_t);
> +    if (root =3D=3D NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	status =3D NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    /* Create the tree-wide context */
> +    mctx =3D talloc_zero (root, mime_node_context_t);
> +    if (mctx =3D=3D NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	status =3D NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +    talloc_set_destructor (mctx, _mime_node_context_free);
> +
> +    mctx->file =3D fopen (filename, "r");
> +    if (! mctx->file) {
> +	fprintf (stderr, "Error opening %s: %s\n", filename, strerror (errno));
> +	status =3D NOTMUCH_STATUS_FILE_ERROR;
> +	goto DONE;
> +    }
> +
> +    mctx->stream =3D g_mime_stream_file_new (mctx->file);

AFAICT the GMimeStreamFile object owns the FILE * pointer now, and
closes it later. Calling fclose() on it in _mime_node_context_free()
would be undefined behaviour. But please don't just take my word for it.

> +    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (mctx->stream), FALS=
E);
> +
> +    mctx->parser =3D g_mime_parser_new_with_stream (mctx->stream);
> +
> +    mctx->mime_message =3D g_mime_parser_construct_message (mctx->parser=
);
> +
> +    mctx->cryptoctx =3D cryptoctx;
> +    mctx->decrypt =3D decrypt;
> +
> +    /* Create the root node */
> +    root->part =3D GMIME_OBJECT (mctx->mime_message);
> +    root->envelope_file =3D message;
> +    root->children =3D 1;
> +    root->ctx =3D mctx;
> +
> +    *root_out =3D root;
> +    return NOTMUCH_STATUS_SUCCESS;
> +
> +DONE:
> +    talloc_free (root);
> +    return status;
> +}
> +
> +static int
> +_signature_validity_free (GMimeSignatureValidity **proxy)
> +{
> +    g_mime_signature_validity_free (*proxy);
> +    return 0;
> +}
> +
> +static mime_node_t *
> +_mime_node_create (const mime_node_t *parent, GMimeObject *part)
> +{
> +    mime_node_t *out =3D talloc_zero (parent, mime_node_t);
> +    GError *err =3D NULL;
> +
> +    /* Set basic node properties */
> +    out->part =3D part;
> +    out->ctx =3D parent->ctx;
> +    if (!talloc_reference (out, out->ctx)) {
> +	fprintf (stderr, "Out of memory.\n");
> +	talloc_free (out);
> +	return NULL;
> +    }
> +
> +    /* Deal with the different types of parts */
> +    if (GMIME_IS_PART (part)) {
> +	out->children =3D 0;
> +    } else if (GMIME_IS_MULTIPART (part)) {
> +	out->children =3D g_mime_multipart_get_count (GMIME_MULTIPART (part));
> +    } else if (GMIME_IS_MESSAGE_PART (part)) {
> +	/* Promote part to an envelope and open it */
> +	GMimeMessagePart *message_part =3D GMIME_MESSAGE_PART (part);
> +	GMimeMessage *message =3D g_mime_message_part_get_message (message_part=
);
> +	out->envelope_part =3D message_part;
> +	out->part =3D GMIME_OBJECT (message);
> +	out->children =3D 1;
> +    } else {
> +	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
> +		 g_type_name (G_OBJECT_TYPE (part)));
> +	talloc_free (out);
> +	return NULL;
> +    }
> +
> +    /* Handle PGP/MIME parts */
> +    if (GMIME_IS_MULTIPART_ENCRYPTED (part) && out->ctx->decrypt) {
> +	if (out->children !=3D 2) {
> +	    /* this violates RFC 3156 section 4, so we won't bother with it. */
> +	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
> +		     "message (should be exactly 2)\n",
> +		     out->children);
> +	} else {
> +	    out->is_encrypted =3D TRUE;
> +	    GMimeMultipartEncrypted *encrypteddata =3D
> +		GMIME_MULTIPART_ENCRYPTED (part);
> +	    out->decrypted_child =3D g_mime_multipart_encrypted_decrypt
> +		(encrypteddata, out->ctx->cryptoctx, &err);
> +	    if (out->decrypted_child) {
> +		out->decrypt_success =3D TRUE;
> +		out->is_signed =3D TRUE;
> +		out->sig_validity =3D g_mime_multipart_encrypted_get_signature_validit=
y (encrypteddata);
> +	    } else {
> +		fprintf (stderr, "Failed to decrypt part: %s\n",
> +			 (err ? err->message : "no error explanation given"));
> +	    }
> +	}
> +    } else if (GMIME_IS_MULTIPART_SIGNED (part) && out->ctx->cryptoctx) {
> +	if (out->children !=3D 2) {
> +	    /* this violates RFC 3156 section 5, so we won't bother with it. */
> +	    fprintf (stderr, "Error: %d part(s) for a multipart/signed message "
> +		     "(should be exactly 2)\n",
> +		     out->children);
> +	} else {
> +	    out->is_signed =3D TRUE;
> +	    /* For some reason the GMimeSignatureValidity returned
> +	     * here is not a const (inconsistent with that
> +	     * returned by
> +	     * g_mime_multipart_encrypted_get_signature_validity,
> +	     * and therefore needs to be properly disposed of.
> +	     * Hopefully the API will become more consistent. */
> +	    GMimeSignatureValidity *sig_validity =3D g_mime_multipart_signed_ve=
rify
> +		(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);
> +	    out->sig_validity =3D sig_validity;
> +	    if (sig_validity) {
> +		GMimeSignatureValidity **proxy =3D
> +		    talloc (out, GMimeSignatureValidity *);
> +		*proxy =3D sig_validity;
> +		talloc_set_destructor (proxy, _signature_validity_free);
> +	    }
> +	}
> +    }
> +
> +    if (out->is_signed && !out->sig_validity)
> +	fprintf (stderr, "Failed to verify signed part: %s\n",
> +		 (err ? err->message : "no error explanation given"));
> +
> +    if (err)
> +	g_error_free (err);
> +
> +    return out;
> +}
> +
> +mime_node_t *
> +mime_node_child (const mime_node_t *parent, int child)
> +{
> +    if (!parent || child < 0 || child >=3D parent->children)
> +	return NULL;
> +
> +    if (GMIME_IS_MULTIPART (parent->part)) {
> +	GMimeMultipart *multipart =3D GMIME_MULTIPART (parent->part);
> +	if (child =3D=3D 1 && parent->decrypted_child)
> +	    return _mime_node_create (parent, parent->decrypted_child);
> +	return _mime_node_create (parent, g_mime_multipart_get_part (multipart,=
 child));
> +    } else if (GMIME_IS_MESSAGE (parent->part)) {
> +	GMimeMessage *message =3D GMIME_MESSAGE (parent->part);
> +	GMimeObject *child =3D g_mime_message_get_mime_part (message);
> +	return _mime_node_create (parent, child);
> +    } else {
> +	/* This should have been caught by message_part_create */
> +	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
> +			g_type_name (G_OBJECT_TYPE (parent->part)));
> +    }
> +}
> diff --git a/notmuch-client.h b/notmuch-client.h
> index d7fb6ee..58bd21c 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -238,4 +238,84 @@ notmuch_config_set_maildir_synchronize_flags (notmuc=
h_config_t *config,
>  notmuch_bool_t
>  debugger_is_active (void);
>=20=20
> +/* mime-node.c */
> +
> +/* mime_node_t represents a single node in a MIME tree.  A MIME tree
> + * abstracts the different ways of traversing different types of MIME
> + * parts, allowing a MIME message to be viewed as a generic tree of
> + * parts.  Message-type parts have one child, multipart-type parts
> + * have multiple children, and leaf parts have zero children.
> + */
> +typedef struct mime_node {
> +    /* The MIME object of this part.  This will be a GMimeMessage,
> +     * GMimePart, GMimeMultipart, or a subclass of one of these.
> +     *
> +     * This will never be a GMimeMessagePart because GMimeMessagePart
> +     * is structurally redundant with GMimeMessage.  If this part is a
> +     * message (that is, 'part' is a GMimeMessage), then either
> +     * envelope_file will be set to a notmuch_message_t (for top-level
> +     * messages) or envelope_part will be set to a GMimeMessagePart
> +     * (for embedded message parts).
> +     */
> +    GMimeObject *part;
> +
> +    /* If part is a GMimeMessage, these record the envelope of the
> +     * message: either a notmuch_message_t representing a top-level
> +     * message, or a GMimeMessagePart representing a MIME part
> +     * containing a message.
> +     */
> +    notmuch_message_t *envelope_file;
> +    GMimeMessagePart *envelope_part;
> +
> +    /* The number of children of this part. */
> +    int children;
> +
> +    /* True if this is the container for an encrypted or signed part.
> +     * This does *not* mean that decryption or signature verification
> +     * succeeded. */
> +    notmuch_bool_t is_encrypted, is_signed;
> +    /* True if decryption of this part's child succeeded.  In this
> +     * case, the decrypted part is substituted for the second child of
> +     * this part (which would usually be the encrypted data). */
> +    notmuch_bool_t decrypt_success;
> +    /* For signed or encrypted containers, the validity of the
> +     * signature.  May be NULL if signature verification failed. */
> +    const GMimeSignatureValidity *sig_validity;
> +
> +    /* Internal: Context inherited from the root iterator. */
> +    struct mime_node_context *ctx;
> +
> +    /* Internal: For successfully decrypted multipart parts, the
> +     * decrypted part to substitute for the second child. */
> +    GMimeObject *decrypted_child;
> +} mime_node_t;
> +
> +/* Construct a new MIME node pointing to the root message part of
> + * message.  If cryptoctx is non-NULL, it will be used to verify
> + * signatures on any child parts.  If decrypt is true, then cryptoctx
> + * will additionally be used to decrypt any encrypted child parts.
> + *
> + * Return value:
> + *
> + * NOTMUCH_STATUS_SUCCESS: Root node is returned in *node_out.
> + *
> + * NOTMUCH_STATUS_FILE_ERROR: Failed to open message file.
> + *
> + * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
> + */
> +notmuch_status_t
> +mime_node_open (const void *ctx, notmuch_message_t *message,
> +		GMimeCipherContext* cryptoctx, notmuch_bool_t decrypt,
> +		mime_node_t **node_out);
> +
> +/* Return a new MIME node for the requested child part of parent.
> + * parent will be used as the talloc context for the returned child
> + * node.
> + *
> + * In case of any failure, this function returns NULL, (after printing
> + * an error message on stderr).
> + */
> +mime_node_t *
> +mime_node_child (const mime_node_t *parent, int child);
> +
>  #endif
> --=20
> 1.7.5.4
>=20
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch