Thanks for the review, David.

On Fri 2019-09-13 22:58:27 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> +/* see
>> + * https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling-00#section-4.1.1 */
>> +static bool
>> +_notmuch_is_mixed_up_mangled (GMimeObject *part)
>> +{
>> +    GMimeMultipart *mpart = NULL;
>> +    GMimeObject *first, *second, *third = NULL;
>> +    char *prelude_string = NULL;
>> +    bool prelude_is_empty;
>> +
>> +    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (part),
>> +				       "multipart", "mixed"))
>> +	return false;
>
> Can g_mime_object_get_content_type plausibly fail (and return NULL) here?

hm, yes, it can -- if it's passed NULL, for example.  It's not an
external function, but I can program this more defensively.  i'll do
that in the next revision.

>> +    if (! GMIME_IS_MULTIPART (part))
>> +	return false;
>
> I guess this happens if the mime structure does not match the content
> type declaration? Not sure if this needs a comment or if it's clear
> enough.

yeah, this is unlikely, maybe even impossible given the current gmime
implementation, but it's a defensive step.

>> +    mpart = GMIME_MULTIPART (part);
>> +    if (mpart == NULL)
>> +	return false;
>> +    if (g_mime_multipart_get_count (mpart) != 3)
>> +	return false;
>> +    first = g_mime_multipart_get_part (mpart, 0);
>
> there's a slight cognitive dissonance for me between the zero and one
> based indexing schemes here. part0, part1, and part2? or maybe an
> GMimeObject *part[3]

sure, i'll make it *parts[3] to avoid the dissonance. ("part" is already
taken -- it's the argument for the function.

>> +    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (first),
>> +				       "text", "plain"))
>> +	return false;
>> +    if (! GMIME_IS_TEXT_PART (first))
>> +	return false;
>> +    second = g_mime_multipart_get_part (mpart, 1);
>> +    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (second),
>> +				       "application", "pgp-encrypted"))
>> +	return false;
>> +    third = g_mime_multipart_get_part (mpart, 2);
>> +    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (third),
>> +				       "application", "octet-stream"))
>> +	return false;
>> +
>> +    /* Is first subpart length 0? */
>> +    prelude_string = g_mime_text_part_get_text (GMIME_TEXT_PART (first));
>> +    prelude_is_empty = ! (strcmp ("", prelude_string));
>> +    g_free (prelude_string);
>
> It might make sense to use the EMPTY_STRING macro here, although
> currently it's only accesible via notmuch-private.h

hm, notmuch-private.h is in lib/, which seems inappropriate for use
outside the library.  however, i agree that this seems like a
superfluous call to strcmp -- i'll change it for a simple test.

I've updated this 2/4 patch on my mixed-up-mangling branch (visible at
https://gitlab.com/dkg/notmuch), and i'll send it to this thread
shortly.

            --dkg