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

classic Classic list List threaded Threaded
4 messages Options
Jani Nikula Jani Nikula
Reply | Threaded
Open this post in threaded view
|

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

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

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

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

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

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (847 bytes) Download Attachment
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Jani Nikula
Jani Nikula <[hidden email]> 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:[hidden email]

[2] the notion of reporting errors on stderr for the CLI makes much less
    sense for the structured output case
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Jani Nikula
Jani Nikula <[hidden email]> 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
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch