Hi! We cleaned up your code for you! #14
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Almost all of these are intentional and updating the test vector without changing the matching test would break. Don't throw random crap like this without testing. Besides, development of Git itself does not happen via Github pull requests. Closing this, and blocking the troll robot. |
|
'like' |
asmeurer
pushed a commit
to asmeurer/git
that referenced
this pull request
Jan 9, 2012
pclouds
referenced
this pull request
in TortoiseGit/tgit
Jan 13, 2012
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
kusma
pushed a commit
to kusma/git
that referenced
this pull request
May 31, 2012
Reported by postiffm as issue git#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
aroben
referenced
this pull request
in aroben/git
Jun 15, 2012
Reported by postiffm as issue msysgit#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
kusma
pushed a commit
to kusma/git
that referenced
this pull request
Jun 24, 2012
Reported by postiffm as issue git#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
kblees
referenced
this pull request
in kblees/git
Oct 24, 2012
Reported by postiffm as issue msysgit#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
kblees
referenced
this pull request
in kblees/git
Oct 24, 2012
Reported by postiffm as issue msysgit#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
frogonwheels
referenced
this pull request
in frogonwheels/git
Dec 23, 2012
Reported by postiffm as issue msysgit#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
frogonwheels
referenced
this pull request
in frogonwheels/git
Feb 5, 2013
Reported by postiffm as issue msysgit#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
frogonwheels
referenced
this pull request
in frogonwheels/git
Feb 5, 2013
Reported by postiffm as issue msysgit#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
frogonwheels
referenced
this pull request
in frogonwheels/git
Feb 5, 2013
Reported by postiffm as issue msysgit#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
gitster
pushed a commit
that referenced
this pull request
Jun 7, 2013
Reported by postiffm as issue #14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
kblees
referenced
this pull request
in kblees/git
Jun 9, 2013
Reported by postiffm as issue msysgit#14. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
gitster
pushed a commit
that referenced
this pull request
Jul 9, 2018
builtin/merge.c says that when we are about to perform a merge:
...the index must be in sync with the head commit. The strategies are
responsible to ensure this.
merge-recursive has always relied on unpack_trees() to enforce this
requirement, except in the case of an "Already up to date!" merge.
unpack-trees.c does not actually enforce this requirement, though. It
allows for a pair of exceptions, in cases which it refers to as #14(ALT)
and #2ALT. Documentation/technical/trivial-merge.txt can be consulted for
the precise meanings of the various case numbers and their meanings for
unpack-trees.c, but we have a high-level description of the intent behind
these two exceptions in a combined and summarized form in
Documentation/git-merge.txt:
...[merge will] abort if there are any changes registered in the index
relative to the `HEAD` commit. (One exception is when the changed index
entries are in the state that would result from the merge already.)
While this high-level description does describe conditions under which it
would be safe to allow the index to diverge from HEAD, it does not match
what is actually implemented. In particular, unpack-trees.c has no
knowledge of renames, and these two exceptions were written assuming that
no renames take place. Once renames get into the mix, it is no longer
safe to allow the index to not match for #2ALT. We could modify
unpack-trees to only allow #14(ALT) as an exception, but that would be
more strict than required for the resolve strategy (since the resolve
strategy doesn't handle renames at all). Therefore, unpack_trees.c seems
like the wrong place to fix this.
Further, if someone fixes the combination of break and rename detection
and modifies merge-recursive to take advantage of the combination, then it
will also no longer be safe to allow the index to not match for #14(ALT)
when the recursive strategy is in use. Therefore, leaving one of the
exceptions in place with the recursive merge strategy feels like we are
just leaving a latent bug in the code for folks in the future to stumble
across.
It may be possible to fix both unpack-trees and merge-recursive in a way
that implements the exception as stated in Documentation/git-merge.txt,
but it would be somewhat complex, possibly also buggy at first, and
ultimately, not all that valuable. Instead, just enforce the requirement
stated in builtin/merge.c; error out if the index does not match the HEAD
commit, just like the 'ours' and 'octopus' strategies do.
Some testcase fixups were in order:
t7611: had many tests designed to show that `git merge --abort` could
not always restore the index and working tree to the state they
were in before the merge started. The tests that were associated
with having changes in the index before the merge started are no
longer applicable, so they have been removed.
t7504: had a few tests that had stray staged changes that were not
actually part of the test under consideration
t6044: We no longer expect stray staged changes to sometimes result
in the merge continuing. Also, fix a case where a merge
didn't abort but should have.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitster
pushed a commit
that referenced
this pull request
Jul 11, 2018
builtin/merge.c says that when we are about to perform a merge:
...the index must be in sync with the head commit. The strategies are
responsible to ensure this.
merge-recursive has always relied on unpack_trees() to enforce this
requirement, except in the case of an "Already up to date!" merge.
unpack-trees.c does not actually enforce this requirement, though. It
allows for a pair of exceptions, in cases which it refers to as #14(ALT)
and #2ALT. Documentation/technical/trivial-merge.txt can be consulted for
the precise meanings of the various case numbers and their meanings for
unpack-trees.c, but we have a high-level description of the intent behind
these two exceptions in a combined and summarized form in
Documentation/git-merge.txt:
...[merge will] abort if there are any changes registered in the index
relative to the `HEAD` commit. (One exception is when the changed index
entries are in the state that would result from the merge already.)
While this high-level description does describe conditions under which it
would be safe to allow the index to diverge from HEAD, it does not match
what is actually implemented. In particular, unpack-trees.c has no
knowledge of renames, and these two exceptions were written assuming that
no renames take place. Once renames get into the mix, it is no longer
safe to allow the index to not match for #2ALT. We could modify
unpack-trees to only allow #14(ALT) as an exception, but that would be
more strict than required for the resolve strategy (since the resolve
strategy doesn't handle renames at all). Therefore, unpack_trees.c seems
like the wrong place to fix this.
Further, if someone fixes the combination of break and rename detection
and modifies merge-recursive to take advantage of the combination, then it
will also no longer be safe to allow the index to not match for #14(ALT)
when the recursive strategy is in use. Therefore, leaving one of the
exceptions in place with the recursive merge strategy feels like we are
just leaving a latent bug in the code for folks in the future to stumble
across.
It may be possible to fix both unpack-trees and merge-recursive in a way
that implements the exception as stated in Documentation/git-merge.txt,
but it would be somewhat complex, possibly also buggy at first, and
ultimately, not all that valuable. Instead, just enforce the requirement
stated in builtin/merge.c; error out if the index does not match the HEAD
commit, just like the 'ours' and 'octopus' strategies do.
Some testcase fixups were in order:
t7611: had many tests designed to show that `git merge --abort` could
not always restore the index and working tree to the state they
were in before the merge started. The tests that were associated
with having changes in the index before the merge started are no
longer applicable, so they have been removed.
t7504: had a few tests that had stray staged changes that were not
actually part of the test under consideration
t6044: We no longer expect stray staged changes to sometimes result
in the merge continuing. Also, fix a case where a merge
didn't abort but should have.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
avar
added a commit
to avar/git
that referenced
this pull request
Feb 3, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Feb 3, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Feb 6, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Feb 6, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Feb 6, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Feb 7, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Feb 7, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Feb 8, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Feb 8, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Feb 10, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Mar 6, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Mar 7, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
Mar 7, 2023
* avar/nuke-test_i18ngrep-again: tests with git#17 test_i18ngrep: replace it with 'grep' tests with git#16 test_i18ngrep: replace it with 'grep' tests with git#15 test_i18ngrep: replace it with 'grep' tests with git#14 test_i18ngrep: replace it with 'grep' tests with git#13 test_i18ngrep: replace it with 'grep' tests with git#12 test_i18ngrep: replace it with 'grep' tests with git#11 test_i18ngrep: replace it with 'grep'
avar
added a commit
to avar/git
that referenced
this pull request
May 1, 2023
For those tests that have 14 occurances of 'test_i18ngrep', replace it with 'grep'. Splitting this incremental conversion by number of occurances in the file is arbitrary, but help so keep the commit size down. The 'test_i18ngrep' wrapper has been deprecated since d162b25 (tests: remove support for GIT_TEST_GETTEXT_POISON, 2021-01-20). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
ttaylorr
added a commit
to ttaylorr/git
that referenced
this pull request
Aug 23, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 git#6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 git#8 0x55e075e2edb0 in do_push builtin/push.c:458 git#9 0x55e075e2ff7a in cmd_push builtin/push.c:702 git#10 0x55e075d8aaf0 in run_builtin git.c:452 git#11 0x55e075d8af08 in handle_builtin git.c:706 git#12 0x55e075d8b12c in run_argv git.c:770 git#13 0x55e075d8b6a0 in cmd_main git.c:905 git#14 0x55e075e81f07 in main common-main.c:60 git#15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 git#16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 git#17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com>
ttaylorr
added a commit
to ttaylorr/git
that referenced
this pull request
Aug 23, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 git#6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 git#8 0x55e075e2edb0 in do_push builtin/push.c:458 git#9 0x55e075e2ff7a in cmd_push builtin/push.c:702 git#10 0x55e075d8aaf0 in run_builtin git.c:452 git#11 0x55e075d8af08 in handle_builtin git.c:706 git#12 0x55e075d8b12c in run_argv git.c:770 git#13 0x55e075d8b6a0 in cmd_main git.c:905 git#14 0x55e075e81f07 in main common-main.c:60 git#15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 git#16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 git#17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com>
ttaylorr
added a commit
to ttaylorr/git
that referenced
this pull request
Aug 23, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 git#6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 git#8 0x55e075e2edb0 in do_push builtin/push.c:458 git#9 0x55e075e2ff7a in cmd_push builtin/push.c:702 git#10 0x55e075d8aaf0 in run_builtin git.c:452 git#11 0x55e075d8af08 in handle_builtin git.c:706 git#12 0x55e075d8b12c in run_argv git.c:770 git#13 0x55e075d8b6a0 in cmd_main git.c:905 git#14 0x55e075e81f07 in main common-main.c:60 git#15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 git#16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 git#17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com>
ttaylorr
added a commit
to ttaylorr/git
that referenced
this pull request
Aug 24, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 git#6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 git#8 0x55e075e2edb0 in do_push builtin/push.c:458 git#9 0x55e075e2ff7a in cmd_push builtin/push.c:702 git#10 0x55e075d8aaf0 in run_builtin git.c:452 git#11 0x55e075d8af08 in handle_builtin git.c:706 git#12 0x55e075d8b12c in run_argv git.c:770 git#13 0x55e075d8b6a0 in cmd_main git.c:905 git#14 0x55e075e81f07 in main common-main.c:60 git#15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 git#16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 git#17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com>
ttaylorr
added a commit
to ttaylorr/git
that referenced
this pull request
Aug 24, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 git#6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 git#8 0x55e075e2edb0 in do_push builtin/push.c:458 git#9 0x55e075e2ff7a in cmd_push builtin/push.c:702 git#10 0x55e075d8aaf0 in run_builtin git.c:452 git#11 0x55e075d8af08 in handle_builtin git.c:706 git#12 0x55e075d8b12c in run_argv git.c:770 git#13 0x55e075d8b6a0 in cmd_main git.c:905 git#14 0x55e075e81f07 in main common-main.c:60 git#15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 git#16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 git#17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com>
ttaylorr
added a commit
to ttaylorr/git
that referenced
this pull request
Aug 24, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 git#6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 git#8 0x55e075e2edb0 in do_push builtin/push.c:458 git#9 0x55e075e2ff7a in cmd_push builtin/push.c:702 git#10 0x55e075d8aaf0 in run_builtin git.c:452 git#11 0x55e075d8af08 in handle_builtin git.c:706 git#12 0x55e075d8b12c in run_argv git.c:770 git#13 0x55e075d8b6a0 in cmd_main git.c:905 git#14 0x55e075e81f07 in main common-main.c:60 git#15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 git#16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 git#17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com>
gitster
pushed a commit
that referenced
this pull request
Aug 24, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 #6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 #8 0x55e075e2edb0 in do_push builtin/push.c:458 #9 0x55e075e2ff7a in cmd_push builtin/push.c:702 #10 0x55e075d8aaf0 in run_builtin git.c:452 #11 0x55e075d8af08 in handle_builtin git.c:706 #12 0x55e075d8b12c in run_argv git.c:770 #13 0x55e075d8b6a0 in cmd_main git.c:905 #14 0x55e075e81f07 in main common-main.c:60 #15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 #17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
ttaylorr
added a commit
to ttaylorr/git
that referenced
this pull request
Aug 25, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 git#6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 git#8 0x55e075e2edb0 in do_push builtin/push.c:458 git#9 0x55e075e2ff7a in cmd_push builtin/push.c:702 git#10 0x55e075d8aaf0 in run_builtin git.c:452 git#11 0x55e075d8af08 in handle_builtin git.c:706 git#12 0x55e075d8b12c in run_argv git.c:770 git#13 0x55e075d8b6a0 in cmd_main git.c:905 git#14 0x55e075e81f07 in main common-main.c:60 git#15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 git#16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 git#17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com>
ttaylorr
added a commit
to ttaylorr/git
that referenced
this pull request
Aug 28, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 git#6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 git#8 0x55e075e2edb0 in do_push builtin/push.c:458 git#9 0x55e075e2ff7a in cmd_push builtin/push.c:702 git#10 0x55e075d8aaf0 in run_builtin git.c:452 git#11 0x55e075d8af08 in handle_builtin git.c:706 git#12 0x55e075d8b12c in run_argv git.c:770 git#13 0x55e075d8b6a0 in cmd_main git.c:905 git#14 0x55e075e81f07 in main common-main.c:60 git#15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 git#16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 git#17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com>
gitster
pushed a commit
that referenced
this pull request
Aug 29, 2023
When t5583-push-branches.sh was originally introduced via 425b4d7 (push: introduce '--branches' option, 2023-05-06), it was not leak-free. In fact, the test did not even run correctly until 022fbb6 (t5583: fix shebang line, 2023-05-12), but after applying that patch, we see a failure at t5583.8: ==2529087==ERROR: LeakSanitizer: detected memory leaks Direct leak of 384 byte(s) in 1 object(s) allocated from: #0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98 #1 0x55e07606cbf9 in xrealloc wrapper.c:140 #2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42 #3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917 #4 0x55e075fe9cce in add_missing_tags remote.c:1518 #5 0x55e075fea1e4 in match_push_refs remote.c:1665 #6 0x55e076050a8e in transport_push transport.c:1378 #7 0x55e075e2eb74 in push_with_options builtin/push.c:401 #8 0x55e075e2edb0 in do_push builtin/push.c:458 #9 0x55e075e2ff7a in cmd_push builtin/push.c:702 #10 0x55e075d8aaf0 in run_builtin git.c:452 #11 0x55e075d8af08 in handle_builtin git.c:706 #12 0x55e075d8b12c in run_argv git.c:770 #13 0x55e075d8b6a0 in cmd_main git.c:905 #14 0x55e075e81f07 in main common-main.c:60 #15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360 #17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453) SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s). This leak was addressed independently via 68b5117 (commit-reach: fix memory leak in get_reachable_subset(), 2023-06-03), which makes t5583 leak-free. But t5583 was not in the tree when 68b5117 was written, and the two only met after the latter was merged back in via 693bde4 (Merge branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20). At that point, t5583 was leak-free. Let's mark it as such accordingly. Signed-off-by: Taylor Blau <me@ttaylorr.com> Acked-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
vdye
pushed a commit
to vdye/git
that referenced
this pull request
Jan 9, 2024
When I was playing around with trace2 data and creating flamegraphs, I tried a `git fetch` call to see how the `git-remote-https` command would show up. What I didn't expect was an `ensure_full_index()` region! It turns out that `git fetch` and `git pull` need to check the index for a `.gitmodules` file to see if it should recurse into any submodules. Here is the stack trace from a debugger: ``` #0 ensure_full_index (istate=0x555555ac1c80 <the_index>) at sparse-index.c:404 #1 0x000055555571a979 in do_read_index (istate=istate@entry=0x555555ac1c80 <the_index>, path=path@entry=0x555555ad7b90 ".git/index", must_exist=must_exist@entry=0) at read-cache.c:2386 #2 0x000055555571eb7d in do_read_index (must_exist=0, path=0x555555ad7b90 ".git/index", istate=0x555555ac1c80 <the_index>) at hash.h:244 #3 read_index_from (istate=0x555555ac1c80 <the_index>, path=0x555555ad7b90 ".git/index", gitdir=0x555555ad7b30 ".git") at read-cache.c:2426 #4 0x000055555573f4c2 in repo_read_index (repo=repo@entry=0x555555ac1da0 <the_repo>) at repository.c:286 #5 0x00005555556f14d0 in get_oid_with_context_1 (repo=repo@entry=0x555555ac1da0 <the_repo>, name=name@entry=0x55555582c022 ":.gitmodules", flags=flags@entry=0, prefix=prefix@entry=0x0, oid=oid@entry=0x7fffffffdb00, oc=oc@entry=0x7fffffffda70) at object-name.c:1850 git#6 0x00005555556f1f53 in get_oid_with_context (oc=0x7fffffffda70, oid=0x7fffffffdb00, flags=0, str=0x55555582c022 ":.gitmodules", repo=0x555555ac1da0 <the_repo>) at object-name.c:1947 #7 repo_get_oid (r=r@entry=0x555555ac1da0 <the_repo>, name=name@entry=0x55555582c022 ":.gitmodules", oid=oid@entry=0x7fffffffdb00) at object-name.c:1603 git#8 0x000055555577330f in config_from_gitmodules (fn=fn@entry=0x555555773460 <gitmodules_fetch_config>, repo=0x555555ac1da0 <the_repo>, data=data@entry=0x7fffffffdb60) at submodule-config.c:650 git#9 0x000055555577462d in config_from_gitmodules (data=0x7fffffffdb60, repo=<optimized out>, fn=0x555555773460 <gitmodules_fetch_config>) at submodule-config.c:638 git#10 fetch_config_from_gitmodules (max_children=<optimized out>, recurse_submodules=<optimized out>) at submodule-config.c:800 git#11 0x00005555555b9e41 in cmd_fetch (argc=1, argv=0x7fffffffe090, prefix=0x0) at builtin/fetch.c:1999 git#12 0x0000555555573ff6 in run_builtin (argv=<optimized out>, argc=<optimized out>, p=<optimized out>) at git.c:528 git#13 handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:785 git#14 0x000055555557528c in run_argv (argv=0x7fffffffddf0, argcp=0x7fffffffddfc) at git.c:857 git#15 cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:993 git#16 0x0000555555573ac8 in main (argc=3, argv=0x7fffffffe088) at common-main.c:52 ``` The operations these commands use are guarded by items such as `index_name_pos()` and others. Since the `.gitmodules` file is always at root, we would not need to expand, anyway.
spectral54
added a commit
to spectral54/git
that referenced
this pull request
Jun 12, 2024
Memory sanitizer (msan) is detecting a use of an uninitialized variable
(`size`) in `read_attr_from_index`:
==2268==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x5651f3416504 in read_attr_from_index git/attr.c:868:11
#1 0x5651f3415530 in read_attr git/attr.c
#2 0x5651f3413d74 in bootstrap_attr_stack git/attr.c:968:6
#3 0x5651f3413d74 in prepare_attr_stack git/attr.c:1004:2
#4 0x5651f3413d74 in collect_some_attrs git/attr.c:1199:2
#5 0x5651f3413144 in git_check_attr git/attr.c:1345:2
git#6 0x5651f34728da in convert_attrs git/convert.c:1320:2
#7 0x5651f3473425 in would_convert_to_git_filter_fd git/convert.c:1373:2
git#8 0x5651f357a35e in index_fd git/object-file.c:2630:34
git#9 0x5651f357aa15 in index_path git/object-file.c:2657:7
git#10 0x5651f35db9d9 in add_to_index git/read-cache.c:766:7
git#11 0x5651f35dc170 in add_file_to_index git/read-cache.c:799:9
git#12 0x5651f321f9b2 in add_files git/builtin/add.c:346:7
git#13 0x5651f321f9b2 in cmd_add git/builtin/add.c:565:18
git#14 0x5651f321d327 in run_builtin git/git.c:474:11
git#15 0x5651f321bc9e in handle_builtin git/git.c:729:3
git#16 0x5651f321a792 in run_argv git/git.c:793:4
git#17 0x5651f321a792 in cmd_main git/git.c:928:19
git#18 0x5651f33dde1f in main git/common-main.c:62:11
The issue exists because `size` is an output parameter from
`read_blob_data_from_index`, but it's only modified if
`read_blob_data_from_index` returns non-NULL. The read of `size` when
calling `read_attr_from_buf` unconditionally may read from an
uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL
before reading from `size`, but by then it's already too late: the
uninitialized read will have happened already. Furthermore, there's no
guarantee that the compiler won't reorder things so that it checks
`size` before checking `!buf`.
Make the call to `read_attr_from_buf` conditional on `buf` being
non-NULL, ensuring that `size` is not read if it's never set.
Signed-off-by: Kyle Lippincott <spectral@google.com>
gitster
pushed a commit
that referenced
this pull request
Jun 17, 2024
Memory sanitizer (msan) is detecting a use of an uninitialized variable
(`size`) in `read_attr_from_index`:
==2268==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x5651f3416504 in read_attr_from_index git/attr.c:868:11
#1 0x5651f3415530 in read_attr git/attr.c
#2 0x5651f3413d74 in bootstrap_attr_stack git/attr.c:968:6
#3 0x5651f3413d74 in prepare_attr_stack git/attr.c:1004:2
#4 0x5651f3413d74 in collect_some_attrs git/attr.c:1199:2
#5 0x5651f3413144 in git_check_attr git/attr.c:1345:2
#6 0x5651f34728da in convert_attrs git/convert.c:1320:2
#7 0x5651f3473425 in would_convert_to_git_filter_fd git/convert.c:1373:2
#8 0x5651f357a35e in index_fd git/object-file.c:2630:34
#9 0x5651f357aa15 in index_path git/object-file.c:2657:7
#10 0x5651f35db9d9 in add_to_index git/read-cache.c:766:7
#11 0x5651f35dc170 in add_file_to_index git/read-cache.c:799:9
#12 0x5651f321f9b2 in add_files git/builtin/add.c:346:7
#13 0x5651f321f9b2 in cmd_add git/builtin/add.c:565:18
#14 0x5651f321d327 in run_builtin git/git.c:474:11
#15 0x5651f321bc9e in handle_builtin git/git.c:729:3
#16 0x5651f321a792 in run_argv git/git.c:793:4
#17 0x5651f321a792 in cmd_main git/git.c:928:19
#18 0x5651f33dde1f in main git/common-main.c:62:11
The issue exists because `size` is an output parameter from
`read_blob_data_from_index`, but it's only modified if
`read_blob_data_from_index` returns non-NULL. The read of `size` when
calling `read_attr_from_buf` unconditionally may read from an
uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL
before reading from `size`, but by then it's already too late: the
uninitialized read will have happened already. Furthermore, there's no
guarantee that the compiler won't reorder things so that it checks
`size` before checking `!buf`.
Make the call to `read_attr_from_buf` conditional on `buf` being
non-NULL, ensuring that `size` is not read if it's never set.
Signed-off-by: Kyle Lippincott <spectral@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitster
pushed a commit
that referenced
this pull request
Aug 19, 2024
It was recently reported that concurrent reads and writes may cause the
reftable backend to segfault. The root cause of this is that we do not
properly keep track of reftable readers across reloads.
Suppose that you have a reftable iterator and then decide to reload the
stack while iterating through the iterator. When the stack has been
rewritten since we have created the iterator, then we would end up
discarding a subset of readers that may still be in use by the iterator.
The consequence is that we now try to reference deallocated memory,
which of course segfaults.
One way to trigger this is in t5616, where some background maintenance
jobs have been leaking from one test into another. This leads to stack
traces like the following one:
+ git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin
AddressSanitizer:DEADLYSIGNAL
=================================================================
==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp
0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0)
==657994==The signal is caused by a READ memory access.
#0 0x55f23e52ddf9 in get_var_int reftable/record.c:29
#1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170
#2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194
#3 0x55f23e54e72e in block_iter_next reftable/block.c:398
#4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240
#5 0x55f23e5573dc in table_iter_next reftable/reader.c:355
#6 0x55f23e5573dc in table_iter_next reftable/reader.c:339
#7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69
#8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123
#9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172
#10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175
#11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464
#12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13
#13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452
#14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623
#15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659
#16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133
#17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432
#18 0x55f23dba7764 in run_builtin git.c:484
#19 0x55f23dba7764 in handle_builtin git.c:741
#20 0x55f23dbab61e in run_argv git.c:805
#21 0x55f23dbab61e in cmd_main git.c:1000
#22 0x55f23dba4781 in main common-main.c:64
#23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360
#25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27)
While it is somewhat awkward that the maintenance processes survive
tests in the first place, it is totally expected that reftables should
work alright with concurrent writers. Seemingly they don't.
The only underlying resource that we need to care about in this context
is the reftable reader, which is responsible for reading a single table
from disk. These readers get discarded immediately (unless reused) when
calling `reftable_stack_reload()`, which is wrong. We can only close
them once we know that there are no iterators using them anymore.
Prepare for a fix by converting the reftable readers to be refcounted.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitster
pushed a commit
that referenced
this pull request
Aug 22, 2024
It was recently reported that concurrent reads and writes may cause the
reftable backend to segfault. The root cause of this is that we do not
properly keep track of reftable readers across reloads.
Suppose that you have a reftable iterator and then decide to reload the
stack while iterating through the iterator. When the stack has been
rewritten since we have created the iterator, then we would end up
discarding a subset of readers that may still be in use by the iterator.
The consequence is that we now try to reference deallocated memory,
which of course segfaults.
One way to trigger this is in t5616, where some background maintenance
jobs have been leaking from one test into another. This leads to stack
traces like the following one:
+ git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin
AddressSanitizer:DEADLYSIGNAL
=================================================================
==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp
0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0)
==657994==The signal is caused by a READ memory access.
#0 0x55f23e52ddf9 in get_var_int reftable/record.c:29
#1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170
#2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194
#3 0x55f23e54e72e in block_iter_next reftable/block.c:398
#4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240
#5 0x55f23e5573dc in table_iter_next reftable/reader.c:355
#6 0x55f23e5573dc in table_iter_next reftable/reader.c:339
#7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69
#8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123
#9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172
#10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175
#11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464
#12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13
#13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452
#14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623
#15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659
#16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133
#17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432
#18 0x55f23dba7764 in run_builtin git.c:484
#19 0x55f23dba7764 in handle_builtin git.c:741
#20 0x55f23dbab61e in run_argv git.c:805
#21 0x55f23dbab61e in cmd_main git.c:1000
#22 0x55f23dba4781 in main common-main.c:64
#23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360
#25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27)
While it is somewhat awkward that the maintenance processes survive
tests in the first place, it is totally expected that reftables should
work alright with concurrent writers. Seemingly they don't.
The only underlying resource that we need to care about in this context
is the reftable reader, which is responsible for reading a single table
from disk. These readers get discarded immediately (unless reused) when
calling `reftable_stack_reload()`, which is wrong. We can only close
them once we know that there are no iterators using them anymore.
Prepare for a fix by converting the reftable readers to be refcounted.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitster
pushed a commit
that referenced
this pull request
Aug 23, 2024
It was recently reported that concurrent reads and writes may cause the
reftable backend to segfault. The root cause of this is that we do not
properly keep track of reftable readers across reloads.
Suppose that you have a reftable iterator and then decide to reload the
stack while iterating through the iterator. When the stack has been
rewritten since we have created the iterator, then we would end up
discarding a subset of readers that may still be in use by the iterator.
The consequence is that we now try to reference deallocated memory,
which of course segfaults.
One way to trigger this is in t5616, where some background maintenance
jobs have been leaking from one test into another. This leads to stack
traces like the following one:
+ git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin
AddressSanitizer:DEADLYSIGNAL
=================================================================
==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp
0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0)
==657994==The signal is caused by a READ memory access.
#0 0x55f23e52ddf9 in get_var_int reftable/record.c:29
#1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170
#2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194
#3 0x55f23e54e72e in block_iter_next reftable/block.c:398
#4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240
#5 0x55f23e5573dc in table_iter_next reftable/reader.c:355
#6 0x55f23e5573dc in table_iter_next reftable/reader.c:339
#7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69
#8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123
#9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172
#10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175
#11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464
#12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13
#13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452
#14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623
#15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659
#16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133
#17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432
#18 0x55f23dba7764 in run_builtin git.c:484
#19 0x55f23dba7764 in handle_builtin git.c:741
#20 0x55f23dbab61e in run_argv git.c:805
#21 0x55f23dbab61e in cmd_main git.c:1000
#22 0x55f23dba4781 in main common-main.c:64
#23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360
#25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27)
While it is somewhat awkward that the maintenance processes survive
tests in the first place, it is totally expected that reftables should
work alright with concurrent writers. Seemingly they don't.
The only underlying resource that we need to care about in this context
is the reftable reader, which is responsible for reading a single table
from disk. These readers get discarded immediately (unless reused) when
calling `reftable_stack_reload()`, which is wrong. We can only close
them once we know that there are no iterators using them anymore.
Prepare for a fix by converting the reftable readers to be refcounted.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi there!
This is WhitespaceBot from Gun.io. I'm an open-source robot that removes trailing white space in your code, and gives you a gitignore file if you didn't have one! I've only cleaned your most popular project, and I've added you to a list of users not to contact again, so you won't get any more pull requests from me unless you ask. If I'm misbehaving, please email my owner and tell him to turn me off!
== About Gun.io ==
Gun.io is a place for hackers to hire each other for small tasks. We offer no-hassle, winner-take-all freelance gigs, by hackers, for hackers. Got a bug you can't fix or a feature you want for your project? Post a gig and have somebody else sort it out for you. Oh, and it's free for open source! Sign up and get notified about new gigs you can work on!
== About WhitespaceBot==
WhitespaceBot is a simple open source robot which uses GitHub's API as a way of cleaning up open source projects! We've put up a paid bounty for whoever can add security fixing features to it.