Junio C Hamano <gitster@xxxxxxxxx> writes:
> One worry that I have is if the strings embedded in this function to
> the final format are safe. As far as I can tell, the pieces of
> strings that are literally inserted into the resulting format string
> by this function are maxwidth, remote_prefix, and return values from
> branch_get_color() calls.
>
> The maxwidth is inserted via "%d" and made into decimal constant,
> and there is no risk for it being in the resulting format. Are
> the return values of branch_get_color() calls safe? I do not think
> they can have '%' in them, but if they do, they need to be quoted.
> The same worry exists for remote_prefix. Currently it can either be
> an empty string or "remotes/", and is safe to be embedded in a
> format string.
In case it was not clear, in short, I do not think there is anything
broken in the code, but it is a longer-term improvement to introduce
a helper that takes a string and returns a version of the string
that is safely quoted to be used in the for-each-ref format string
use it like so:
strbuf_addf(&remote,
"%s"
"%%(align:%d,left)%s%%(refname:strip=2)%%(end)"
...
"%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
quote_literal_for_format(branch_get_color(BRANCH_COLOR_REMOTE)),
...);
and the implementation of the helper may look like:
const char *quote_literal_for_format(const char *s)
{
static strbuf buf = STRBUF_INIT;
strbuf_reset(&buf);
while (*s) {
const char *ep = strchrnul(s, '%');
if (s < ep)
strbuf_add(&buf, s, ep - s);
if (*ep == '%') {
strbuf_addstr(&buf, "%%");
s = ep + 1;
} else {
s = ep;
}
}
return buf.buf;
}