Re: [PATCH] cli/crypto: fix segfault on failed gmime2 crypto context creation
Jani Nikula 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
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] cli/crypto: fix segfault on failed gmime2 crypto context creation
Jani Nikula 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-da...@tethera.net [2] the notion of reporting errors on stderr for the CLI makes much less sense for the structured output case ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
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
signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
[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
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
