From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 47473429E35 for ; Tue, 17 Jan 2012 02:46:05 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.7 X-Spam-Level: X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zmvkwppDBWQ0 for ; Tue, 17 Jan 2012 02:46:01 -0800 (PST) Received: from smtp4.mundo-r.com (smtp4.mundo-r.com [212.51.32.151]) by olra.theworths.org (Postfix) with ESMTP id 381A6429E2F for ; Tue, 17 Jan 2012 02:46:00 -0800 (PST) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: Ap8EAANRFU9bdWOb/2dsb2JhbABCAoURpyuCC4FyAQEFIwQvMwsYCQ0UAgIPASkNERmHfAakVZFiiQcXQgEFCAUEEQUBBgEBBgEFLQIBCQEBAS4KTQ0FGBEKgWkCBgE3EQsBgX6BFgSVEZJD X-IronPort-AV: E=Sophos;i="4.71,522,1320620400"; d="scan'208";a="862337471" Received: from 155.99.117.91.static.mundo-r.com (HELO fanzine.igalia.com) ([91.117.99.155]) by smtp4.mundo-r.com with ESMTP; 17 Jan 2012 11:45:58 +0100 Received: from dsl-hkibrasgw3-fe7cfb00-20.dhcp.inet.fi ([84.251.124.20] helo=kaze.ara-ara.org) by fanzine.igalia.com with esmtpsa (Cipher TLS1.0:RSA_ARCFOUR_SHA1:16) (Exim) id 1Rn6Y9-0003E3-Vm for ; Tue, 17 Jan 2012 11:45:58 +0100 Received: (nullmailer pid 975 invoked by uid 1000); Tue, 17 Jan 2012 10:48:18 -0000 From: Adrian Perez To: notmuch@notmuchmail.org Subject: Re: [PATCH] Add pseudo-compatibility with gmime 2.6 In-Reply-To: <20120117034714.GG16740@mit.edu> References: <8762gbtd6p.fsf@schnouki.net> <1326758199-18058-1-git-send-email-schnouki@schnouki.net> <20120117034714.GG16740@mit.edu> User-Agent: Notmuch/0.11 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-unknown-linux-gnu) Date: Tue, 17 Jan 2012 12:48:14 +0200 Message-ID: <87ty3u38r5.fsf@igalia.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2012 10:46:05 -0000 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi,=20 Just a couple of comments inline :) On Mon, 16 Jan 2012 22:47:14 -0500, Austin Clements wrot= e: > 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 compat= ibility > > while preserving compatibility with gmime 2.4 too. >=20 > Awesome. Comments inline below. >=20 > > This is mostly based on id:"8762i8hrb9.fsf@bookbinder.fernseed.info". > >=20 > > 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 wil= l be hard > > to fix since the new API does not report the reason why a signature ver= ification > > fails (other than the human-readable error message). >=20 > What is the result of this failing test? >=20 > > --- > > 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(-) > >=20 > > 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; > >=20=20 > > /* Context provided by the caller. */ > > +#ifdef GMIME_26 > > + GMimeCryptoContext *cryptoctx; > > +#else > > GMimeCipherContext *cryptoctx; > > +#endif > > notmuch_bool_t decrypt; > > } mime_node_context_t; > >=20=20 > > @@ -57,8 +61,12 @@ _mime_node_context_free (mime_node_context_t *res) > >=20=20 > > 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 =3D notmuch_message_get_filename (message); > > mime_node_context_t *mctx; > > @@ -112,12 +120,21 @@ DONE: > > return status; > > } > >=20=20 > > +#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 > >=20=20 > > 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, GMi= meObject *part) > > GMimeMultipartEncrypted *encrypteddata =3D > > GMIME_MULTIPART_ENCRYPTED (part); > > node->decrypt_attempted =3D TRUE; > > +#ifdef GMIME_26 > > + GMimeDecryptResult *decrypt_result =3D g_mime_decrypt_result_new = (); >=20 > 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 =3D g_mime_multipart_encrypted_decrypt > > + (encrypteddata, node->ctx->cryptoctx, &decrypt_result, &err); > > + if (node->decrypted_child) { > > + node->decrypt_success =3D node->verify_attempted =3D TRUE; > > + node->sig_list =3D g_mime_decrypt_result_get_signatures (decrypt_res= ult); > > + if (!node->sig_list) > > + fprintf (stderr, "Failed to get signatures: %s\n", > > + (err ? err->message : "no error explanation given")); >=20 > 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. >=20 > 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. >=20 > > +#else > > node->decrypted_child =3D g_mime_multipart_encrypted_decrypt > > (encrypteddata, node->ctx->cryptoctx, &err); > > if (node->decrypted_child) { > > node->decrypt_success =3D node->verify_attempted =3D TRUE; > > node->sig_validity =3D g_mime_multipart_encrypted_get_signature_vali= dity (encrypteddata); > > +#endif >=20 > 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 >=20 > #ifdef GMIME_26 > .. node->decrypted_child =3D .. > #else > .. node->decrypted_child =3D .. > #endif > if (node->decrypted_child) { > node->decrypt_success =3D node->verify_attempted =3D TRUE; > #ifdef GMIME_26 > node->sig_list =3D .. > #else > node->sig_validity =3D .. > #endif > } else { > fprintf (stderr, ..); > } >=20 > > } 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, GMim= eObject *part) > > "(must be exactly 2)\n", > > node->nchildren); > > } else { > > +#ifdef GMIME_26 > > + GMimeSignatureList *sig_list =3D g_mime_multipart_signed_verify > > + (GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, &err); > > + node->verify_attempted =3D TRUE; > > + node->sig_list =3D sig_list; > > + if (sig_list) { > > + GMimeSignatureList **proxy =3D > > + talloc (node, GMimeSignatureList *); > > + *proxy =3D 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, GMi= meObject *part) > > *proxy =3D sig_validity; > > talloc_set_destructor (proxy, _signature_validity_free); > > } > > +#endif > > } > > } > >=20=20 > > +#ifdef GMIME_26 > > + if (node->verify_attempted && !node->sig_list) >=20 > 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. >=20 > > +#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")); > >=20=20 > > 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 @@ > >=20=20 > > #include > >=20=20 > > +/* GMIME_CHECK_VERSION is only available in gmime >=3D 2.5. But so are > > + * GMIME_MAJOR_VERSION and friends. */ >=20 > Hah. >=20 > > +#ifdef GMIME_MAJOR_VERSION > > +#define GMIME_26 > > +#endif > > + > > #include "notmuch.h" > >=20=20 > > /* 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; > >=20=20 > > @@ -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... */ >=20 > Since this behaves very differently in 2.6, I think documenting it is > important (and being very careful about the differences). Maybe >=20 > /* The list of signatures for signed or encrypted containers. If > * there are no signatures, this will be NULL. */ >=20 > > + GMimeSignatureList* sig_list; > > +#else > > const GMimeSignatureValidity *sig_validity; > > +#endif > >=20=20 > > /* 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); > >=20=20 > > /* 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 =3D notmuch_reply_format_default; > >=20=20 > > if (decrypt) { > > +#ifdef GMIME_26 > > + /* TODO: GMimePasswordRequestFunc */ > > + params.cryptoctx =3D g_mime_gpg_context_new (NULL, "gpg"); > > +#else > > GMimeSession* session =3D g_object_new (g_mime_session_get_type(), NU= LL); > > params.cryptoctx =3D g_mime_gpg_context_new (session, "gpg"); > > +#endif > > if (params.cryptoctx) { > > g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cr= yptoctx, FALSE); > > params.decrypt =3D TRUE; > > } else { > > fprintf (stderr, "Failed to construct gpg context.\n"); > > } > > +#ifndef GMIME_26 > > g_object_unref (session); > > +#endif > > } > >=20=20 > > config =3D 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); > >=20=20 > > static void > > +#ifdef GMIME_26 > > +format_part_sigstatus_json (GMimeSignatureList* siglist); > > +#else > > format_part_sigstatus_json (const GMimeSignatureValidity* validity); > > +#endif > >=20=20 > > static void > > format_part_content_json (GMimeObject *part); > > @@ -486,6 +490,21 @@ show_text_part_content (GMimeObject *part, GMimeSt= ream *stream_out) > > g_object_unref(stream_filter); > > } > >=20=20 > > +#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 > >=20=20 > > static void > > format_part_start_text (GMimeObject *part, int *part_count) > > @@ -592,6 +612,75 @@ format_part_encstatus_json (int status) > > printf ("}]"); > > } > >=20=20 > > +#ifdef GMIME_26 > > +static void > > +format_part_sigstatus_json (GMimeSignatureList *siglist) > > +{ > > + printf (", \"sigstatus\": ["); > > + > > + if (!siglist) { > > + printf ("]"); > > + return; > > + } > > + > > + void *ctx_quote =3D talloc_new (NULL); > > + int i; > > + for (i =3D 0; i < g_mime_signature_list_length (siglist); ++i) { >=20 > Style nit: notmuch uses i++. >=20 > > + GMimeSignature *signature =3D g_mime_signature_list_get_signature (si= glist, i); > > + > > + if (i > 0) > > + printf (", "); > > + > > + printf ("{"); > > + > > + /* status */ > > + GMimeSignatureStatus status =3D g_mime_signature_get_status (signatur= e); > > + printf ("\"status\": %s", > > + json_quote_str (ctx_quote, > > + signature_status_to_string (status))); > > + > > + GMimeCertificate *certificate =3D g_mime_signature_get_certificate (s= ignature); > > + if (status =3D=3D GMIME_SIGNATURE_STATUS_GOOD) > > + { >=20 > Style nit: break after brace. >=20 > (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.) >=20 > > + if (certificate) > > + printf (", \"fingerprint\": %s", json_quote_str (ctx_quote, g_mime_c= ertificate_get_fingerprint (certificate))); > > + /* these dates are seconds since the epoch; should we > > + * provide a more human-readable format string? */ > > + time_t created =3D g_mime_signature_get_created (signature); > > + if (created !=3D -1) > > + printf (", \"created\": %d", (int) created); > > + time_t expires =3D g_mime_signature_get_expires (signature); > > + if (expires > 0) > > + printf (", \"expires\": %d", (int) expires); >=20 > Is it intentional that the two above checks are different? I would > think the second should be expires !=3D -1. Yes, it should check for =E2=80=9Cexpires !=3D -1=E2=80=9D. Also, wouldn't = it be needed to cast do =E2=80=9Cexpires !=3D (time_t) -1=E2=80=9D to avoid compiler warnin= gs? (time_t may be different from int.) =20 > > + /* 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) > > + { >=20 > Break after brace. >=20 > > + const char *name =3D g_mime_certificate_get_name (certificate); > > + GMimeCertificateTrust trust =3D g_mime_certificate_get_trust (certif= icate); > > + if (name && (trust =3D=3D GMIME_CERTIFICATE_TRUST_FULLY || trust =3D= =3D GMIME_CERTIFICATE_TRUST_ULTIMATE)) > > + printf (", \"userid\": %s", json_quote_str (ctx_quote, name)); > > + } > > + } else if (certificate) { > > + const char *key_id =3D g_mime_certificate_get_key_id (certificate= ); > > + if (key_id) > > + printf (", \"keyid\": %s", json_quote_str (ctx_quote, key_id)); > > + } > > + > > + GMimeSignatureError errors =3D g_mime_signature_get_errors (signature= ); > > + if (errors !=3D GMIME_SIGNATURE_ERROR_NONE) { > > + printf (", \"errors\": %x", errors); >=20 > 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? >=20 > > + } > > + > > + 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 GMimeSignatureVal= idity* validity) > >=20=20 > > talloc_free (ctx_quote); > > } > > +#endif > >=20=20 > > static void > > format_part_content_json (GMimeObject *part) > > @@ -990,13 +1080,20 @@ notmuch_show_command (void *ctx, unused (int arg= c), unused (char *argv[])) > > } else if ((STRNCMP_LITERAL (argv[i], "--verify") =3D=3D 0) || > > (STRNCMP_LITERAL (argv[i], "--decrypt") =3D=3D 0)) { > > if (params.cryptoctx =3D=3D NULL) { > > +#ifdef GMIME_26 > > + /* TODO: GMimePasswordRequestFunc */ > > + if (NULL =3D=3D (params.cryptoctx =3D g_mime_gpg_context_new(NULL, "= gpg"))) > > +#else > > GMimeSession* session =3D g_object_new(g_mime_session_get_type(), NU= LL); > > if (NULL =3D=3D (params.cryptoctx =3D 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.cry= ptoctx, FALSE); > > +#ifndef GMIME_26 > > g_object_unref (session); > > session =3D NULL; > > +#endif > > } > > if (STRNCMP_LITERAL (argv[i], "--decrypt") =3D=3D 0) > > params.decrypt =3D 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); > >=20=20 > > if (node->verify_attempted && format->part_sigstatus) > > +#ifdef GMIME_26 > > + format->part_sigstatus (node->sig_list); > > +#else > > format->part_sigstatus (node->sig_validity); > > +#endif > >=20=20 > > format->part_content (part); > >=20=20 =2D-=20 Adrian Perez - Sent from my toaster Igalia - Free Software Engineering --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iEYEARECAAYFAk8VUe4ACgkQkcVZ2+TJEjtcwgCaAjJvz03iNmXNkcc/g76IsYcB fNoAn0SWJIRc+5XxE/xkXo7Cxfno3crv =RWWF -----END PGP SIGNATURE----- --=-=-=--