@matheusbsbs / matheus-bagliano / commits / 3f018ec716

fast-import: fix over-allocation of marks storage

Fast-import stores its marks in a trie-like structure made of mark_set structs. Each struct has a fixed size (1024). If our id number is too large to fit in the struct, then we allocate a new struct which shifts the id number by 10 bits. Our original struct becomes a child node of this new layer, and the new struct becomes the top level of the trie. This scheme was broken by ddddf8d7e2 (fast-import: permit reading multiple marks files, 2020-02-22). Before then, we had a top-level "marks" pointer, and the push-down worked by assigning the new top-level struct to "marks". But after that commit, insert_mark() takes a pointer to the mark_set, rather than using the global "marks". It continued to assign to the global "marks" variable during the push down, which was wrong for two reasons: - we added a call in option_rewrite_submodules() which uses a separate mark set; pushing down on "marks" is outright wrong here. We'd corrupt the "marks" set, and we'd fail to correctly store any submodule mappings with an id over 1024. - the other callers passed "marks", but the push-down was still wrong. In read_mark_file(), we take the pointer to the mark_set as a parameter. So even though insert_mark() was updating the global "marks", the local pointer we had in read_mark_file() was not updated. As a result, we'd add a new level when needed, but then the next call to insert_mark() wouldn't see it! It would then allocate a new layer, which would also not be seen, and so on. Lookups for the lost layers obviously wouldn't work, but before we even hit any lookup stage, we'd generally run out of memory and die. Our tests didn't notice either of these cases because they didn't have enough marks to trigger the push-down behavior. The new tests in t9304 cover both cases (and fail without this patch). We can solve the problem by having insert_mark() take a pointer-to-pointer of the top-level of the set. Then our push down can assign to it in a way that the caller actually sees. Note the subtle reordering in option_rewrite_submodules(). Our call to read_mark_file() may modify our top-level set pointer, so we have to wait until after it returns to assign its value into the string_list. Reported-by: Sergey Brester <serg.brester@sebres.de> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 15, 2020 at 11:38 UTC 3f018ec716292b4d757385686f42f57af3bca685

2 files changed +68 -14

builtin/fast-import.c

+17 -14

@@ -150,7 +150,7 @@ struct recent_command {

150 char *buf;

151 };

152

153 -typedef void (*mark_set_inserter_t)(struct mark_set *s, struct object_id *oid, uintmax_t mark);

153 +typedef void (*mark_set_inserter_t)(struct mark_set **s, struct object_id *oid, uintmax_t mark);

154 typedef void (*each_mark_fn_t)(uintmax_t mark, void *obj, void *cbp);

155

156 /* Configured limits on output */

@@ -526,13 +526,15 @@ static unsigned int hc_str(const char *s, size_t len)

526 return r;

527 }

528

529 -static void insert_mark(struct mark_set *s, uintmax_t idnum, struct object_entry *oe)

529 +static void insert_mark(struct mark_set **top, uintmax_t idnum, struct object_entry *oe)

