Skip to content

Commit ce9bf42

Browse files
committed
fsmonitor: don't fill bitmap with entries to be removed
While doing some testing with fsmonitor enabled I found that git commands would segfault after staging and unstaging an untracked file. Looking at the crash it appeared that fsmonitor_ewah_callback was attempting to adjust bits beyond the bounds of the index cache. Digging into how this could happen it became clear that the fsmonitor extension must have been written with more bits than there were entries in the index. The root cause ended up being that fill_fsmonitor_bitmap was populating fsmonitor_dirty with bits for all entries in the index, even those that had been marked for removal. To solve this problem fill_fsmonitor_bitmap has been updated to skip entries with the the CE_REMOVE flag set. With this change the bits written for the fsmonitor extension will be consistent with the index entries written by do_write_index. Additionally, BUG checks have been added to detect if the number of bits in fsmonitor_dirty should ever exceed the number of entries in the index again. Another option that was considered was moving the call to fill_fsmonitor_bitmap closer to where the index is written (and where the fsmonitor extension itself is written). However, that did not work as the fsmonitor_dirty bitmap must be filled before the index is split during writing. Signed-off-by: William Baker <[email protected]>
1 parent 5fa0f52 commit ce9bf42

File tree

3 files changed

+60
-5
lines changed

3 files changed

+60
-5
lines changed

fsmonitor.c

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,13 @@ struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
1414
static void fsmonitor_ewah_callback(size_t pos, void *is)
1515
{
1616
struct index_state *istate = (struct index_state *)is;
17-
struct cache_entry *ce = istate->cache[pos];
17+
struct cache_entry *ce;
18+
19+
if (pos >= istate->cache_nr)
20+
BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %"PRIuMAX")",
21+
(uintmax_t)pos, (uintmax_t)istate->cache_nr);
1822

23+
ce = istate->cache[pos];
1924
ce->ce_flags &= ~CE_FSMONITOR_VALID;
2025
}
2126

@@ -50,17 +55,24 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
5055
}
5156
istate->fsmonitor_dirty = fsmonitor_dirty;
5257

58+
if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
59+
BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
60+
(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
61+
5362
trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
5463
return 0;
5564
}
5665

5766
void fill_fsmonitor_bitmap(struct index_state *istate)
5867
{
59-
unsigned int i;
68+
unsigned int i, skipped = 0;
6069
istate->fsmonitor_dirty = ewah_new();
61-
for (i = 0; i < istate->cache_nr; i++)
62-
if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
63-
ewah_set(istate->fsmonitor_dirty, i);
70+
for (i = 0; i < istate->cache_nr; i++) {
71+
if (istate->cache[i]->ce_flags & CE_REMOVE)
72+
skipped++;
73+
else if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
74+
ewah_set(istate->fsmonitor_dirty, i - skipped);
75+
}
6476
}
6577

6678
void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
@@ -71,6 +83,10 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
7183
uint32_t ewah_size = 0;
7284
int fixup = 0;
7385

86+
if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
87+
BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
88+
(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
89+
7490
put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
7591
strbuf_add(sb, &hdr_version, sizeof(uint32_t));
7692

@@ -236,6 +252,9 @@ void tweak_fsmonitor(struct index_state *istate)
236252
}
237253

238254
/* Mark all previously saved entries as dirty */
255+
if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
256+
BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %"PRIuMAX")",
257+
(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
239258
ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
240259

241260
/* Now mark the untracked cache for fsmonitor usage */

t/t7519-status-fsmonitor.sh

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,4 +354,16 @@ test_expect_success 'discard_index() also discards fsmonitor info' '
354354
test_cmp expect actual
355355
'
356356

357+
# Use test files that start with 'z' so that the entries being added
358+
# and removed appear at the end of the index.
359+
test_expect_success 'status succeeds after staging/unstaging ' '
360+
test_commit initial &&
361+
removed=$(test_seq 1 100 | sed "s/^/z/") &&
362+
touch $removed &&
363+
git add $removed &&
364+
test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-env" &&
365+
FSMONITOR_LIST="$removed" git restore -S $removed &&
366+
FSMONITOR_LIST="$removed" git status
367+
'
368+
357369
test_done

t/t7519/fsmonitor-env

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
#!/bin/sh
2+
#
3+
# An test hook script to integrate with git to test fsmonitor.
4+
#
5+
# The hook is passed a version (currently 1) and a time in nanoseconds
6+
# formatted as a string and outputs to stdout all files that have been
7+
# modified since the given time. Paths must be relative to the root of
8+
# the working tree and separated by a single NUL.
9+
#
10+
#echo "$0 $*" >&2
11+
12+
if test "$#" -ne 2
13+
then
14+
echo "$0: exactly 2 arguments expected" >&2
15+
exit 2
16+
fi
17+
18+
if test "$1" != 1
19+
then
20+
echo "Unsupported core.fsmonitor hook version." >&2
21+
exit 1
22+
fi
23+
24+
printf '%s\n' $FSMONITOR_LIST

0 commit comments

Comments
 (0)