Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 3, 2015, 15:01 UTC
Message-ID: <vpqwpw7fsxt.fsf@anie.imag.fr>
In-Reply-To: <CAPig+cRJG7t1M-FyrB84UG4Ar_NBW3J+wFvFeLg599sAEAweHw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 21 quoted lines

>> @@ -624,6 +633,33 @@ static void find_subpos(const char *buf, unsigned long sz,
>>         *nonsiglen = *sig - buf;
>>  }
>>
>> +/*
>> + * If 'lines' is greater than 0, append that many lines from the given
>> + * 'buf' of length 'size' to the given strbuf.
>> + */
>> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
>> +{
>> +       int i;
>> +       const char *sp, *eol;
>> +       size_t len;
>> +
>> +       if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
>> +               size += 2;
>
> Aside from the +2 which Matthieu already questioned, this code has a
> much more serious problem. strstr() assumes that 'buf' is
> NUL-terminated, however, the fact that buf's size is also being passed
> to the function, implies that it may not be NUL-terminated.

If Karthik applies my suggestion, then the strstr would go away. I think the code would be correct even on non-null-terminated strings.

Actually, we're already making the assumption that the buffer for the whole tag object is null-terminated (and contains no '\0') for %(contents):

		else if (!strcmp(name, "contents"))
			v->s = xstrdup(subpos);

(But I agree that even if the assumption is correct, it should be made explicit if it remains a precondition of append_lines).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/