Skip to content

Commit

Permalink
merge: Fix double free issue after merging the diffs
Browse files Browse the repository at this point in the history
This commit addresses a double free issue that occurs when
merging two diffs and free the diff objects.

The problem was caused from copying the pointer using a vector swap
git function and then make a deep free of the original vector.

This operation made on a vector of pointers deallocated all the
pointers inside the new vector where we swap the content to.

As a result, an invalid object was returned to the user.

The valgrid report.

```
==256010== HEAP SUMMARY:
==256010==     in use at exit: 696,247 bytes in 11,244 blocks
==256010==   total heap usage: 50,123 allocs, 38,880 frees, 4,419,142 bytes allocated
==256010==
==256010== Searching for pointers to 11,244 not-freed blocks
==256010== Checked 850,720 bytes
==256010==
==256010== 112 bytes in 1 blocks are definitely lost in loss record 69 of 163
==256010==    at 0x4841848: malloc (vg_replace_malloc.c:431)
==256010==    by 0x48964FD: stdalloc__malloc.lto_priv.0 (stdalloc.c:22)
==256010==    by 0x48D4C34: git_diff__delta_dup (diff_tform.c:23)
==256010==    by 0x48DAA22: git_diff__merge_like_cgit (diff_tform.c:81)
==256010==    by 0x48D5C14: git_diff__merge (diff_tform.c:160)
==256010==    by 0x1092A6: main (in /home/vincent/Github/libgit2/build/a.out)
==256010==
==256010== LEAK SUMMARY:
==256010==    definitely lost: 112 bytes in 1 blocks
==256010==    indirectly lost: 0 bytes in 0 blocks
==256010==      possibly lost: 0 bytes in 0 blocks
==256010==    still reachable: 696,135 bytes in 11,243 blocks
==256010==         suppressed: 0 bytes in 0 blocks
==256010== Reachable blocks (those to which a pointer was found) are not shown.
==256010== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==256010==
==256010== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
==256010==
==256010== 1 errors in context 1 of 2:
==256010== Invalid free() / delete / delete[] / realloc()
==256010==    at 0x484412F: free (vg_replace_malloc.c:974)
==256010==    by 0x490D5B0: patch_parsed__free.part.0.lto_priv.0 (patch_parse.c:1150)
==256010==    by 0x48CC454: diff_parsed_free (diff_parse.c:21)
==256010==    by 0x1092EA: main (in /home/vincent/Github/libgit2/build/a.out)
==256010==  Address 0x59e55b0 is 0 bytes inside a block of size 112 free'd
==256010==    at 0x484412F: free (vg_replace_malloc.c:974)
==256010==    by 0x48A17CD: git_vector_free_deep.part.0 (vector.c:99)
==256010==    by 0x48D5A2B: UnknownInlinedFun (vector.c:95)
==256010==    by 0x48D5A2B: git_diff__merge (diff_tform.c:193)
==256010==    by 0x1092A6: main (in /home/vincent/Github/libgit2/build/a.out)
==256010==  Block was alloc'd at
==256010==    at 0x48469B3: calloc (vg_replace_malloc.c:1554)
==256010==    by 0x489652D: stdalloc__calloc.lto_priv.0 (stdalloc.c:42)
==256010==    by 0x48CCDE9: UnknownInlinedFun (patch_parse.c:1184)
==256010==    by 0x48CCDE9: git_diff_from_buffer (diff_parse.c:86)
==256010==    by 0x109219: main (in /home/vincent/Github/libgit2/build/a.out)
==256010==
==256010== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
```

Link: libgit2#6588
Reported-by: Eric Huss
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
  • Loading branch information
vincenzopalazzo committed Jul 9, 2023
1 parent f041a94 commit 99da38d
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 3 deletions.
9 changes: 6 additions & 3 deletions src/libgit2/diff_tform.c
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,12 @@ int git_diff__merge(
git_pool_strdup_safe(&onto->pool, onto->opts.old_prefix);
onto->opts.new_prefix =
git_pool_strdup_safe(&onto->pool, onto->opts.new_prefix);
}

git_vector_free_deep(&onto_new);
/* Since we swap the vector, there is no need to perform a deep free; otherwise, we
* deliver to the caller a partially dead object. */
git_vector_free(&onto_new);
} else
/* In the case of an error, it is safe to perform a deep free of the vector. */
git_vector_free_deep(&onto_new);
git_pool_clear(&onto_pool);

return error;
Expand Down
29 changes: 29 additions & 0 deletions tests/libgit2/diff/patch.c
Original file line number Diff line number Diff line change
Expand Up @@ -701,3 +701,32 @@ void test_diff_patch__can_strip_bad_utf8(void)
git_patch_free(patch);
git_buf_dispose(&buf);
}

void test_diff_patch__free_diff_after_git_diff_merge(void)
{
git_diff *diff_a;
git_diff *diff_b;

const char *a = "diff --git a/README.md b/README.md\n"
"index 18fb8328..ce60f40c 100644\n"
"--- a/README.md\n"
"+++ b/README.md\n"
"@@ -4,1 +4,2 @@ componentwise\n"
" reusing\n"
"+proverb\n";

const char *b = "diff --git a/README.md b/README.md\n"
"index 18fb8328..ce60f40c 100644\n"
"--- a/README.md\n"
"+++ b/README.md\n"
"@@ -4,2 +4,3 @@ componentwise\n"
" reusing\n"
" proverb\n"
"+offended\n";

cl_git_pass(git_diff_from_buffer(&diff_a, a, strlen(a)));
cl_git_pass(git_diff_from_buffer(&diff_b, b, strlen(b)));
cl_git_pass(git_diff_merge(diff_a, diff_b));
git_diff_free(diff_b);
git_diff_free(diff_a);
}

0 comments on commit 99da38d

Please sign in to comment.