diff --color-moved-ws: fix double free crash
Running git diff --color-moved-ws=allow-indentation-change v2.18.0 v2.19.0 results in a crash due to a double free. This happens when two potential moved blocks start with consecutive lines. As pmb_advance_or_null_multi_match() advances it copies the ws_delta from the last matching line to the next. When the first of our consecutive lines is advanced its ws_delta well be copied to the second, overwriting the ws_delta of the block containing the second line. Then when the second line is advanced it will copy the new ws_delta to the line below it and so on. Eventually one of these blocks will stop matching and the ws_delta will be freed. From then on the other block is in a use-after-free state and when it stops matching it will try to free the ws_delta that has already been freed by the other block. The solution is to store the ws_delta in the array of potential moved blocks rather than with the lines. This means that it no longer needs to be copied around and one block cannot overwrite the ws_delta of another. Additionally it saves some malloc/free calls as we don't keep allocating and freeing ws_deltas. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Reviewed-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood committed
Oct 4, 2018 at 11:07 UTC
74d156f4a1b1d563b422127ee347eaa348973a0c
1 file changed
+45
-37
diff.c
+45
-37
@@ -776,7 +776,6 @@ struct moved_entry {
776
struct hashmap_entry ent;
777
const struct emitted_diff_symbol *es;
778
struct moved_entry *next_line;
779
- struct ws_delta *wsd;
779
};
780
781
/**
@@ -793,6 +792,17 @@ struct ws_delta {
792
};
793
#define WS_DELTA_INIT { NULL, 0 }
794
795
+struct moved_block {
796
+ struct moved_entry *match;
797
+ struct ws_delta wsd;
798
+};
799
+
800
+static void moved_block_clear(struct moved_block *b)
801
+{
802
+ FREE_AND_NULL(b->wsd.string);
803
+ b->match = NULL;
804
+}
805
+
806
static int compute_ws_delta(const struct emitted_diff_symbol *a,
807
const struct emitted_diff_symbol *b,
808
struct ws_delta *out)
@@ -810,7 +820,7 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,
820
static int cmp_in_block_with_wsd(const struct diff_options *o,
821
const struct moved_entry *cur,
822
const struct moved_entry *match,
813
- struct moved_entry *pmb,
823
+ struct moved_block *pmb,
824
int n)
825
{
826
struct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];
@@ -830,16 +840,15 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,
840
if (strcmp(a, b))
841
return 1;
842
833
- if (!pmb->wsd)
843
+ if (!pmb->wsd.string)
844
/*
835
- * No white space delta was carried forward? This can happen
836
- * when we exit early in this function and do not carry
837
- * forward ws.
845
+ * The white space delta is not active? This can happen
846
+ * when we exit early in this function.
847
*/
848
return 1;
849
850
/*
842
- * The indent changes of the block are known and carried forward in
851
+ * The indent changes of the block are known and stored in
852
* pmb->wsd; however we need to check if the indent changes of the
853
* current line are still the same as before.
854
*
@@ -847,8 +856,8 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,
856
* one of them for the white spaces, depending which was longer.
857
*/
858
850
- wslen = strlen(pmb->wsd->string);
851
- if (pmb->wsd->current_longer) {
859
+ wslen = strlen(pmb->wsd.string);
860
+ if (pmb->wsd.current_longer) {
861
c += wslen;
862
cl -= wslen;
863
} else {
@@ -898,7 +907,6 @@ static struct moved_entry *prepare_entry(struct diff_options *o,
907
ret->ent.hash = xdiff_hash_string(l->line, l->len, flags);
908
ret->es = l;
909
ret->next_line = NULL;
901
- ret->wsd = NULL;
910
911
return ret;
912
}
@@ -938,18 +946,18 @@ static void add_lines_to_move_detection(struct diff_options *o,
946
static void pmb_advance_or_null(struct diff_options *o,
947
struct moved_entry *match,
948
struct hashmap *hm,
941
- struct moved_entry **pmb,
949
+ struct moved_block *pmb,
950
int pmb_nr)
951
{
952
int i;
953
for (i = 0; i < pmb_nr; i++) {
946
- struct moved_entry *prev = pmb[i];
954
+ struct moved_entry *prev = pmb[i].match;
955
struct moved_entry *cur = (prev && prev->next_line) ?
956
prev->next_line : NULL;
957
if (cur && !hm->cmpfn(o, cur, match, NULL)) {
950
- pmb[i] = cur;
958
+ pmb[i].match = cur;
959
} else {
952
- pmb[i] = NULL;
960
+ pmb[i].match = NULL;
961
}
962
}
963
}
@@ -957,7 +965,7 @@ static void pmb_advance_or_null(struct diff_options *o,
965
static void pmb_advance_or_null_multi_match(struct diff_options *o,
966
struct moved_entry *match,
967
struct hashmap *hm,
960
- struct moved_entry **pmb,
968
+ struct moved_block *pmb,
969
int pmb_nr, int n)
970
{
971
int i;
@@ -965,49 +973,45 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,
973
974
for (; match; match = hashmap_get_next(hm, match)) {
975
for (i = 0; i < pmb_nr; i++) {
968
- struct moved_entry *prev = pmb[i];
976
+ struct moved_entry *prev = pmb[i].match;
977
struct moved_entry *cur = (prev && prev->next_line) ?
978
prev->next_line : NULL;
979
if (!cur)
980
continue;
973
- if (!cmp_in_block_with_wsd(o, cur, match, pmb[i], n))
981
+ if (!cmp_in_block_with_wsd(o, cur, match, &pmb[i], n))
982
got_match[i] |= 1;
983
}
984
}
985
986
for (i = 0; i < pmb_nr; i++) {
987
if (got_match[i]) {
980
- /* Carry the white space delta forward */
981
- pmb[i]->next_line->wsd = pmb[i]->wsd;
982
- pmb[i] = pmb[i]->next_line;
988
+ /* Advance to the next line */
989
+ pmb[i].match = pmb[i].match->next_line;
990
} else {
984
- if (pmb[i]->wsd) {
985
- free(pmb[i]->wsd->string);
986
- FREE_AND_NULL(pmb[i]->wsd);
987
- }
988
- pmb[i] = NULL;
991
+ moved_block_clear(&pmb[i]);
992
}
993
}
994
}
995
993
-static int shrink_potential_moved_blocks(struct moved_entry **pmb,
996
+static int shrink_potential_moved_blocks(struct moved_block *pmb,
997
int pmb_nr)
998
{
999
int lp, rp;
1000
1001
/* Shrink the set of potential block to the remaining running */
1002
for (lp = 0, rp = pmb_nr - 1; lp <= rp;) {
1000
- while (lp < pmb_nr && pmb[lp])
1003
+ while (lp < pmb_nr && pmb[lp].match)
1004
lp++;
1005
/* lp points at the first NULL now */
1006
1004
- while (rp > -1 && !pmb[rp])
1007
+ while (rp > -1 && !pmb[rp].match)
1008
rp--;
1009
/* rp points at the last non-NULL */
1010
1011
if (lp < pmb_nr && rp > -1 && lp < rp) {
1012
pmb[lp] = pmb[rp];
1010
- pmb[rp] = NULL;
1013
+ pmb[rp].match = NULL;
1014
+ pmb[rp].wsd.string = NULL;
1015
rp--;
1016
lp++;
1017
}
@@ -1054,7 +1058,7 @@ static void mark_color_as_moved(struct diff_options *o,
1058
struct hashmap *add_lines,
1059
struct hashmap *del_lines)
1060
{
1057
- struct moved_entry **pmb = NULL; /* potentially moved blocks */
1061
+ struct moved_block *pmb = NULL; /* potentially moved blocks */
1062
int pmb_nr = 0, pmb_alloc = 0;
1063
int n, flipped_block = 1, block_length = 0;
1064
@@ -1083,7 +1087,11 @@ static void mark_color_as_moved(struct diff_options *o,
1087
}
1088
1089
if (!match) {
1090
+ int i;
1091
+
1092
adjust_last_block(o, n, block_length);
1093
+ for(i = 0; i < pmb_nr; i++)
1094
+ moved_block_clear(&pmb[i]);
1095
pmb_nr = 0;
1096
block_length = 0;
1097
continue;
@@ -1111,14 +1119,12 @@ static void mark_color_as_moved(struct diff_options *o,
1119
ALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc);
1120
if (o->color_moved_ws_handling &
1121
COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {
1114
- struct ws_delta *wsd = xmalloc(sizeof(*match->wsd));
1115
- if (compute_ws_delta(l, match->es, wsd)) {
1116
- match->wsd = wsd;
1117
- pmb[pmb_nr++] = match;
1118
- } else
1119
- free(wsd);
1122
+ if (compute_ws_delta(l, match->es,
1123
+ &pmb[pmb_nr].wsd))
1124
+ pmb[pmb_nr++].match = match;
1125
} else {
1121
- pmb[pmb_nr++] = match;
1126
+ pmb[pmb_nr].wsd.string = NULL;
1127
+ pmb[pmb_nr++].match = match;
1128
}
1129
}
1130
@@ -1135,6 +1141,8 @@ static void mark_color_as_moved(struct diff_options *o,
1141
}
1142
adjust_last_block(o, n, block_length);
1143
1144
+ for(n = 0; n < pmb_nr; n++)
1145
+ moved_block_clear(&pmb[n]);
1146
free(pmb);
1147
}
1148