[pull] master from git:master#210
Merged
Merged
Conversation
When running git-maintenance(1), we create a lockfile that is supposed to keep other maintenance processes from running at the same time. This lockfile is broken though in case the "--detach" flag is passed: the lockfile is created by the parent process and will be cleaned up either manually or on exit. But when detaching, the parent will exit before all of the background maintenance tasks have been run, and consequently the lock only covers a smaller part of the whole maintenance process. Fix this bug by reassigning all tempfiles from the parent process to the child process when daemonizing so that it becomes the responsibility of the child to clean them up. Note that this is a broader fix, as we now always reassign tempfiles when daemonizing. This is a natural consequence of the semantics of `daemonize()` though, as it essentially promises to continue running the current process in the background. It is thus sensible to have that function perform the whole dance of assigning resources to the child process, including tempfiles. There's only a single other caller in "daemon.c", but that process doesn't create any tempfiles before the call to `daemonize()` and is thus not impacted by this change. Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com> Helped-by: Jeff King <peff@peff.net> Helped-by: Derrick Stolee <stolee@gmail.com> Co-authored-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "gc.auto" configuration has traditionally been used to turn off running git-gc(1) as part of our auto-maintenance. We have eventually switched over to git-maintenance(1) in a95ce12 (maintenance: replace run_auto_gc(), 2020-09-17), and with 1942d48 (maintenance: optionally skip --auto process, 2020-08-28) we have introduced "maintenance.auto" to control whether or not to run auto-maintenance. At that point though we still shelled out to git-gc(1) internally. So if "gc.auto=0" was set we would still _execute_ git-maintenance(1), but the command would have exited fast because git-gc(1) itself knew to honor the config key. This has recently changed though, as we have adapted the default maintenance strategy to not use git-gc(1) anymore. The consequence is that "gc.auto=0" doesn't have an effect anymore, which is a somewhat surprising change in behaviour for our users. Adapt `run_auto_maintenance()` so that it knows to also read "gc.auto", similar to how it also reads both "maintenance.autoDetach" and "gc.autoDetach". Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In apply_patch(), we return immediately if read_patch_file() returns an error. Traditionally this was OK, since an error from strbuf_read() would restore the strbuf to its unallocated state. But since f1c0e39 (apply: reject patches larger than ~1 GiB, 2022-10-25), we may also return an error if we successfully read the patch but it is too large. In this case we leak the strbuf contents when apply_patch() returns. You can see it in action by running t4141 under LSan with the EXPENSIVE prereq enabled. We can fix this in one of two places: 1. In read_patch_file(), we could release the buffer before returning the error, behaving more like a raw strbuf_read() call. 2. In apply_patch(), we can release the strbuf ourselves before returning. I picked the latter, since it future proofs us against read_patch_file() getting new error modes. We also have a cleanup label in that function already, so now our error handling at this spot matches the rest of the function (and all of the variables are initialized such that the rest of the cleanup is correctly a noop at this point). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Running t4205 under UBSan with the EXPENSIVE prereq enabled triggers an
error when we try to create a commit message that is over 2GB:
commit.c:1574:6: runtime error: signed integer overflow:
-2147483648 - 1 cannot be represented in type 'int'
The problem is that find_invalid_utf8() is not prepared to handle
large buffers, as it uses an "int" to represent buffer sizes and
offsets.
We can fix this with a few changes:
1. We'll take in "len" as a size_t (which is what the caller has
anyway, since it's working with a strbuf).
2. We need to return a size_t to give the offset to the invalid utf8,
but we also need a sentinel value for "no invalid value"
(previously "-1"). Let's split these to return a bool for "found
invalid utf8" and then pass back the offset as an out-parameter.
We'll switch the function name to match the new semantics.
3. The caller in verify_utf8() uses a "long" to store buffer
positions, which is a bit funny. This goes back to 08a94a1
(commit/commit-tree: correct latin1 to utf-8, 2012-06-28) and is
perhaps trying to match our use of "unsigned long" for object sizes
(though we don't care about it ever becoming negative here). This
should be a size_t, too, as some platforms (like Windows) still use
a 32-bit long on machines with 64-bit pointers.
4. The "bytes" field within find_invalid_utf() does not have range
problems. It is the number of bytes the utf8 sequence claims to
have, so is limited by how many bits can be set in a single 8-bit
byte. However, if we leave it as an "int" then the compiler will
complain about the sign mismatch when comparing it to "len". So
let's make it unsigned, too.
All of this is a little silly, of course, because 2GB text commit
messages are clearly nonsense. So we might consider rejecting them
outright, but it is easy enough to make these helper functions more
robust in the meantime.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Leakfix. * jk/commit-sign-overflow-fix: commit: handle large commit messages in utf8 verification
Leakfix. * jk/apply-leakfix: apply: plug leak on "patch too large" error
"git maintenance" that goes background did not use the lockfile to prevent multiple maintenance processes from running at the same time, which has been corrected. * ps/maintenance-daemonize-lockfix: run-command: honor "gc.auto" for auto-maintenance builtin/maintenance: fix locking with "--detach"
With this batch, we have flushed all the topics that need to be merged to 'maint' to make its build healthy. Signed-off-by: Junio C Hamano <gitster@pobox.com>
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )