On Sat, Feb 6, 2016 at 7:23 PM, Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx> wrote: > If you need to re-roll your 'kn/ref-filter-atom-parsing' branch, could > you please squash this (or something like it) into the relevant patch > (commit 6613d5f1, "ref-filter: introduce parsing functions for each valid > atom", 31-01-2016). > > This evening, (by mistake!) I built the pu branch with -fsanitize=address > in my CFLAGS. This resulted in many test failures, which were all caused > by the memcmp() call below stomping all over memory. > > Hmm, as I was writing this email, I had a vague recollection of another > email on the list recently mentioning this code. So, if this has already > been reported, sorry for the noise! You're probably thinking of [1]. Interestingly, the two proposed fixes differ... (more below) [1]: http://git.661346.n2.nabble.com/PATCH-v4-00-12-ref-filter-use-parsing-functions-td7646877i20.html#a7647418 > diff --git a/ref-filter.c b/ref-filter.c > @@ -260,7 +260,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep) > * table. > */ > arg = memchr(sp, ':', ep - sp); > - if ((!arg || len == arg - sp) && > + if ((( arg && len == arg - sp) || > + (!arg && len == ep - sp )) && > !memcmp(valid_atom[i].name, sp, len)) > break; > } Your fix is pretty easy to to read and seems correct. Karthik's fix required several re-reads, and I *think* it may be correct, however, it's difficult to grok due to its logic inversion. Aside from the two proposed fixes, a fix patterned after the original code which patch 5/12 replaced would be even easier to understand. That is, something like this: arg = memchr(...); if (!arg) arg = ep; if (len == arg - sp && !memcmp(...)) ... -- 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