[PATCH] urlmatch: append_normalized_escapes can reallocate norm.buf

From: Thomas Rast <trast@inf.ethz.ch>
Date: Sep 12, 2013, 09:57 UTC
Message-ID: <75d702a744eb33a456622dd2ff901abef83e51d8.1378979451.git.trast@inf.ethz.ch>

The calls to strbuf_add* within append_normalized_escapes() can reallocate the buffer passed to it. Therefore, the seg_start pointer into the string cannot be kept across such calls.

The actual bug is from 3402a8d (config: add helper to normalize and match URLs, 2013-07-31). It can first be detected by valgrind after 6a56993 (config: parse http.<url>.<variable> using urlmatch, 2013-08-05) introduced tests covering url_normalize().

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---

My apologies if this is redundant; I didn't have time to watch the list over the last two weeks. However it seems today's pu is still broken.

The valgrind error looks like this:

  ==4607== Invalid read of size 1
  ==4607==    at 0x4C2D3A1: __GI_strcmp (mc_replace_strmem.c:731)
  ==4607==    by 0x404C68: url_normalize (urlmatch.c:300)
  ==4607==    by 0x403F33: main (test-urlmatch-normalization.c:34)
  ==4607==  Address 0x5be9046 is 6 bytes inside a block of size 24 free'd
  ==4607==    at 0x4C2BFC6: realloc (vg_replace_malloc.c:687)
  ==4607==    by 0x405F6B: xrealloc (wrapper.c:100)
  ==4607==    by 0x40794E: strbuf_grow (strbuf.c:74)
  ==4607==    by 0x40854D: strbuf_vaddf (strbuf.c:268)
  ==4607==    by 0x40817E: strbuf_addf (strbuf.c:203)
  ==4607==    by 0x404300: append_normalized_escapes (urlmatch.c:58)
  ==4607==    by 0x404C0A: url_normalize (urlmatch.c:291)
  ==4607==    by 0x403F33: main (test-urlmatch-normalization.c:34)

It went undetected for a while because it does not fail the test: the calls to test-urlmatch-normalization happen inside a $() substitution.

I checked the other call sites to append_normalized_escapes() for the same type of problem, and they seem to be okay.

 urlmatch.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/urlmatch.c b/urlmatch.c
index 1db76c8..59abc80 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -281,7 +281,8 @@ char *url_normalize(const char *url, struct url_info *out_info)
 		url_len--;
 	}
 	for (;;) {
-		const char *seg_start = norm.buf + norm.len;
+		const char *seg_start;
+		size_t prev_len = norm.len;
 		const char *next_slash = url + strcspn(url, "/?#");
 		int skip_add_slash = 0;
 		/*
@@ -297,6 +298,7 @@ char *url_normalize(const char *url, struct url_info *out_info)
 			strbuf_release(&norm);
 			return NULL;
 		}
+		seg_start = norm.buf + prev_len;
 		if (!strcmp(seg_start, ".")) {
 			/* ignore a . segment; be careful not to remove initial '/' */
 			if (seg_start == path_start + 1) {

-- 
1.8.4.609.g4395a4f