[PATCH] ref-filter.c: don't stomp on memory

[PATCH] ref-filter.c: don't stomp on memory

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

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

Re: [PATCH] ref-filter.c: don't stomp on memory

[Eric Sunshine]

On Sat, Feb 6, 2016 at 7:23 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> 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(...))
        ...

Re: [PATCH] ref-filter.c: don't stomp on memory

[Karthik Nayak]

On Sun, Feb 7, 2016 at 8:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Feb 6, 2016 at 7:23 PM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> 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