# [PATCH] ref-filter.c: don't stomp on memory 3 messages from 2016-02-07 to 2016-02-07. Participants: Ramsay Jones, Eric Sunshine, Karthik Nayak. Thread: https://gitlist.dev/t/41362 ## Ramsay Jones, 2016-02-07 00:23 Subject: [PATCH] ref-filter.c: don't stomp on memory Message-ID: <56B68E73.2050303@ramsayjones.plus.com> URL: https://gitlist.dev/e/56B68E73.2050303%40ramsayjones.plus.com ``` Signed-off-by: Ramsay Jones --- Hi Karthik, 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! ATB, Ramsay Jones ref-filter.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ref-filter.c b/ref-filter.c index d48e2a3..c98065e 100644 --- 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; } -- 2.7.0 ``` ## Eric Sunshine, 2016-02-07 03:16 Subject: Re: [PATCH] ref-filter.c: don't stomp on memory Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT%3Dh49xNTb%3Dnu7%2BxvbOXAFoC0gyOW8R5tb-oosapN2eiA%40mail.gmail.com In-Reply-To: <56B68E73.2050303@ramsayjones.plus.com> ``` On Sat, Feb 6, 2016 at 7:23 PM, Ramsay Jones 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(...)) ... ``` ## Karthik Nayak, 2016-02-07 04:50 Subject: Re: [PATCH] ref-filter.c: don't stomp on memory Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZQN1WJPQzyZE2jOQj43%2B_6oS-jzxshKqzdn9Rw0Bb1c_A%40mail.gmail.com In-Reply-To: ``` On Sun, Feb 7, 2016 at 8:46 AM, Eric Sunshine wrote: > On Sat, Feb 6, 2016 at 7:23 PM, Ramsay Jones > 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! > Thanks for reporting, I stumbled upon the same problem myself. > 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. > True, its not so easy to comprehend at first. > 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(...)) > ... This seems good, will change, Thanks to both of you -- Regards, Karthik Nayak ```