530 {

531 + struct mark_set *s = *top;

532 +

533 while ((idnum >> s->shift) >= 1024) {

534 s = mem_pool_calloc(&fi_mem_pool, 1, sizeof(struct mark_set));

533 - s->shift = marks->shift + 10;

534 - s->data.sets[0] = marks;

535 - marks = s;

535 + s->shift = (*top)->shift + 10;

536 + s->data.sets[0] = *top;

537 + *top = s;

538 }

539 while (s->shift) {

540 uintmax_t i = idnum >> s->shift;

@@ -944,7 +946,7 @@ static int store_object(

946

947 e = insert_object(&oid);

948 if (mark)

947 - insert_mark(marks, mark, e);

949 + insert_mark(&marks, mark, e);

950 if (e->idx.offset) {

951 duplicate_count_by_type[type]++;

952 return 1;

@@ -1142,7 +1144,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)

1144 e = insert_object(&oid);

1145

1146 if (mark)

1145 - insert_mark(marks, mark, e);

1147 + insert_mark(&marks, mark, e);

1148

1149 if (e->idx.offset) {

1150 duplicate_count_by_type[OBJ_BLOB]++;

@@ -1717,7 +1719,7 @@ static void dump_marks(void)

1719 }

1720 }

1721

1720 -static void insert_object_entry(struct mark_set *s, struct object_id *oid, uintmax_t mark)

1722 +static void insert_object_entry(struct mark_set **s, struct object_id *oid, uintmax_t mark)

1723 {

1724 struct object_entry *e;

1725 e = find_object(oid);

@@ -1734,12 +1736,12 @@ static void insert_object_entry(struct mark_set *s, struct object_id *oid, uintm

1736 insert_mark(s, mark, e);

1737 }

1738

1737 -static void insert_oid_entry(struct mark_set *s, struct object_id *oid, uintmax_t mark)

1739 +static void insert_oid_entry(struct mark_set **s, struct object_id *oid, uintmax_t mark)

1740 {

1741 insert_mark(s, mark, xmemdupz(oid, sizeof(*oid)));

1742 }

1743

1742 -static void read_mark_file(struct mark_set *s, FILE *f, mark_set_inserter_t inserter)

1744 +static void read_mark_file(struct mark_set **s, FILE *f, mark_set_inserter_t inserter)

1745 {

1746 char line[512];

1747 while (fgets(line, sizeof(line), f)) {

@@ -1772,7 +1774,7 @@ static void read_marks(void)

1774 goto done; /* Marks file does not exist */

1775 else

1776 die_errno("cannot read '%s'", import_marks_file);

1775 - read_mark_file(marks, f, insert_object_entry);

1777 + read_mark_file(&marks, f, insert_object_entry);

1778 fclose(f);

1779 done:

1780 import_marks_file_done = 1;

@@ -3228,7 +3230,7 @@ static void parse_alias(void)

3230 die(_("Expected 'to' command, got %s"), command_buf.buf);

3231 e = find_object(&b.oid);

3232 assert(e);

3231 - insert_mark(marks, next_mark, e);

3233 + insert_mark(&marks, next_mark, e);

3234 }

3235

3236 static char* make_fast_import_path(const char *path)

@@ -3321,13 +3323,14 @@ static void option_rewrite_submodules(const char *arg, struct string_list *list)

3323 *f = '\0';

3324 f++;

3325 ms = xcalloc(1, sizeof(*ms));

3324 - string_list_insert(list, s)->util = ms;

3326

3327 fp = fopen(f, "r");

3328 if (!fp)

3329 die_errno("cannot read '%s'", f);

3329 - read_mark_file(ms, fp, insert_oid_entry);

3330 + read_mark_file(&ms, fp, insert_oid_entry);

3331 fclose(fp);

3332 +

3333 + string_list_insert(list, s)->util = ms;

3334 }

3335

3336 static int parse_one_option(const char *option)

t/t9304-fast-import-marks.sh new

+51

@@ -0,0 +1,51 @@

1 +#!/bin/sh

2 +

3 +test_description='test exotic situations with marks'

4 +. ./test-lib.sh

5 +

6 +test_expect_success 'setup dump of basic history' '

7 + test_commit one &&

8 + git fast-export --export-marks=marks HEAD >dump

9 +'

10 +

11 +test_expect_success 'setup large marks file' '

12 + # normally a marks file would have a lot of useful, unique

13 + # marks. But for our purposes, just having a lot of nonsense

14 + # ones is fine. Start at 1024 to avoid clashing with marks

15 + # legitimately used in our tiny dump.

16 + blob=$(git rev-parse HEAD:one.t) &&

17 + for i in $(test_seq 1024 16384)

18 + do

19 + echo ":$i $blob"

20 + done >>marks

21 +'

22 +

23 +test_expect_success 'import with large marks file' '

24 + git fast-import --import-marks=marks <dump

25 +'

26 +

27 +test_expect_success 'setup dump with submodule' '

28 + git submodule add "$PWD" sub &&

29 + git commit -m "add submodule" &&

30 + git fast-export HEAD >dump

31 +'

32 +

33 +test_expect_success 'setup submodule mapping with large id' '

34 + old=$(git rev-parse HEAD:sub) &&

35 + new=$(echo $old | sed s/./a/g) &&

36 + echo ":12345 $old" >from &&

37 + echo ":12345 $new" >to

38 +'

39 +

40 +test_expect_success 'import with submodule mapping' '

41 + git init dst &&

42 + git -C dst fast-import \

43 + --rewrite-submodules-from=sub:../from \

44 + --rewrite-submodules-to=sub:../to \

45 + <dump &&

46 + git -C dst rev-parse HEAD:sub >actual &&

47 + echo "$new" >expect &&

48 + test_cmp expect actual

49 +'

50 +

51 +test_done