From 05ac978495e84fe03d91ab591524b7ea138900ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 6 Oct 2011 18:03:35 +0200 Subject: [PATCH 1/7] pickaxe: plug diff filespec leak with empty needle Check first for the unlikely case of an empty needle string and only then populate the filespec, lest we leak it. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index c3760cfef..0835a3be8 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -152,10 +152,10 @@ static unsigned int contains(struct diff_filespec *one, unsigned int cnt; unsigned long sz; const char *data; - if (diff_populate_filespec(one, 0)) - return 0; if (!len) return 0; + if (diff_populate_filespec(one, 0)) + return 0; sz = one->size; data = one->data; From 2b5f07f16c9e554482ed4a2355a051feabd62848 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 6 Oct 2011 18:14:55 +0200 Subject: [PATCH 2/7] pickaxe: plug regex leak With -G... --pickaxe-all, free the regex before returning even if we found a match. Also get rid of the variable has_changes, as we can simply break out of the loop. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 0835a3be8..96f7ea67d 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -96,7 +96,7 @@ static int diff_grep(struct diff_filepair *p, regex_t *regexp, struct diff_optio static void diffcore_pickaxe_grep(struct diff_options *o) { struct diff_queue_struct *q = &diff_queued_diff; - int i, has_changes, err; + int i, err; regex_t regex; struct diff_queue_struct outq; outq.queue = NULL; @@ -112,13 +112,11 @@ static void diffcore_pickaxe_grep(struct diff_options *o) if (o->pickaxe_opts & DIFF_PICKAXE_ALL) { /* Showing the whole changeset if needle exists */ - for (i = has_changes = 0; !has_changes && i < q->nr; i++) { + for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; if (diff_grep(p, ®ex, o)) - has_changes++; + goto out; /* do not munge the queue */ } - if (has_changes) - return; /* do not munge the queue */ /* * Otherwise we will clear the whole queue by copying @@ -138,10 +136,11 @@ static void diffcore_pickaxe_grep(struct diff_options *o) } } - regfree(®ex); - free(q->queue); *q = outq; + + out: + regfree(®ex); return; } From 8e854b00d86070dd463a91bdbc401fb25dc9ddb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 6 Oct 2011 18:23:11 +0200 Subject: [PATCH 3/7] pickaxe: plug regex/kws leak With -S... --pickaxe-all, free the regex or the kws before returning even if we found a match. Also get rid of the variable has_changes, as we can simply break out of the loop. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 96f7ea67d..c367d8d17 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -221,7 +221,7 @@ static void diffcore_pickaxe_count(struct diff_options *o) if (opts & DIFF_PICKAXE_ALL) { /* Showing the whole changeset if needle exists */ - for (i = has_changes = 0; !has_changes && i < q->nr; i++) { + for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; if (!DIFF_FILE_VALID(p->one)) { if (!DIFF_FILE_VALID(p->two)) @@ -238,9 +238,9 @@ static void diffcore_pickaxe_count(struct diff_options *o) contains(p->one, needle, len, regexp, kws) != contains(p->two, needle, len, regexp, kws)) has_changes++; + if (has_changes) + goto out; /* do not munge the queue */ } - if (has_changes) - return; /* not munge the queue */ /* otherwise we will clear the whole queue * by copying the empty outq at the end of this @@ -278,13 +278,14 @@ static void diffcore_pickaxe_count(struct diff_options *o) diff_free_filepair(p); } + free(q->queue); + *q = outq; + + out: if (opts & DIFF_PICKAXE_REGEX) regfree(®ex); else kwsfree(kws); - - free(q->queue); - *q = outq; return; } From 15dafaf80dda8e90114ae178569b9600ebd07109 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 6 Oct 2011 18:26:24 +0200 Subject: [PATCH 4/7] pickaxe: factor out has_changes Move duplicate if/else construct into its own helper function. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 57 +++++++++++++++++----------------------------- 1 file changed, 21 insertions(+), 36 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index c367d8d17..4347ec108 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -190,13 +190,31 @@ static unsigned int contains(struct diff_filespec *one, return cnt; } +static int has_changes(struct diff_filepair *p, const char *needle, + unsigned long len, regex_t *regexp, kwset_t kws) +{ + if (!DIFF_FILE_VALID(p->one)) { + if (!DIFF_FILE_VALID(p->two)) + return 0; /* ignore unmerged */ + /* created */ + return contains(p->two, needle, len, regexp, kws) != 0; + } + if (!DIFF_FILE_VALID(p->two)) + return contains(p->one, needle, len, regexp, kws) != 0; + if (!diff_unmodified_pair(p)) { + return contains(p->one, needle, len, regexp, kws) != + contains(p->two, needle, len, regexp, kws); + } + return 0; +} + static void diffcore_pickaxe_count(struct diff_options *o) { const char *needle = o->pickaxe; int opts = o->pickaxe_opts; struct diff_queue_struct *q = &diff_queued_diff; unsigned long len = strlen(needle); - int i, has_changes; + int i; regex_t regex, *regexp = NULL; kwset_t kws = NULL; struct diff_queue_struct outq; @@ -223,22 +241,7 @@ static void diffcore_pickaxe_count(struct diff_options *o) /* Showing the whole changeset if needle exists */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (!DIFF_FILE_VALID(p->one)) { - if (!DIFF_FILE_VALID(p->two)) - continue; /* ignore unmerged */ - /* created */ - if (contains(p->two, needle, len, regexp, kws)) - has_changes++; - } - else if (!DIFF_FILE_VALID(p->two)) { - if (contains(p->one, needle, len, regexp, kws)) - has_changes++; - } - else if (!diff_unmodified_pair(p) && - contains(p->one, needle, len, regexp, kws) != - contains(p->two, needle, len, regexp, kws)) - has_changes++; - if (has_changes) + if (has_changes(p, needle, len, regexp, kws)) goto out; /* do not munge the queue */ } @@ -254,25 +257,7 @@ static void diffcore_pickaxe_count(struct diff_options *o) /* Showing only the filepairs that has the needle */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - has_changes = 0; - if (!DIFF_FILE_VALID(p->one)) { - if (!DIFF_FILE_VALID(p->two)) - ; /* ignore unmerged */ - /* created */ - else if (contains(p->two, needle, len, regexp, - kws)) - has_changes = 1; - } - else if (!DIFF_FILE_VALID(p->two)) { - if (contains(p->one, needle, len, regexp, kws)) - has_changes = 1; - } - else if (!diff_unmodified_pair(p) && - contains(p->one, needle, len, regexp, kws) != - contains(p->two, needle, len, regexp, kws)) - has_changes = 1; - - if (has_changes) + if (has_changes(p, needle, len, regexp, kws)) diff_q(&outq, p); else diff_free_filepair(p); From 5d176fb6b6a22f19f7a15fc344dfb08af1e29ed7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 6 Oct 2011 18:50:06 +0200 Subject: [PATCH 5/7] pickaxe: pass diff_options to contains and has_changes Remove the unused parameter needle from contains() and has_changes(). Also replace the parameter len with a pointer to the diff_options. We can use its member pickaxe to check if the needle is an empty string and use the kwsmatch structure to find out the length of the match instead. This change is done as a preparation to unify the signatures of has_changes() and diff_grep(), which will be used in the patch after the next one to factor out common code. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 4347ec108..4d66ba9b7 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -144,14 +144,13 @@ static void diffcore_pickaxe_grep(struct diff_options *o) return; } -static unsigned int contains(struct diff_filespec *one, - const char *needle, unsigned long len, +static unsigned int contains(struct diff_filespec *one, struct diff_options *o, regex_t *regexp, kwset_t kws) { unsigned int cnt; unsigned long sz; const char *data; - if (!len) + if (!o->pickaxe[0]) return 0; if (diff_populate_filespec(one, 0)) return 0; @@ -175,14 +174,15 @@ static unsigned int contains(struct diff_filespec *one, } else { /* Classic exact string match */ while (sz) { - size_t offset = kwsexec(kws, data, sz, NULL); + struct kwsmatch kwsm; + size_t offset = kwsexec(kws, data, sz, &kwsm); const char *found; if (offset == -1) break; else found = data + offset; - sz -= found - data + len; - data = found + len; + sz -= found - data + kwsm.size[0]; + data = found + kwsm.size[0]; cnt++; } } @@ -190,20 +190,20 @@ static unsigned int contains(struct diff_filespec *one, return cnt; } -static int has_changes(struct diff_filepair *p, const char *needle, - unsigned long len, regex_t *regexp, kwset_t kws) +static int has_changes(struct diff_filepair *p, struct diff_options *o, + regex_t *regexp, kwset_t kws) { if (!DIFF_FILE_VALID(p->one)) { if (!DIFF_FILE_VALID(p->two)) return 0; /* ignore unmerged */ /* created */ - return contains(p->two, needle, len, regexp, kws) != 0; + return contains(p->two, o, regexp, kws) != 0; } if (!DIFF_FILE_VALID(p->two)) - return contains(p->one, needle, len, regexp, kws) != 0; + return contains(p->one, o, regexp, kws) != 0; if (!diff_unmodified_pair(p)) { - return contains(p->one, needle, len, regexp, kws) != - contains(p->two, needle, len, regexp, kws); + return contains(p->one, o, regexp, kws) != + contains(p->two, o, regexp, kws); } return 0; } @@ -241,7 +241,7 @@ static void diffcore_pickaxe_count(struct diff_options *o) /* Showing the whole changeset if needle exists */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (has_changes(p, needle, len, regexp, kws)) + if (has_changes(p, o, regexp, kws)) goto out; /* do not munge the queue */ } @@ -257,7 +257,7 @@ static void diffcore_pickaxe_count(struct diff_options *o) /* Showing only the filepairs that has the needle */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (has_changes(p, needle, len, regexp, kws)) + if (has_changes(p, o, regexp, kws)) diff_q(&outq, p); else diff_free_filepair(p); From db99cb700098942d3fced9e5d8ec873aabe6e1ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 6 Oct 2011 18:50:18 +0200 Subject: [PATCH 6/7] pickaxe: give diff_grep the same signature as has_changes Change diff_grep() to match the signature of has_changes() as a preparation for the next patch that will use function pointers to the two. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 4d66ba9b7..226fa0c5a 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -45,7 +45,8 @@ static void fill_one(struct diff_filespec *one, } } -static int diff_grep(struct diff_filepair *p, regex_t *regexp, struct diff_options *o) +static int diff_grep(struct diff_filepair *p, struct diff_options *o, + regex_t *regexp, kwset_t kws) { regmatch_t regmatch; struct userdiff_driver *textconv_one = NULL; @@ -114,7 +115,7 @@ static void diffcore_pickaxe_grep(struct diff_options *o) /* Showing the whole changeset if needle exists */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (diff_grep(p, ®ex, o)) + if (diff_grep(p, o, ®ex, NULL)) goto out; /* do not munge the queue */ } @@ -129,7 +130,7 @@ static void diffcore_pickaxe_grep(struct diff_options *o) /* Showing only the filepairs that has the needle */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if (diff_grep(p, ®ex, o)) + if (diff_grep(p, o, ®ex, NULL)) diff_q(&outq, p); else diff_free_filepair(p); From 8a94151d61eda539a3588f2ad009e0117867525f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 6 Oct 2011 18:50:28 +0200 Subject: [PATCH 7/7] pickaxe: factor out pickaxe Move the duplicate diff queue loop into its own function that accepts a match function: has_changes() for -S and diff_grep() for -G. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- diffcore-pickaxe.c | 110 ++++++++++++++++++--------------------------- 1 file changed, 43 insertions(+), 67 deletions(-) diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 226fa0c5a..380a837b5 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c @@ -8,6 +8,46 @@ #include "xdiff-interface.h" #include "kwset.h" +typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, regex_t *regexp, kwset_t kws); + +static void pickaxe(struct diff_queue_struct *q, struct diff_options *o, + regex_t *regexp, kwset_t kws, pickaxe_fn fn) +{ + int i; + struct diff_queue_struct outq; + + DIFF_QUEUE_CLEAR(&outq); + + if (o->pickaxe_opts & DIFF_PICKAXE_ALL) { + /* Showing the whole changeset if needle exists */ + for (i = 0; i < q->nr; i++) { + struct diff_filepair *p = q->queue[i]; + if (fn(p, o, regexp, kws)) + return; /* do not munge the queue */ + } + + /* + * Otherwise we will clear the whole queue by copying + * the empty outq at the end of this function, but + * first clear the current entries in the queue. + */ + for (i = 0; i < q->nr; i++) + diff_free_filepair(q->queue[i]); + } else { + /* Showing only the filepairs that has the needle */ + for (i = 0; i < q->nr; i++) { + struct diff_filepair *p = q->queue[i]; + if (fn(p, o, regexp, kws)) + diff_q(&outq, p); + else + diff_free_filepair(p); + } + } + + free(q->queue); + *q = outq; +} + struct diffgrep_cb { regex_t *regexp; int hit; @@ -96,12 +136,8 @@ static int diff_grep(struct diff_filepair *p, struct diff_options *o, static void diffcore_pickaxe_grep(struct diff_options *o) { - struct diff_queue_struct *q = &diff_queued_diff; - int i, err; + int err; regex_t regex; - struct diff_queue_struct outq; - outq.queue = NULL; - outq.nr = outq.alloc = 0; err = regcomp(®ex, o->pickaxe, REG_EXTENDED | REG_NEWLINE); if (err) { @@ -111,36 +147,8 @@ static void diffcore_pickaxe_grep(struct diff_options *o) die("invalid log-grep regex: %s", errbuf); } - if (o->pickaxe_opts & DIFF_PICKAXE_ALL) { - /* Showing the whole changeset if needle exists */ - for (i = 0; i < q->nr; i++) { - struct diff_filepair *p = q->queue[i]; - if (diff_grep(p, o, ®ex, NULL)) - goto out; /* do not munge the queue */ - } + pickaxe(&diff_queued_diff, o, ®ex, NULL, diff_grep); - /* - * Otherwise we will clear the whole queue by copying - * the empty outq at the end of this function, but - * first clear the current entries in the queue. - */ - for (i = 0; i < q->nr; i++) - diff_free_filepair(q->queue[i]); - } else { - /* Showing only the filepairs that has the needle */ - for (i = 0; i < q->nr; i++) { - struct diff_filepair *p = q->queue[i]; - if (diff_grep(p, o, ®ex, NULL)) - diff_q(&outq, p); - else - diff_free_filepair(p); - } - } - - free(q->queue); - *q = outq; - - out: regfree(®ex); return; } @@ -213,13 +221,9 @@ static void diffcore_pickaxe_count(struct diff_options *o) { const char *needle = o->pickaxe; int opts = o->pickaxe_opts; - struct diff_queue_struct *q = &diff_queued_diff; unsigned long len = strlen(needle); - int i; regex_t regex, *regexp = NULL; kwset_t kws = NULL; - struct diff_queue_struct outq; - DIFF_QUEUE_CLEAR(&outq); if (opts & DIFF_PICKAXE_REGEX) { int err; @@ -238,36 +242,8 @@ static void diffcore_pickaxe_count(struct diff_options *o) kwsprep(kws); } - if (opts & DIFF_PICKAXE_ALL) { - /* Showing the whole changeset if needle exists */ - for (i = 0; i < q->nr; i++) { - struct diff_filepair *p = q->queue[i]; - if (has_changes(p, o, regexp, kws)) - goto out; /* do not munge the queue */ - } - - /* otherwise we will clear the whole queue - * by copying the empty outq at the end of this - * function, but first clear the current entries - * in the queue. - */ - for (i = 0; i < q->nr; i++) - diff_free_filepair(q->queue[i]); - } - else - /* Showing only the filepairs that has the needle */ - for (i = 0; i < q->nr; i++) { - struct diff_filepair *p = q->queue[i]; - if (has_changes(p, o, regexp, kws)) - diff_q(&outq, p); - else - diff_free_filepair(p); - } - - free(q->queue); - *q = outq; + pickaxe(&diff_queued_diff, o, regexp, kws, has_changes); - out: if (opts & DIFF_PICKAXE_REGEX) regfree(®ex); else