@matheusbsbs / matheus-bagliano / commits / 56d388e6ad

diff: avoid segfault with freed entries

When computing a diff in a partial clone, there is a chance that we could trigger a prefetch of missing objects at the same time as we are freeing entries from the global diff queue. This is difficult to reproduce, as we need to have some objects be freed from the queue before triggering the prefetch of missing objects. There is a new test in t4067 that does trigger the segmentation fault that results in this case. The fix is to set the queue pointer to NULL after it is freed, and then to be careful about NULL values in the prefetch. The more elaborate explanation is that within diffcore_std(), we may skip the initial prefetch due to the output format (--name-only in the test) and go straight to diffcore_skip_stat_unmatch(). In that method, the index entries that have been invalidated by path changes show up as entries but may be deleted because they are not actually content diffs and only newer timestamps than expected. As those entries are deleted, later entries are checked with diff_filespec_check_stat_unmatch(), which uses diff_queued_diff_prefetch() as the missing_object_cb in its diff options. That can trigger downloading missing objects if the appropriate scenario occurs to trigger a call to diff_popoulate_filespec(). It's finally within that callback to diff_queued_diff_prefetch() that the segfault occurs. The test was hard to find because it required some real differences, some not-different files that had a newer modified time, and the order of those files alphabetically was important to trigger the deletion before the prefetch was triggered. I briefly considered a "lock" member for the diff queue, but it was a much larger diff and introduced many more possible error scenarios. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Derrick Stolee committed Dec 29, 2025 at 21:44 UTC 56d388e6ad9e819935a902b6d5ce3a6b3485b6e2

2 files changed +40

diff.c

@@ -7098,6 +7098,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)

7098 if (!diffopt->flags.no_index)

7099 diffopt->skip_stat_unmatch++;

7100 diff_free_filepair(p);

7101 + q->queue[i] = NULL;

7102 }

7103 }

7104 free(q->queue);

@@ -7141,6 +7142,10 @@ void diff_queued_diff_prefetch(void *repository)

7142

7143 for (i = 0; i < q->nr; i++) {

7144 struct diff_filepair *p = q->queue[i];

7145 +

7146 + if (!p)

7147 + continue;

7148 +

7149 diff_add_if_missing(repo, &to_fetch, p->one);

7150 diff_add_if_missing(repo, &to_fetch, p->two);

7151 }

t/t4067-diff-partial-clone.sh

+35

@@ -132,6 +132,41 @@ test_expect_success 'diff with rename detection batches blobs' '

132 test_line_count = 1 done_lines

133 '

134

135 +test_expect_success 'diff succeeds even if entries are removed from queue' '

136 + test_when_finished "rm -rf server client trace" &&

137 +

138 + test_create_repo server &&

139 + for l in a c e g i p

140 + do

141 + echo $l >server/$l &&

142 + git -C server add $l || return 1

143 + done &&

144 + git -C server commit -m x &&

145 +

146 + for l in a e i

147 + do

148 + git -C server rm $l || return 1

149 + done &&

150 +

151 + for l in b d f i

152 + do

153 + echo $l$l >server/$l &&

154 + git -C server add $l || return 1

155 + done &&

156 + git -C server commit -a -m x &&

157 +

158 + test_config -C server uploadpack.allowfilter 1 &&

159 + test_config -C server uploadpack.allowanysha1inwant 1 &&

160 + git clone --filter=blob:limit=0 "file://$(pwd)/server" client &&

161 +

162 + for file in $(ls client)

163 + do

164 + cat client/$file >$file &&

165 + mv $file client/$file || return 1

166 + done &&

167 + git -C client diff --name-only --relative HEAD^

168 +'

169 +

170 test_expect_success 'diff does not fetch anything if inexact rename detection is not needed' '

171 test_when_finished "rm -rf server client trace" &&

172