{"thread":{"id":"34915","subject":"[PATCH] urlmatch: append_normalized_escapes can reallocate norm.buf","startedAt":"2013-09-12T09:57:31Z","lastAt":"2013-09-12T22:26:10Z","messageCount":8,"participants":["Thomas Rast","Kyle J. McKay","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"227520","messageId":"75d702a744eb33a456622dd2ff901abef83e51d8.1378979451.git.trast@inf.ethz.ch","threadId":"34915","inReplyTo":null,"subject":"[PATCH] urlmatch: append_normalized_escapes can reallocate norm.buf","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-09-12T09:57:31Z","receivedAt":"2013-09-12T09:57:31Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The calls to strbuf_add* within append_normalized_escapes() can\nreallocate the buffer passed to it.  Therefore, the seg_start pointer\ninto the string cannot be kept across such calls.\n\nThe actual bug is from 3402a8d (config: add helper to normalize and\nmatch URLs, 2013-07-31).  It can first be detected by valgrind after\n6a56993 (config: parse http.<url>.<variable> using urlmatch,\n2013-08-05) introduced tests covering url_normalize().\n\nSigned-off-by: Thomas Rast <trast@inf.ethz.ch>\n---\n\nMy apologies if this is redundant; I didn't have time to watch the\nlist over the last two weeks.  However it seems today's pu is still\nbroken.\n\nThe valgrind error looks like this:\n\n  ==4607== Invalid read of size 1\n  ==4607==    at 0x4C2D3A1: __GI_strcmp (mc_replace_strmem.c:731)\n  ==4607==    by 0x404C68: url_normalize (urlmatch.c:300)\n  ==4607==    by 0x403F33: main (test-urlmatch-normalization.c:34)\n  ==4607==  Address 0x5be9046 is 6 bytes inside a block of size 24 free'd\n  ==4607==    at 0x4C2BFC6: realloc (vg_replace_malloc.c:687)\n  ==4607==    by 0x405F6B: xrealloc (wrapper.c:100)\n  ==4607==    by 0x40794E: strbuf_grow (strbuf.c:74)\n  ==4607==    by 0x40854D: strbuf_vaddf (strbuf.c:268)\n  ==4607==    by 0x40817E: strbuf_addf (strbuf.c:203)\n  ==4607==    by 0x404300: append_normalized_escapes (urlmatch.c:58)\n  ==4607==    by 0x404C0A: url_normalize (urlmatch.c:291)\n  ==4607==    by 0x403F33: main (test-urlmatch-normalization.c:34)\n\nIt went undetected for a while because it does not fail the test: the\ncalls to test-urlmatch-normalization happen inside a $() substitution.\n\nI checked the other call sites to append_normalized_escapes() for the\nsame type of problem, and they seem to be okay.\n\n urlmatch.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/urlmatch.c b/urlmatch.c\nindex 1db76c8..59abc80 100644\n--- a/urlmatch.c\n+++ b/urlmatch.c\n@@ -281,7 +281,8 @@ char *url_normalize(const char *url, struct url_info *out_info)\n \t\turl_len--;\n \t}\n \tfor (;;) {\n-\t\tconst char *seg_start = norm.buf + norm.len;\n+\t\tconst char *seg_start;\n+\t\tsize_t prev_len = norm.len;\n \t\tconst char *next_slash = url + strcspn(url, \"/?#\");\n \t\tint skip_add_slash = 0;\n \t\t/*\n@@ -297,6 +298,7 @@ char *url_normalize(const char *url, struct url_info *out_info)\n \t\t\tstrbuf_release(&norm);\n \t\t\treturn NULL;\n \t\t}\n+\t\tseg_start = norm.buf + prev_len;\n \t\tif (!strcmp(seg_start, \".\")) {\n \t\t\t/* ignore a . segment; be careful not to remove initial '/' */\n \t\t\tif (seg_start == path_start + 1) {\n-- \n1.8.4.609.g4395a4f\n"},{"id":"227533","messageId":"c8915eaaf877abe0e69864ffdc6c50f@f74d39fa044aa309eaea14b9f57fe79","threadId":"34915","inReplyTo":"75d702a744eb33a456622dd2ff901abef83e51d8.1378979451.git.trast@inf.ethz.ch","subject":"[PATCH v2] urlmatch.c: recompute ptr after append_normalized_escapes","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-09-12T14:15:40Z","receivedAt":"2013-09-12T14:15:40Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Sep 12, 2013, at 02:57, Thomas Rast wrote:\n\n> The calls to strbuf_add* within append_normalized_escapes() can\n> reallocate the buffer passed to it.  Therefore, the seg_start pointer\n> into the string cannot be kept across such calls.\n\nThanks for finding this.\n\n> It went undetected for a while because it does not fail the test: the\n> calls to test-urlmatch-normalization happen inside a $() substitution.\n> \n> I checked the other call sites to append_normalized_escapes() for the\n> same type of problem, and they seem to be okay.\n\n> diff --git a/urlmatch.c b/urlmatch.c\n> index 1db76c8..59abc80 100644\n> --- a/urlmatch.c\n> +++ b/urlmatch.c\n> @@ -281,7 +281,8 @@ char *url_normalize(const char *url, struct url_info *out_info)\n> \t\turl_len--;\n> \t}\n> \tfor (;;) {\n> -\t\tconst char *seg_start = norm.buf + norm.len;\n> +\t\tconst char *seg_start;\n> +\t\tsize_t prev_len = norm.len;\n\nHow about a more descriptive name for what prev_len is?  It's actually the\nsegment start offset.\n\n> \t\tconst char *next_slash = url + strcspn(url, \"/?#\");\n> \t\tint skip_add_slash = 0;\n> \t\t/*\n> @@ -297,6 +298,7 @@ char *url_normalize(const char *url, struct url_info *out_info)\n> \t\t\tstrbuf_release(&norm);\n> \t\t\treturn NULL;\n> \t\t}\n> +\t\tseg_start = norm.buf + prev_len;\n\nA comment would be nice here to remind folks who might be tempted to\nrevert this to the previous version why it's being done this way.\n\nI'm sure at some point someone will propose a \"simplification\" patch\notherwise.\n\nAlso some nits.  The patch description should be imperative mood\n(cf. Documentation/SubmittingPatches).  And instead of mentioning the seg_start\npointer in the description (which will be meaningless to just about everyone and\nit's clear from the diff), mention the bad thing the code was doing in more\ngeneral terms that will be clear to anyone familiar with a strbuf.\n\nSo how about this patch instead...\n\n-- 8< --\nFrom: Thomas Rast <trast@inf.ethz.ch>\nSubject: urlmatch.c: recompute pointer after append_normalized_escapes\n\nWhen append_normalized_escapes is called, its internal strbuf_add* calls can\ncause the strbuf's buf to be reallocated changing the value of the buf pointer.\n\nDo not use the strbuf buf pointer from before any append_normalized_escapes\ncalls afterwards.  Instead recompute the needed pointer.\n\nSigned-off-by: Thomas Rast <trast@inf.ethz.ch>\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\n---\n urlmatch.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/urlmatch.c b/urlmatch.c\nindex 1db76c89..01c67467 100644\n--- a/urlmatch.c\n+++ b/urlmatch.c\n@@ -281,8 +281,9 @@ char *url_normalize(const char *url, struct url_info *out_info)\n \t\turl_len--;\n \t}\n \tfor (;;) {\n-\t\tconst char *seg_start = norm.buf + norm.len;\n+\t\tconst char *seg_start;\n+\t\tsize_t seg_start_off = norm.len;\n \t\tconst char *next_slash = url + strcspn(url, \"/?#\");\n \t\tint skip_add_slash = 0;\n \t\t/*\n \t\t * RFC 3689 indicates that any . or .. segments should be\n@@ -297,6 +298,8 @@ char *url_normalize(const char *url, struct url_info *out_info)\n \t\t\tstrbuf_release(&norm);\n \t\t\treturn NULL;\n \t\t}\n+\t\t/* append_normalized_escapes can cause norm.buf to change */\n+\t\tseg_start = norm.buf + seg_start_off;\n \t\tif (!strcmp(seg_start, \".\")) {\n \t\t\t/* ignore a . segment; be careful not to remove initial '/' */\n \t\t\tif (seg_start == path_start + 1) {\n-- \n1.8.3\n"},{"id":"227535","messageId":"87a9jidpv5.fsf@linux-k42r.v.cablecom.net","threadId":"34915","inReplyTo":"c8915eaaf877abe0e69864ffdc6c50f@f74d39fa044aa309eaea14b9f57fe79","subject":"Re: [PATCH v2] urlmatch.c: recompute ptr after append_normalized_escapes","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-09-12T15:25:02Z","receivedAt":"2013-09-12T15:25:02Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"\"Kyle J. McKay\" <mackyle@gmail.com> writes:\n\n> Also some nits.  The patch description should be imperative mood\n> (cf. Documentation/SubmittingPatches).\n\nHeh.  Serves me right to go away for a while and get SubmittingPatches\ncited at me on return ;-)\n\nThanks for the updated patch.  I agree with the changes.  I particularly\nlike the better variable name.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"227548","messageId":"xmqqob7xq4q0.fsf@gitster.dls.corp.google.com","threadId":"34915","inReplyTo":"87a9jidpv5.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH v2] urlmatch.c: recompute ptr after append_normalized_escapes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-12T18:23:19Z","receivedAt":"2013-09-12T18:23:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, both.\n"},{"id":"227549","messageId":"xmqqioy5q4dd.fsf@gitster.dls.corp.google.com","threadId":"34915","inReplyTo":"c8915eaaf877abe0e69864ffdc6c50f@f74d39fa044aa309eaea14b9f57fe79","subject":"Re: [PATCH v2] urlmatch.c: recompute ptr after append_normalized_escapes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-12T18:30:54Z","receivedAt":"2013-09-12T18:30:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" <mackyle@gmail.com> writes:\n\n> So how about this patch instead...\n>\n> -- 8< --\n> From: Thomas Rast <trast@inf.ethz.ch>\n> Subject: urlmatch.c: recompute pointer after append_normalized_escapes\n>\n> When append_normalized_escapes is called, its internal strbuf_add* calls can\n> cause the strbuf's buf to be reallocated changing the value of the buf pointer.\n>\n> Do not use the strbuf buf pointer from before any append_normalized_escapes\n> calls afterwards.  Instead recompute the needed pointer.\n>\n> Signed-off-by: Thomas Rast <trast@inf.ethz.ch>\n> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>\n> ---\n>  urlmatch.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/urlmatch.c b/urlmatch.c\n> index 1db76c89..01c67467 100644\n> --- a/urlmatch.c\n> +++ b/urlmatch.c\n> @@ -281,8 +281,9 @@ char *url_normalize(const char *url, struct url_info *out_info)\n>  \t\turl_len--;\n>  \t}\n>  \tfor (;;) {\n> -\t\tconst char *seg_start = norm.buf + norm.len;\n> +\t\tconst char *seg_start;\n> +\t\tsize_t seg_start_off = norm.len;\n>  \t\tconst char *next_slash = url + strcspn(url, \"/?#\");\n>  \t\tint skip_add_slash = 0;\n>  \t\t/*\n>  \t\t * RFC 3689 indicates that any . or .. segments should be\n> @@ -297,6 +298,8 @@ char *url_normalize(const char *url, struct url_info *out_info)\n>  \t\t\tstrbuf_release(&norm);\n>  \t\t\treturn NULL;\n>  \t\t}\n> +\t\t/* append_normalized_escapes can cause norm.buf to change */\n> +\t\tseg_start = norm.buf + seg_start_off;\n\nThe change looks good, but I find that this comment is not placed in\nthe right place.  It is good if the reader knows about an old bug to\nput it here, but if the first thing a reader reads is this updated\nversion, the comment is better placed close to the place where the\nstart_ofs variable captures the original value (i.e. \"because the\nnext call may relocate the buffer, we cannot grab seg_start upfront;\ninstead we need to record the start_ofs here, and that is what this\nvariable is about\").\n\nIt is too minor a point for a reroll, so I'll try to tweak it\nlocally.  Something like this (but now I think about it, the comment\nmay not even be necessary).\n\n urlmatch.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/urlmatch.c b/urlmatch.c\nindex 01c6746..d1600e2 100644\n--- a/urlmatch.c\n+++ b/urlmatch.c\n@@ -282,9 +282,17 @@ char *url_normalize(const char *url, struct url_info *out_info)\n \t}\n \tfor (;;) {\n \t\tconst char *seg_start;\n-\t\tsize_t seg_start_off = norm.len;\n+\t\tsize_t seg_start_off;\n \t\tconst char *next_slash = url + strcspn(url, \"/?#\");\n \t\tint skip_add_slash = 0;\n+\n+\t\t/*\n+\t\t * record the starting offset; appending escapes may\n+\t\t * relocate the buffer, so we cannot capture seg_start\n+\t\t * upfront and use it later.\n+\t\t */\n+\t\tseg_start_off = norm.len;\n+\n \t\t/*\n \t\t * RFC 3689 indicates that any . or .. segments should be\n \t\t * unescaped before being checked for.\n@@ -298,7 +306,7 @@ char *url_normalize(const char *url, struct url_info *out_info)\n \t\t\tstrbuf_release(&norm);\n \t\t\treturn NULL;\n \t\t}\n-\t\t/* append_normalized_escapes can cause norm.buf to change */\n+\n \t\tseg_start = norm.buf + seg_start_off;\n \t\tif (!strcmp(seg_start, \".\")) {\n \t\t\t/* ignore a . segment; be careful not to remove initial '/' */\n"},{"id":"227576","messageId":"85CEBFAA-8905-46EC-88C5-5DDE629B73F9@gmail.com","threadId":"34915","inReplyTo":"xmqqioy5q4dd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] urlmatch.c: recompute ptr after append_normalized_escapes","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-09-12T20:38:37Z","receivedAt":"2013-09-12T20:38:37Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Sep 12, 2013, at 11:30, Junio C Hamano wrote:\n\n>> +\t\t/* append_normalized_escapes can cause norm.buf to change */\n>> +\t\tseg_start = norm.buf + seg_start_off;\n>\n> The change looks good, but I find that this comment is not placed in\n> the right place.  It is good if the reader knows about an old bug to\n> put it here, but if the first thing a reader reads is this updated\n> version, the comment is better placed close to the place where the\n> start_ofs variable captures the original value (i.e. \"because the\n> next call may relocate the buffer, we cannot grab seg_start upfront;\n> instead we need to record the start_ofs here, and that is what this\n> variable is about\").\n>\n> It is too minor a point for a reroll, so I'll try to tweak it\n> locally.  Something like this (but now I think about it, the comment\n> may not even be necessary).\n\nThe longer comment looks good to me.  If you think the code will be  \nsafe from\nsimplification patches without a comment, that works for me too.  I've  \njust seen\nso many \"simplification\" patches go by on the list I'm concerned it  \nwill be a\ntarget otherwise leading to re-introduction of the problem.\n\n> diff --git a/urlmatch.c b/urlmatch.c\n> index 01c6746..d1600e2 100644\n> --- a/urlmatch.c\n> +++ b/urlmatch.c\n> @@ -282,9 +282,17 @@ char *url_normalize(const char *url, struct  \n> url_info *out_info)\n> \t}\n> \tfor (;;) {\n> \t\tconst char *seg_start;\n> -\t\tsize_t seg_start_off = norm.len;\n> +\t\tsize_t seg_start_off;\n> \t\tconst char *next_slash = url + strcspn(url, \"/?#\");\n> \t\tint skip_add_slash = 0;\n> +\n> +\t\t/*\n> +\t\t * record the starting offset; appending escapes may\n> +\t\t * relocate the buffer, so we cannot capture seg_start\n> +\t\t * upfront and use it later.\n> +\t\t */\n> +\t\tseg_start_off = norm.len;\n> +\n> \t\t/*\n> \t\t * RFC 3689 indicates that any . or .. segments should be\n> \t\t * unescaped before being checked for.\n> @@ -298,7 +306,7 @@ char *url_normalize(const char *url, struct  \n> url_info *out_info)\n> \t\t\tstrbuf_release(&norm);\n> \t\t\treturn NULL;\n> \t\t}\n> -\t\t/* append_normalized_escapes can cause norm.buf to change */\n> +\n> \t\tseg_start = norm.buf + seg_start_off;\n> \t\tif (!strcmp(seg_start, \".\")) {\n> \t\t\t/* ignore a . segment; be careful not to remove initial '/' */\n"},{"id":"227584","messageId":"20130912220505.GM4326@google.com","threadId":"34915","inReplyTo":"85CEBFAA-8905-46EC-88C5-5DDE629B73F9@gmail.com","subject":"Re: [PATCH v2] urlmatch.c: recompute ptr after append_normalized_escapes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-12T22:05:05Z","receivedAt":"2013-09-12T22:05:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Kyle J. McKay wrote:\n\n> The longer comment looks good to me.  If you think the code will be safe from\n> simplification patches without a comment, that works for me too.\n\nI think if we can't trust reviewers to catch this kind of thing, we're\nin trouble (i.e., moving too fast). :)\n\nSo FWIW my instinct is to leave the comment out, since I actually find\nit more readable that way (otherwise I would wonder, \"Why am I being\ntold that a strbuf's buffer has a nonconstant address?  Do some other\nstrbufs have a constant address or something?\")\n\nThanks,\nJonathan\n"},{"id":"227588","messageId":"xmqq61u5oewt.fsf@gitster.dls.corp.google.com","threadId":"34915","inReplyTo":"20130912220505.GM4326@google.com","subject":"Re: [PATCH v2] urlmatch.c: recompute ptr after append_normalized_escapes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-12T22:26:10Z","receivedAt":"2013-09-12T22:26:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Kyle J. McKay wrote:\n>\n>> The longer comment looks good to me.  If you think the code will be safe from\n>> simplification patches without a comment, that works for me too.\n>\n> I think if we can't trust reviewers to catch this kind of thing, we're\n> in trouble (i.e., moving too fast). :)\n>\n> So FWIW my instinct is to leave the comment out, since I actually find\n> it more readable that way (otherwise I would wonder, \"Why am I being\n> told that a strbuf's buffer has a nonconstant address?  Do some other\n> strbufs have a constant address or something?\")\n\nYeah, I was staring at that message and coming to the same\nconclusion.\n"}]}