On Thu, Sep 3, 2015 at 8:09 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> In 'tag.c' we can print N lines from the annotation of the tag using
>> the '-n<num>' option. Copy code from 'tag.c' to 'ref-filter' and
>> modify it to support appending of N lines from the annotation of tags
>> to the given strbuf.
>>
>> Implement %(contents:lines=X) where X lines of the given object are
>> obtained.
>>
>> Add documentation and test for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
>> ---
>> struct atom_value {
>> const char *s;
>> - struct align align;
>> + union {
>> + struct align align;
>> + struct contents contents;
>> + } u;
>> void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
>> unsigned long ul; /* used for sorting when not FIELD_STR */
>> };
>> @@ -226,7 +235,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
>> push_stack_element(&state->stack);
>> new = state->stack;
>> new->at_end = align_handler;
>> - new->cb_data = &atomv->align;
>> + new->cb_data = &atomv->u.align;
>
> You could declare atom_value with the union from the start, even if it
> has only a single member ('align', at first). Doing so would make this
> patch less noisy and wouldn't distract the reader with these seemingly
> unrelated changes.
>
Will do.
>> }
>>
>> static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> @@ -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.
> Consequently, strstr() could zip well past the end of 'buf', trying to
> consult memory not part of 'buf', which is a violation of the C
> standard. Even with the band-aid (sp <= buf + size) which tries to
> detect this situation, it's still a violation, and could crash if
> strstr() attempts to consult memory which hasn't even been allocated
> to the process or is otherwise protected in some fashion.
>
> It's possible that 'buf' may actually always be NUL-terminated, but
> this code does not convey that fact. If it is known to be
> NUL-terminated, then it is safe to use strstr(), however, that should
> be shown more clearly either by revising the code or adding an
> appropriate comment.
>
Although it is NUL terminated, you do have a point, I hope you checked out
Matthieu's suggestion of removing that snippet of code and calculating size
in the function call itself.
>> + sp = buf;
>> +
>> + for (i = 0; i < lines && sp < buf + size; i++) {
>> + if (i)
>> + strbuf_addstr(out, "\n ");
>> + eol = memchr(sp, '\n', size - (sp - buf));
>> + len = eol ? eol - sp : size - (sp - buf);
>> + strbuf_add(out, sp, len);
>> + if (!eol)
>> + break;
>> + sp = eol + 1;
>> + }
>> +}
>> @@ -663,6 +701,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>> v->s = xmemdupz(sigpos, siglen);
>> else if (!strcmp(name, "contents"))
>> v->s = xstrdup(subpos);
>> + else if (skip_prefix(name, "contents:lines=", &valp)) {
>> + struct strbuf s = STRBUF_INIT;
>> + if (strtoul_ui(valp, 10, &v->u.contents.lines))
>> + die(_("positive width expected contents:lines=%s"), valp);
>
> This error message is still too tightly coupled to %(align:), from
> which it was copied. "width"?
>
Will change it, thanks.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html