Re: [PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display

[Daniel Kahn Gillmor <dkg@fifthhorseman.net>]

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

Hi Bremner--

thanks for the review!

On Sat 2019-08-03 12:15:30 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> +	ret = true;
>> +	for (int i = 0; i < g_mime_header_list_get_count (legacy_display_headers); i++) {
>> +	    GMimeHeader *dh = g_mime_header_list_get_header_at (legacy_display_headers, i);
>> +	    if (dh == NULL) {
>> +		ret = false;
>> +		break;
>> +	    }
>
> I can live with the use of break if you think it's superior, but I think
> the idiom of "goto DONE" is more common in the notmuch codebase. I
> personally always have think about the semantics of "break" and
> "continue" in C pretty carefully.

i thought i was the only one who got confused between "break" and
"continue"!  I will convert to goto DONE, i agree it's more readable.

>> +	    if (strcmp (g_mime_header_get_value (dh), g_mime_header_get_value (ph))) {
>> +		ret = false;
>> +		break;
>> +	    }
>
> It's not really clear to me what kind of "invalid" causes
> g_mime_header_get_value to return NULL. Maybe this strcmp should be
> guarded against that?

i think it's impossible in the current implementation for this to go
wrong, since we've already got a GMimeHeader object from an existing
block of headers, but i'll add some protection just in case GMime
changes its implementation or some fuzzer constructs a truly devious
not-quite-RFC-822 input.

         --dkg

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