[PATCH] cli/crypto: fix segfault on failed gmime2 crypto context creation

[PATCH] cli/crypto: fix segfault on failed gmime2 crypto context creation

[Jani Nikula]

Commit 1fdc08d0ffab ("cli/crypto: treat failure to create a crypto
context as fatal.") started treating crypto context creation failures
"as fatal", returning NULL from _mime_node_create().

Unfortunately, we do not have NULL checks for _mime_node_create()
failures. The only caller, mime_node_child(), could check and return
NULL (as it's documented to do on errors) but none of the several call
sites have NULL checks either. And none of them really have a trivial
but feasible and graceful way of recovery.

So while the right thing to do would be to handle NULL returns
properly all over the place, and we have other scenarios that do
return NULL from above mentioned functions, the crypto context
creation failure is something that does seem to show up regularly in
some scenarios, revert back to the functionality before commit
1fdc08d0ffab as an interim fix.
---
 mime-node.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mime-node.c b/mime-node.c
index d48be4c46695..27de72fa2f84 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -271,7 +271,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 	const char *protocol = g_mime_content_type_get_parameter (content_type, "protocol");
 	cryptoctx = _notmuch_crypto_get_gmime_context (node->ctx->crypto, protocol);
 	if (!cryptoctx)
-	    return NULL;
+	    return node;
     }
 #endif
 
-- 
2.11.0

Re: [PATCH] cli/crypto: fix segfault on failed gmime2 crypto context creation

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

On Mon 2017-10-16 18:40:44 +0300, Jani Nikula wrote:
> Commit 1fdc08d0ffab ("cli/crypto: treat failure to create a crypto
> context as fatal.") started treating crypto context creation failures
> "as fatal", returning NULL from _mime_node_create().
>
> Unfortunately, we do not have NULL checks for _mime_node_create()
> failures. The only caller, mime_node_child(), could check and return
> NULL (as it's documented to do on errors) but none of the several call
> sites have NULL checks either. And none of them really have a trivial
> but feasible and graceful way of recovery.
>
> So while the right thing to do would be to handle NULL returns
> properly all over the place, and we have other scenarios that do
> return NULL from above mentioned functions, the crypto context
> creation failure is something that does seem to show up regularly in
> some scenarios, revert back to the functionality before commit
> 1fdc08d0ffab as an interim fix.

This suggestion seems reasonable to me.  Transitioning to GMime 3.0 will
also make this interim fix obsolete.

     --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] cli/crypto: fix segfault on failed gmime2 crypto context creation

[David Bremner]

Jani Nikula <jani@nikula.org> writes:
>
> So while the right thing to do would be to handle NULL returns
> properly all over the place, and we have other scenarios that do
> return NULL from above mentioned functions, the crypto context
> creation failure is something that does seem to show up regularly in
> some scenarios, revert back to the functionality before commit
> 1fdc08d0ffab as an interim fix.

I'm not sure how interim this is. Are we ready to deprecate gmime-2.6 in
the next release?

As you mention, even if this particular NULL return goes away, the code
is still somehow broken in terms of checking for NULL returns from
_mime_node_create (and propagating / handling them).  I previously wrote
a patch that did that work [1], but this turns out to be not so helpful,
since we arrive at the formatted output with a NULL pointer.

It occurred to me that we could add an error code and/or string to the
mime-node structure, and then never return NULL from _mime_node_create
except for OOM. Then we could actually report useful errors in the
structured output [2].  If that seems like a reasonable plan, then I
guess I could live with this interim fix to get master working for
people again.

Is this an important important enough issue that we should think about a
point release?

[1] id:20170901001014.7610-1-david@tethera.net

[2] the notion of reporting errors on stderr for the CLI makes much less
    sense for the structured output case

Re: [PATCH] cli/crypto: fix segfault on failed gmime2 crypto context creation

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> Commit 1fdc08d0ffab ("cli/crypto: treat failure to create a crypto
> context as fatal.") started treating crypto context creation failures
> "as fatal", returning NULL from _mime_node_create().
>
> Unfortunately, we do not have NULL checks for _mime_node_create()
> failures. The only caller, mime_node_child(), could check and return
> NULL (as it's documented to do on errors) but none of the several call
> sites have NULL checks either. And none of them really have a trivial
> but feasible and graceful way of recovery.
>
> So while the right thing to do would be to handle NULL returns
> properly all over the place, and we have other scenarios that do
> return NULL from above mentioned functions, the crypto context
> creation failure is something that does seem to show up regularly in
> some scenarios, revert back to the functionality before commit
> 1fdc08d0ffab as an interim fix.
> ---
>  mime-node.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mime-node.c b/mime-node.c
> index d48be4c46695..27de72fa2f84 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -271,7 +271,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>  	const char *protocol = g_mime_content_type_get_parameter (content_type, "protocol");
>  	cryptoctx = _notmuch_crypto_get_gmime_context (node->ctx->crypto, protocol);
>  	if (!cryptoctx)
> -	    return NULL;
> +	    return node;
>      }
>  #endif

Pushed.

d