[PATCH] Add pseudo-compatibility with gmime 2.6
Adrian Perez
aperez at igalia.com
Tue Jan 17 02:48:14 PST 2012
Hi,
Just a couple of comments inline :)
On Mon, 16 Jan 2012 22:47:14 -0500, Austin Clements <amdragon at MIT.EDU> wrote:
> Quoth Thomas Jost on Jan 17 at 12:56 am:
> > There are lots of API changes in gmime 2.6 crypto handling. By adding
> > preprocessor directives, it is however possible to add gmime 2.6 compatibility
> > while preserving compatibility with gmime 2.4 too.
>
> Awesome. Comments inline below.
>
> > This is mostly based on id:"8762i8hrb9.fsf at bookbinder.fernseed.info".
> >
> > This was tested against both gmime 2.6.4 and 2.4.31. With gmime 2.4.31, the
> > crypto tests all work fine (as expected). With gmime 2.6.4, one crypto test
> > fails (signature verification with signer key unavailable) but this will be hard
> > to fix since the new API does not report the reason why a signature verification
> > fails (other than the human-readable error message).
>
> What is the result of this failing test?
>
> > ---
> > mime-node.c | 50 ++++++++++++++++++++++++++-
> > notmuch-client.h | 27 ++++++++++++++-
> > notmuch-reply.c | 7 ++++
> > notmuch-show.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > show-message.c | 4 ++
> > 5 files changed, 181 insertions(+), 4 deletions(-)
> >
> > diff --git a/mime-node.c b/mime-node.c
> > index d26bb44..ae2473d 100644
> > --- a/mime-node.c
> > +++ b/mime-node.c
> > @@ -33,7 +33,11 @@ typedef struct mime_node_context {
> > GMimeMessage *mime_message;
> >
> > /* Context provided by the caller. */
> > +#ifdef GMIME_26
> > + GMimeCryptoContext *cryptoctx;
> > +#else
> > GMimeCipherContext *cryptoctx;
> > +#endif
> > notmuch_bool_t decrypt;
> > } mime_node_context_t;
> >
> > @@ -57,8 +61,12 @@ _mime_node_context_free (mime_node_context_t *res)
> >
> > notmuch_status_t
> > mime_node_open (const void *ctx, notmuch_message_t *message,
> > - GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
> > - mime_node_t **root_out)
> > +#ifdef GMIME_26
> > + GMimeCryptoContext *cryptoctx,
> > +#else
> > + GMimeCipherContext *cryptoctx,
> > +#endif
> > + notmuch_bool_t decrypt, mime_node_t **root_out)
> > {
> > const char *filename = notmuch_message_get_filename (message);
> > mime_node_context_t *mctx;
> > @@ -112,12 +120,21 @@ DONE:
> > return status;
> > }
> >
> > +#ifdef GMIME_26
> > +static int
> > +_signature_list_free (GMimeSignatureList **proxy)
> > +{
> > + g_object_unref (*proxy);
> > + return 0;
> > +}
> > +#else
> > static int
> > _signature_validity_free (GMimeSignatureValidity **proxy)
> > {
> > g_mime_signature_validity_free (*proxy);
> > return 0;
> > }
> > +#endif
> >
> > static mime_node_t *
> > _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > @@ -165,11 +182,23 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > GMimeMultipartEncrypted *encrypteddata =
> > GMIME_MULTIPART_ENCRYPTED (part);
> > node->decrypt_attempted = TRUE;
> > +#ifdef GMIME_26
> > + GMimeDecryptResult *decrypt_result = g_mime_decrypt_result_new ();
>
> I think g_mime_multipart_encrypted_decrypt allocates the
> GMimeDecryptResult for you, so this will just leak memory.
Yes, this is a leak. The documentation says that the function is
responsible of allocating the GMimeDecryptResult. The g_object_unref()
is still needed in notmuch's code to free it, though.
See http://git.gnome.org/browse/gmime/tree/gmime/gmime-multipart-encrypted.c#n282
> > + node->decrypted_child = g_mime_multipart_encrypted_decrypt
> > + (encrypteddata, node->ctx->cryptoctx, &decrypt_result, &err);
> > + if (node->decrypted_child) {
> > + node->decrypt_success = node->verify_attempted = TRUE;
> > + node->sig_list = g_mime_decrypt_result_get_signatures (decrypt_result);
> > + if (!node->sig_list)
> > + fprintf (stderr, "Failed to get signatures: %s\n",
> > + (err ? err->message : "no error explanation given"));
>
> My understanding is that g_mime_decrypt_result_get_signatures returns
> NULL if there are no signatures and that this isn't an error. This
> differs from 2.4, which would return an empty but non-NULL list.
>
> Also, I believe you have to free the sig_list in both branches now,
> which means the talloc_set_destructor can be moved to common logic
> outside of the if decrypted/signed.
>
> > +#else
> > node->decrypted_child = g_mime_multipart_encrypted_decrypt
> > (encrypteddata, node->ctx->cryptoctx, &err);
> > if (node->decrypted_child) {
> > node->decrypt_success = node->verify_attempted = TRUE;
> > node->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
> > +#endif
>
> It's confusing to have the open braces in the #ifdef'd region with a
> matching close brace outside of it (and I imagine this confuses
> editors and uncrustify, too). You could either copy the else part in
> both branches of the #ifdef or avoid duplicated code with something
> like
>
> #ifdef GMIME_26
> .. node->decrypted_child = ..
> #else
> .. node->decrypted_child = ..
> #endif
> if (node->decrypted_child) {
> node->decrypt_success = node->verify_attempted = TRUE;
> #ifdef GMIME_26
> node->sig_list = ..
> #else
> node->sig_validity = ..
> #endif
> } else {
> fprintf (stderr, ..);
> }
>
> > } else {
> > fprintf (stderr, "Failed to decrypt part: %s\n",
> > (err ? err->message : "no error explanation given"));
> > @@ -182,6 +211,18 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > "(must be exactly 2)\n",
> > node->nchildren);
> > } else {
> > +#ifdef GMIME_26
> > + GMimeSignatureList *sig_list = g_mime_multipart_signed_verify
> > + (GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, &err);
> > + node->verify_attempted = TRUE;
> > + node->sig_list = sig_list;
> > + if (sig_list) {
> > + GMimeSignatureList **proxy =
> > + talloc (node, GMimeSignatureList *);
> > + *proxy = sig_list;
> > + talloc_set_destructor (proxy, _signature_list_free);
> > + }
> > +#else
> > /* For some reason the GMimeSignatureValidity returned
> > * here is not a const (inconsistent with that
> > * returned by
> > @@ -200,10 +241,15 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > *proxy = sig_validity;
> > talloc_set_destructor (proxy, _signature_validity_free);
> > }
> > +#endif
> > }
> > }
> >
> > +#ifdef GMIME_26
> > + if (node->verify_attempted && !node->sig_list)
>
> Hmm. This is correct for signed parts, but will incorrectly trigger
> for an encrypted part with no signatures. For 2.6, I think this error
> checking may have to move into the branches of the if encrypted/signed
> since for encrypted parts you have to check if
> g_mime_multipart_encrypted_decrypt returned NULL.
>
> > +#else
> > if (node->verify_attempted && !node->sig_validity)
> > +#endif
> > fprintf (stderr, "Failed to verify signed part: %s\n",
> > (err ? err->message : "no error explanation given"));
> >
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index 517c010..e85f882 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -30,6 +30,12 @@
> >
> > #include <gmime/gmime.h>
> >
> > +/* GMIME_CHECK_VERSION is only available in gmime >= 2.5. But so are
> > + * GMIME_MAJOR_VERSION and friends. */
>
> Hah.
>
> > +#ifdef GMIME_MAJOR_VERSION
> > +#define GMIME_26
> > +#endif
> > +
> > #include "notmuch.h"
> >
> > /* This is separate from notmuch-private.h because we're trying to
> > @@ -69,7 +75,11 @@ typedef struct notmuch_show_format {
> > void (*part_start) (GMimeObject *part,
> > int *part_count);
> > void (*part_encstatus) (int status);
> > +#ifdef GMIME_26
> > + void (*part_sigstatus) (GMimeSignatureList* siglist);
> > +#else
> > void (*part_sigstatus) (const GMimeSignatureValidity* validity);
> > +#endif
> > void (*part_content) (GMimeObject *part);
> > void (*part_end) (GMimeObject *part);
> > const char *part_sep;
> > @@ -83,7 +93,11 @@ typedef struct notmuch_show_params {
> > int entire_thread;
> > int raw;
> > int part;
> > +#ifdef GMIME_26
> > + GMimeCryptoContext* cryptoctx;
> > +#else
> > GMimeCipherContext* cryptoctx;
> > +#endif
> > int decrypt;
> > } notmuch_show_params_t;
> >
> > @@ -286,7 +300,12 @@ typedef struct mime_node {
> > * signature. May be NULL if signature verification failed. If
> > * there are simply no signatures, this will be non-NULL with an
> > * empty signers list. */
> > +#ifdef GMIME_26
> > + /* TODO: update the above comment... */
>
> Since this behaves very differently in 2.6, I think documenting it is
> important (and being very careful about the differences). Maybe
>
> /* The list of signatures for signed or encrypted containers. If
> * there are no signatures, this will be NULL. */
>
> > + GMimeSignatureList* sig_list;
> > +#else
> > const GMimeSignatureValidity *sig_validity;
> > +#endif
> >
> > /* Internal: Context inherited from the root iterator. */
> > struct mime_node_context *ctx;
> > @@ -311,8 +330,12 @@ typedef struct mime_node {
> > */
> > notmuch_status_t
> > mime_node_open (const void *ctx, notmuch_message_t *message,
> > - GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
> > - mime_node_t **node_out);
> > +#ifdef GMIME_26
> > + GMimeCryptoContext *cryptoctx,
> > +#else
> > + GMimeCipherContext *cryptoctx,
> > +#endif
> > + 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
> > diff --git a/notmuch-reply.c b/notmuch-reply.c
> > index da3acce..dc37c51 100644
> > --- a/notmuch-reply.c
> > +++ b/notmuch-reply.c
> > @@ -688,15 +688,22 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
> > reply_format_func = notmuch_reply_format_default;
> >
> > if (decrypt) {
> > +#ifdef GMIME_26
> > + /* TODO: GMimePasswordRequestFunc */
> > + params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg");
> > +#else
> > GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
> > params.cryptoctx = g_mime_gpg_context_new (session, "gpg");
> > +#endif
> > if (params.cryptoctx) {
> > g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cryptoctx, FALSE);
> > params.decrypt = TRUE;
> > } else {
> > fprintf (stderr, "Failed to construct gpg context.\n");
> > }
> > +#ifndef GMIME_26
> > g_object_unref (session);
> > +#endif
> > }
> >
> > config = notmuch_config_open (ctx, NULL, NULL);
> > diff --git a/notmuch-show.c b/notmuch-show.c
> > index d14dac9..263ab72 100644
> > --- a/notmuch-show.c
> > +++ b/notmuch-show.c
> > @@ -76,7 +76,11 @@ static void
> > format_part_encstatus_json (int status);
> >
> > static void
> > +#ifdef GMIME_26
> > +format_part_sigstatus_json (GMimeSignatureList* siglist);
> > +#else
> > format_part_sigstatus_json (const GMimeSignatureValidity* validity);
> > +#endif
> >
> > static void
> > format_part_content_json (GMimeObject *part);
> > @@ -486,6 +490,21 @@ show_text_part_content (GMimeObject *part, GMimeStream *stream_out)
> > g_object_unref(stream_filter);
> > }
> >
> > +#ifdef GMIME_26
> > +static const char*
> > +signature_status_to_string (GMimeSignatureStatus x)
> > +{
> > + switch (x) {
> > + case GMIME_SIGNATURE_STATUS_GOOD:
> > + return "good";
> > + case GMIME_SIGNATURE_STATUS_BAD:
> > + return "bad";
> > + case GMIME_SIGNATURE_STATUS_ERROR:
> > + return "error";
> > + }
> > + return "unknown";
> > +}
> > +#else
> > static const char*
> > signer_status_to_string (GMimeSignerStatus x)
> > {
> > @@ -501,6 +520,7 @@ signer_status_to_string (GMimeSignerStatus x)
> > }
> > return "unknown";
> > }
> > +#endif
> >
> > static void
> > format_part_start_text (GMimeObject *part, int *part_count)
> > @@ -592,6 +612,75 @@ format_part_encstatus_json (int status)
> > printf ("}]");
> > }
> >
> > +#ifdef GMIME_26
> > +static void
> > +format_part_sigstatus_json (GMimeSignatureList *siglist)
> > +{
> > + printf (", \"sigstatus\": [");
> > +
> > + if (!siglist) {
> > + printf ("]");
> > + return;
> > + }
> > +
> > + void *ctx_quote = talloc_new (NULL);
> > + int i;
> > + for (i = 0; i < g_mime_signature_list_length (siglist); ++i) {
>
> Style nit: notmuch uses i++.
>
> > + GMimeSignature *signature = g_mime_signature_list_get_signature (siglist, i);
> > +
> > + if (i > 0)
> > + printf (", ");
> > +
> > + printf ("{");
> > +
> > + /* status */
> > + GMimeSignatureStatus status = g_mime_signature_get_status (signature);
> > + printf ("\"status\": %s",
> > + json_quote_str (ctx_quote,
> > + signature_status_to_string (status)));
> > +
> > + GMimeCertificate *certificate = g_mime_signature_get_certificate (signature);
> > + if (status == GMIME_SIGNATURE_STATUS_GOOD)
> > + {
>
> Style nit: break after brace.
>
> (Presumably this is copied from the existing
> format_part_sigstatus_json, but since it's technically new code
> there's no reason not to fix these up.)
>
> > + if (certificate)
> > + printf (", \"fingerprint\": %s", json_quote_str (ctx_quote, g_mime_certificate_get_fingerprint (certificate)));
> > + /* these dates are seconds since the epoch; should we
> > + * provide a more human-readable format string? */
> > + time_t created = g_mime_signature_get_created (signature);
> > + if (created != -1)
> > + printf (", \"created\": %d", (int) created);
> > + time_t expires = g_mime_signature_get_expires (signature);
> > + if (expires > 0)
> > + printf (", \"expires\": %d", (int) expires);
>
> Is it intentional that the two above checks are different? I would
> think the second should be expires != -1.
Yes, it should check for “expires != -1”. Also, wouldn't it be needed to
cast do “expires != (time_t) -1” to avoid compiler warnings? (time_t may
be different from int.)
> > + /* output user id only if validity is FULL or ULTIMATE. */
> > + /* note that gmime is using the term "trust" here, which
> > + * is WRONG. It's actually user id "validity". */
> > + if (certificate)
> > + {
>
> Break after brace.
>
> > + const char *name = g_mime_certificate_get_name (certificate);
> > + GMimeCertificateTrust trust = g_mime_certificate_get_trust (certificate);
> > + if (name && (trust == GMIME_CERTIFICATE_TRUST_FULLY || trust == GMIME_CERTIFICATE_TRUST_ULTIMATE))
> > + printf (", \"userid\": %s", json_quote_str (ctx_quote, name));
> > + }
> > + } else if (certificate) {
> > + const char *key_id = g_mime_certificate_get_key_id (certificate);
> > + if (key_id)
> > + printf (", \"keyid\": %s", json_quote_str (ctx_quote, key_id));
> > + }
> > +
> > + GMimeSignatureError errors = g_mime_signature_get_errors (signature);
> > + if (errors != GMIME_SIGNATURE_ERROR_NONE) {
> > + printf (", \"errors\": %x", errors);
>
> This should be %d (I would say 0x%x, but JSON doesn't support hex
> literals). I see this bug came from the original
> format_part_sigstatus_json. Maybe there should be a quick patch
> before this one that fixes the source of the bug?
>
> > + }
> > +
> > + printf ("}");
> > + }
> > +
> > + printf ("]");
> > +
> > + talloc_free (ctx_quote);
> > +}
> > +#else
> > static void
> > format_part_sigstatus_json (const GMimeSignatureValidity* validity)
> > {
> > @@ -652,6 +741,7 @@ format_part_sigstatus_json (const GMimeSignatureValidity* validity)
> >
> > talloc_free (ctx_quote);
> > }
> > +#endif
> >
> > static void
> > format_part_content_json (GMimeObject *part)
> > @@ -990,13 +1080,20 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
> > } else if ((STRNCMP_LITERAL (argv[i], "--verify") == 0) ||
> > (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)) {
> > if (params.cryptoctx == NULL) {
> > +#ifdef GMIME_26
> > + /* TODO: GMimePasswordRequestFunc */
> > + if (NULL == (params.cryptoctx = g_mime_gpg_context_new(NULL, "gpg")))
> > +#else
> > GMimeSession* session = g_object_new(g_mime_session_get_type(), NULL);
> > if (NULL == (params.cryptoctx = g_mime_gpg_context_new(session, "gpg")))
> > +#endif
> > fprintf (stderr, "Failed to construct gpg context.\n");
> > else
> > g_mime_gpg_context_set_always_trust((GMimeGpgContext*)params.cryptoctx, FALSE);
> > +#ifndef GMIME_26
> > g_object_unref (session);
> > session = NULL;
> > +#endif
> > }
> > if (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)
> > params.decrypt = 1;
> > diff --git a/show-message.c b/show-message.c
> > index 8768889..65269fd 100644
> > --- a/show-message.c
> > +++ b/show-message.c
> > @@ -48,7 +48,11 @@ show_message_part (mime_node_t *node,
> > format->part_encstatus (node->decrypt_success);
> >
> > if (node->verify_attempted && format->part_sigstatus)
> > +#ifdef GMIME_26
> > + format->part_sigstatus (node->sig_list);
> > +#else
> > format->part_sigstatus (node->sig_validity);
> > +#endif
> >
> > format->part_content (part);
> >
--
Adrian Perez <aperez at igalia.com> - Sent from my toaster
Igalia - Free Software Engineering
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120117/84d3b127/attachment.pgp>
More information about the notmuch
mailing list