{"thread":{"id":"41362","subject":"[PATCH] ref-filter.c: don't stomp on memory","startedAt":"2016-02-07T00:23:15Z","lastAt":"2016-02-07T04:50:21Z","messageCount":3,"participants":["Ramsay Jones","Eric Sunshine","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"277672","messageId":"56B68E73.2050303@ramsayjones.plus.com","threadId":"41362","inReplyTo":null,"subject":"[PATCH] ref-filter.c: don't stomp on memory","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-02-07T00:23:15Z","receivedAt":"2016-02-07T00:23:15Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones \n---\n\nHi Karthik,\n\nIf you need to re-roll your 'kn/ref-filter-atom-parsing' branch, could\nyou please squash this (or something like it) into the relevant patch\n(commit 6613d5f1, \"ref-filter: introduce parsing functions for each valid\natom\", 31-01-2016).\n\nThis evening, (by mistake!) I built the pu branch with -fsanitize=address\nin my CFLAGS. This resulted in many test failures, which were all caused\nby the memcmp() call below stomping all over memory.\n\nHmm, as I was writing this email, I had a vague recollection of another\nemail on the list recently mentioning this code. So, if this has already\nbeen reported, sorry for the noise!\n\nATB,\nRamsay Jones\n\n ref-filter.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex d48e2a3..c98065e 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -260,7 +260,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \t\t * table.\n \t\t */\n \t\targ = memchr(sp, ':', ep - sp);\n-\t\tif ((!arg || len == arg - sp) &&\n+\t\tif ((( arg && len == arg - sp) ||\n+\t\t (!arg && len == ep - sp )) &&\n \t\t !memcmp(valid_atom[i].name, sp, len))\n \t\t\tbreak;\n \t}\n-- \n2.7.0\n"},{"id":"277673","messageId":"CAPig+cT=h49xNTb=nu7+xvbOXAFoC0gyOW8R5tb-oosapN2eiA@mail.gmail.com","threadId":"41362","inReplyTo":"56B68E73.2050303@ramsayjones.plus.com","subject":"Re: [PATCH] ref-filter.c: don't stomp on memory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-07T03:16:11Z","receivedAt":"2016-02-07T03:16:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 6, 2016 at 7:23 PM, Ramsay Jones\n wrote:\n> If you need to re-roll your 'kn/ref-filter-atom-parsing' branch, could\n> you please squash this (or something like it) into the relevant patch\n> (commit 6613d5f1, \"ref-filter: introduce parsing functions for each valid\n> atom\", 31-01-2016).\n>\n> This evening, (by mistake!) I built the pu branch with -fsanitize=address\n> in my CFLAGS. This resulted in many test failures, which were all caused\n> by the memcmp() call below stomping all over memory.\n>\n> Hmm, as I was writing this email, I had a vague recollection of another\n> email on the list recently mentioning this code. So, if this has already\n> been reported, sorry for the noise!\n\nYou're probably thinking of [1]. Interestingly, the two proposed fixes\ndiffer... (more below)\n\n[1]: http://git.661346.n2.nabble.com/PATCH-v4-00-12-ref-filter-use-parsing-functions-td7646877i20.html#a7647418\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -260,7 +260,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n> * table.\n> */\n> arg = memchr(sp, ':', ep - sp);\n> - if ((!arg || len == arg - sp) &&\n> + if ((( arg && len == arg - sp) ||\n> + (!arg && len == ep - sp )) &&\n> !memcmp(valid_atom[i].name, sp, len))\n> break;\n> }\n\nYour fix is pretty easy to to read and seems correct. Karthik's fix\nrequired several re-reads, and I *think* it may be correct, however,\nit's difficult to grok due to its logic inversion.\n\nAside from the two proposed fixes, a fix patterned after the original\ncode which patch 5/12 replaced would be even easier to understand.\nThat is, something like this:\n\n arg = memchr(...);\n if (!arg)\n arg = ep;\n if (len == arg - sp && !memcmp(...))\n ...\n"},{"id":"277675","messageId":"CAOLa=ZQN1WJPQzyZE2jOQj43+_6oS-jzxshKqzdn9Rw0Bb1c_A@mail.gmail.com","threadId":"41362","inReplyTo":"CAPig+cT=h49xNTb=nu7+xvbOXAFoC0gyOW8R5tb-oosapN2eiA@mail.gmail.com","subject":"Re: [PATCH] ref-filter.c: don't stomp on memory","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-02-07T04:50:21Z","receivedAt":"2016-02-07T04:50:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Feb 7, 2016 at 8:46 AM, Eric Sunshine wrote:\n> On Sat, Feb 6, 2016 at 7:23 PM, Ramsay Jones\n> wrote:\n>> If you need to re-roll your 'kn/ref-filter-atom-parsing' branch, could\n>> you please squash this (or something like it) into the relevant patch\n>> (commit 6613d5f1, \"ref-filter: introduce parsing functions for each valid\n>> atom\", 31-01-2016).\n>>\n>> This evening, (by mistake!) I built the pu branch with -fsanitize=address\n>> in my CFLAGS. This resulted in many test failures, which were all caused\n>> by the memcmp() call below stomping all over memory.\n>>\n>> Hmm, as I was writing this email, I had a vague recollection of another\n>> email on the list recently mentioning this code. So, if this has already\n>> been reported, sorry for the noise!\n>\n\nThanks for reporting, I stumbled upon the same problem myself.\n\n> You're probably thinking of [1]. Interestingly, the two proposed fixes\n> differ... (more below)\n>\n> [1]: http://git.661346.n2.nabble.com/PATCH-v4-00-12-ref-filter-use-parsing-functions-td7646877i20.html#a7647418\n>\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -260,7 +260,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>> * table.\n>> */\n>> arg = memchr(sp, ':', ep - sp);\n>> - if ((!arg || len == arg - sp) &&\n>> + if ((( arg && len == arg - sp) ||\n>> + (!arg && len == ep - sp )) &&\n>> !memcmp(valid_atom[i].name, sp, len))\n>> break;\n>> }\n>\n> Your fix is pretty easy to to read and seems correct. Karthik's fix\n> required several re-reads, and I *think* it may be correct, however,\n> it's difficult to grok due to its logic inversion.\n>\n\nTrue, its not so easy to comprehend at first.\n\n> Aside from the two proposed fixes, a fix patterned after the original\n> code which patch 5/12 replaced would be even easier to understand.\n> That is, something like this:\n>\n> arg = memchr(...);\n> if (!arg)\n> arg = ep;\n> if (len == arg - sp && !memcmp(...))\n> ...\n\nThis seems good, will change, Thanks to both of you\n\n-- \nRegards,\nKarthik Nayak\n"}]}