From c92a03948a889bba4bf9f218e88f9c520c515a7e Mon Sep 17 00:00:00 2001 From: Josh Heinrichs Date: Tue, 8 Oct 2024 08:29:20 -0600 Subject: [PATCH 01/10] git-config.1: remove value from positional args in unset usage The synopsis for `git config unset` mentions two positional arguments: `` and ``. While the first argument is correct, the second is not. Users are expected to provide the value via `--value=`. Remove the positional argument. The `--value=` option is already documented correctly, so this is all we need to do to fix the documentation. Signed-off-by: Josh Heinrichs Reviewed-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- Documentation/git-config.txt | 2 +- builtin/config.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 7f81fbbea83a6e..3e420177c1599d 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -12,7 +12,7 @@ SYNOPSIS 'git config list' [] [] [--includes] 'git config get' [] [] [--includes] [--all] [--regexp] [--value=] [--fixed-value] [--default=] 'git config set' [] [--type=] [--all] [--value=] [--fixed-value] -'git config unset' [] [--all] [--value=] [--fixed-value] +'git config unset' [] [--all] [--value=] [--fixed-value] 'git config rename-section' [] 'git config remove-section' [] 'git config edit' [] diff --git a/builtin/config.c b/builtin/config.c index d8fd3def0efdae..cba702210815b7 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -19,7 +19,7 @@ static const char *const builtin_config_usage[] = { N_("git config list [] [] [--includes]"), N_("git config get [] [] [--includes] [--all] [--regexp] [--value=] [--fixed-value] [--default=] "), N_("git config set [] [--type=] [--all] [--value=] [--fixed-value] "), - N_("git config unset [] [--all] [--value=] [--fixed-value] "), + N_("git config unset [] [--all] [--value=] [--fixed-value] "), N_("git config rename-section [] "), N_("git config remove-section [] "), N_("git config edit []"), @@ -43,7 +43,7 @@ static const char *const builtin_config_set_usage[] = { }; static const char *const builtin_config_unset_usage[] = { - N_("git config unset [] [--all] [--value=] [--fixed-value] "), + N_("git config unset [] [--all] [--value=] [--fixed-value] "), NULL }; From 422a4e37e2ac814dbccece9dea8b9c148dc93bb2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 7 Oct 2024 21:49:24 +0000 Subject: [PATCH 02/10] docs: fix the `maintain-git` links in `technical/platform-support` These links should point to `.html` files, not to `.txt` ones. Compare also to 4945f046c7f (api docs: link to html version of api-trace2, 2022-09-16). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- Documentation/technical/platform-support.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/technical/platform-support.txt b/Documentation/technical/platform-support.txt index a227c363d7824c..0a2fb28d6277f1 100644 --- a/Documentation/technical/platform-support.txt +++ b/Documentation/technical/platform-support.txt @@ -49,7 +49,7 @@ will be fixed in a later release: notice problems before they are considered "done with review"; whereas watching `master` means the stable branch could break for your platform, but you have a decent chance of avoiding a tagged release breaking you. See "The - Policy" in link:../howto/maintain-git.txt["How to maintain Git"] for an + Policy" in link:../howto/maintain-git.html["How to maintain Git"] for an overview of which branches are used in the Git project, and how. * The bug report should include information about what platform you are using. @@ -125,7 +125,7 @@ Compatible on `next` To avoid reactive debugging and fixing when changes hit a release or stable, you can aim to ensure `next` always works for your platform. (See "The Policy" in -link:../howto/maintain-git.txt["How to maintain Git"] for an overview of how +link:../howto/maintain-git.html["How to maintain Git"] for an overview of how `next` is used in the Git project.) To do that: * You should add a runner for your platform to the GitHub Actions or GitLab CI From 823223f2f0549a1505720cf2d3b215a483da255a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 8 Oct 2024 04:33:47 -0400 Subject: [PATCH 03/10] simple-ipc: split async server initialization and running To start an async ipc server, you call ipc_server_run_async(). That initializes the ipc_server_data object, and starts all of the threads running, which may immediately start serving clients. This can create some awkward timing problems, though. In the fsmonitor daemon (the sole user of the simple-ipc system), we want to create the ipc server early in the process, which means we may start serving clients before the rest of the daemon is fully initialized. To solve this, let's break run_async() into two parts: an initialization which allocates all data and spawns the threads (without letting them run), and a start function which actually lets them begin work. Since we have two simple-ipc implementations, we have to handle this twice: - in ipc-unix-socket.c, we have a central listener thread which hands connections off to worker threads using a work_available mutex. We can hold that mutex after init, and release it when we're ready to start. We do need an extra "started" flag so that we know whether the main thread is holding the mutex or not (e.g., if we prematurely stop the server, we want to make sure all of the worker threads are released to hear about the shutdown). - in ipc-win32.c, we don't have a central mutex. So we'll introduce a new startup_barrier mutex, which we'll similarly hold until we're ready to let the threads proceed. We again need a "started" flag here to make sure that we release the barrier mutex when shutting down, so that the sub-threads can proceed to the finish. I've renamed the run_async() function to init_async() to make sure we catch all callers, since they'll now need to call the matching start_async(). We could leave run_async() as a wrapper that does both, but there's not much point. There are only two callers, one of which is fsmonitor, which will want to actually do work between the two calls. And the other is just a test-tool wrapper. For now I've added the start_async() calls in fsmonitor where they would otherwise have happened, so there should be no behavior change with this patch. Signed-off-by: Jeff King Acked-by: Koji Nakamaru Signed-off-by: Junio C Hamano --- builtin/fsmonitor--daemon.c | 8 +++-- compat/simple-ipc/ipc-shared.c | 5 +-- compat/simple-ipc/ipc-unix-socket.c | 28 ++++++++++++++--- compat/simple-ipc/ipc-win32.c | 48 ++++++++++++++++++++++++++--- simple-ipc.h | 17 +++++++--- 5 files changed, 88 insertions(+), 18 deletions(-) diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index e1e6b96d09e9d8..4a077810d0e112 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -1208,13 +1208,15 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state) * system event listener thread so that we have the IPC handle * before we need it. */ - if (ipc_server_run_async(&state->ipc_server_data, - state->path_ipc.buf, &ipc_opts, - handle_client, state)) + if (ipc_server_init_async(&state->ipc_server_data, + state->path_ipc.buf, &ipc_opts, + handle_client, state)) return error_errno( _("could not start IPC thread pool on '%s'"), state->path_ipc.buf); + ipc_server_start_async(&state->ipc_server_data); + /* * Start the fsmonitor listener thread to collect filesystem * events. diff --git a/compat/simple-ipc/ipc-shared.c b/compat/simple-ipc/ipc-shared.c index cb176d966f287d..d1c21b49bdba8d 100644 --- a/compat/simple-ipc/ipc-shared.c +++ b/compat/simple-ipc/ipc-shared.c @@ -16,11 +16,12 @@ int ipc_server_run(const char *path, const struct ipc_server_opts *opts, struct ipc_server_data *server_data = NULL; int ret; - ret = ipc_server_run_async(&server_data, path, opts, - application_cb, application_data); + ret = ipc_server_init_async(&server_data, path, opts, + application_cb, application_data); if (ret) return ret; + ipc_server_start_async(server_data); ret = ipc_server_await(server_data); ipc_server_free(server_data); diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c index 9b3f2cdf8c9608..57d919c6b44932 100644 --- a/compat/simple-ipc/ipc-unix-socket.c +++ b/compat/simple-ipc/ipc-unix-socket.c @@ -328,6 +328,7 @@ struct ipc_server_data { int back_pos; int front_pos; + int started; int shutdown_requested; int is_stopped; }; @@ -824,10 +825,10 @@ static int setup_listener_socket( /* * Start IPC server in a pool of background threads. */ -int ipc_server_run_async(struct ipc_server_data **returned_server_data, - const char *path, const struct ipc_server_opts *opts, - ipc_server_application_cb *application_cb, - void *application_data) +int ipc_server_init_async(struct ipc_server_data **returned_server_data, + const char *path, const struct ipc_server_opts *opts, + ipc_server_application_cb *application_cb, + void *application_data) { struct unix_ss_socket *server_socket = NULL; struct ipc_server_data *server_data; @@ -888,6 +889,12 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data, server_data->accept_thread->fd_send_shutdown = sv[0]; server_data->accept_thread->fd_wait_shutdown = sv[1]; + /* + * Hold work-available mutex so that no work can start until + * we unlock it. + */ + pthread_mutex_lock(&server_data->work_available_mutex); + if (pthread_create(&server_data->accept_thread->pthread_id, NULL, accept_thread_proc, server_data->accept_thread)) die_errno(_("could not start accept_thread '%s'"), path); @@ -918,6 +925,15 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data, return 0; } +void ipc_server_start_async(struct ipc_server_data *server_data) +{ + if (!server_data || server_data->started) + return; + + server_data->started = 1; + pthread_mutex_unlock(&server_data->work_available_mutex); +} + /* * Gently tell the IPC server treads to shutdown. * Can be run on any thread. @@ -933,7 +949,9 @@ int ipc_server_stop_async(struct ipc_server_data *server_data) trace2_region_enter("ipc-server", "server-stop-async", NULL); - pthread_mutex_lock(&server_data->work_available_mutex); + /* If we haven't started yet, we are already holding lock. */ + if (server_data->started) + pthread_mutex_lock(&server_data->work_available_mutex); server_data->shutdown_requested = 1; diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c index 8bfe51248e552e..a8fc812adfcbd3 100644 --- a/compat/simple-ipc/ipc-win32.c +++ b/compat/simple-ipc/ipc-win32.c @@ -371,6 +371,9 @@ struct ipc_server_data { HANDLE hEventStopRequested; struct ipc_server_thread_data *thread_list; int is_stopped; + + pthread_mutex_t startup_barrier; + int started; }; enum connect_result { @@ -526,6 +529,16 @@ static int use_connection(struct ipc_server_thread_data *server_thread_data) return ret; } +static void wait_for_startup_barrier(struct ipc_server_data *server_data) +{ + /* + * Temporarily hold the startup_barrier mutex before starting, + * which lets us know that it's OK to start serving requests. + */ + pthread_mutex_lock(&server_data->startup_barrier); + pthread_mutex_unlock(&server_data->startup_barrier); +} + /* * Thread proc for an IPC server worker thread. It handles a series of * connections from clients. It cleans and reuses the hPipe between each @@ -550,6 +563,8 @@ static void *server_thread_proc(void *_server_thread_data) memset(&oConnect, 0, sizeof(oConnect)); oConnect.hEvent = hEventConnected; + wait_for_startup_barrier(server_thread_data->server_data); + for (;;) { cr = wait_for_connection(server_thread_data, &oConnect); @@ -752,10 +767,10 @@ static HANDLE create_new_pipe(wchar_t *wpath, int is_first) return hPipe; } -int ipc_server_run_async(struct ipc_server_data **returned_server_data, - const char *path, const struct ipc_server_opts *opts, - ipc_server_application_cb *application_cb, - void *application_data) +int ipc_server_init_async(struct ipc_server_data **returned_server_data, + const char *path, const struct ipc_server_opts *opts, + ipc_server_application_cb *application_cb, + void *application_data) { struct ipc_server_data *server_data; wchar_t wpath[MAX_PATH]; @@ -787,6 +802,13 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data, strbuf_addstr(&server_data->buf_path, path); wcscpy(server_data->wpath, wpath); + /* + * Hold the startup_barrier lock so that no threads will progress + * until ipc_server_start_async() is called. + */ + pthread_mutex_init(&server_data->startup_barrier, NULL); + pthread_mutex_lock(&server_data->startup_barrier); + if (nr_threads < 1) nr_threads = 1; @@ -837,6 +859,15 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data, return 0; } +void ipc_server_start_async(struct ipc_server_data *server_data) +{ + if (!server_data || server_data->started) + return; + + server_data->started = 1; + pthread_mutex_unlock(&server_data->startup_barrier); +} + int ipc_server_stop_async(struct ipc_server_data *server_data) { if (!server_data) @@ -850,6 +881,13 @@ int ipc_server_stop_async(struct ipc_server_data *server_data) * We DO NOT attempt to force them to drop an active connection. */ SetEvent(server_data->hEventStopRequested); + + /* + * If we haven't yet told the threads they are allowed to run, + * do so now, so they can receive the shutdown event. + */ + ipc_server_start_async(server_data); + return 0; } @@ -900,5 +938,7 @@ void ipc_server_free(struct ipc_server_data *server_data) free(std); } + pthread_mutex_destroy(&server_data->startup_barrier); + free(server_data); } diff --git a/simple-ipc.h b/simple-ipc.h index a849d9f8411fdb..3916eaf70d9863 100644 --- a/simple-ipc.h +++ b/simple-ipc.h @@ -179,11 +179,20 @@ struct ipc_server_opts * When a client IPC message is received, the `application_cb` will be * called (possibly on a random thread) to handle the message and * optionally compose a reply message. + * + * This initializes all threads but no actual work will be done until + * ipc_server_start_async() is called. + */ +int ipc_server_init_async(struct ipc_server_data **returned_server_data, + const char *path, const struct ipc_server_opts *opts, + ipc_server_application_cb *application_cb, + void *application_data); + +/* + * Let an async server start running. This needs to be called only once + * after initialization. */ -int ipc_server_run_async(struct ipc_server_data **returned_server_data, - const char *path, const struct ipc_server_opts *opts, - ipc_server_application_cb *application_cb, - void *application_data); +void ipc_server_start_async(struct ipc_server_data *server_data); /* * Gently signal the IPC server pool to shutdown. No new client From dacf46721fdc91189386a5acaf97f63f466e8ead Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 8 Oct 2024 04:36:13 -0400 Subject: [PATCH 04/10] fsmonitor: initialize fs event listener before accepting clients There's a racy hang in fsmonitor on macOS that we sometimes see in CI. When we serve a client, what's supposed to happen is: 1. The client thread calls with_lock__wait_for_cookie() in which we create a cookie file and then wait for a pthread_cond event 2. The filesystem event listener sees the cookie file creation, does some internal book-keeping, and then triggers the pthread_cond. But there's a problem: we start the listener that accepts client threads before we start the fs event thread. So it's possible for us to accept a client which creates the cookie file and starts waiting before the fs event thread is initialized, and we miss those filesystem events entirely. That leaves the client thread hanging forever. In CI, the symptom is that t9210 (which is testing scalar, which always enables fsmonitor under the hood) may hang forever in "scalar clone". It is waiting on "git fetch" which is waiting on the fsmonitor daemon. The race happens more frequently under load, but you can trigger it predictably with a sleep like this, which delays the start of the fs event thread: --- a/compat/fsmonitor/fsm-listen-darwin.c +++ b/compat/fsmonitor/fsm-listen-darwin.c @@ -510,6 +510,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state) FSEventStreamSetDispatchQueue(data->stream, data->dq); data->stream_scheduled = 1; + sleep(1); if (!FSEventStreamStart(data->stream)) { error(_("Failed to start the FSEventStream")); goto force_error_stop_without_loop; One solution might be to reverse the order of initialization: start the fs event thread before we start the thread listening for clients. But the fsmonitor code explicitly does it in the opposite direction. The fs event thread wants to refer to the ipc_server_data struct, so we need it to be initialized first. A further complication is that we need a signal from the fs event thread that it is actually ready and listening. And those details happen within backend-specific fsmonitor code, whereas the initialization is in the shared code. So instead, let's use the ipc_server init/start split added in the previous commit. The generic fsmonitor code will init the ipc_server but _not_ start it, leaving that to the backend specific code, which now needs to call ipc_server_start_async() at the right time. For macOS, that is right after we start the FSEventStream that you can see in the diff above. It's not clear to me if Windows suffers from the same problem (and we simply don't trigger it in CI), or if it is immune. Regardless, the obvious place to start accepting clients there is right after we've established the ReadDirectoryChanges watch. This makes the hangs go away in our macOS CI environment, even when compiled with the sleep() above. Helped-by: Koji Nakamaru Signed-off-by: Jeff King Acked-by: Koji Nakamaru Signed-off-by: Junio C Hamano --- builtin/fsmonitor--daemon.c | 2 -- compat/fsmonitor/fsm-listen-darwin.c | 6 ++++++ compat/fsmonitor/fsm-listen-win32.c | 6 ++++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index 4a077810d0e112..f3f6bd330b9ec9 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -1215,8 +1215,6 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state) _("could not start IPC thread pool on '%s'"), state->path_ipc.buf); - ipc_server_start_async(&state->ipc_server_data); - /* * Start the fsmonitor listener thread to collect filesystem * events. diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c index 2fc67442eb5e87..dfa551459d2d1b 100644 --- a/compat/fsmonitor/fsm-listen-darwin.c +++ b/compat/fsmonitor/fsm-listen-darwin.c @@ -516,6 +516,12 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state) } data->stream_started = 1; + /* + * Our fs event listener is now running, so it's safe to start + * serving client requests. + */ + ipc_server_start_async(state->ipc_server_data); + pthread_mutex_lock(&data->dq_lock); pthread_cond_wait(&data->dq_finished, &data->dq_lock); pthread_mutex_unlock(&data->dq_lock); diff --git a/compat/fsmonitor/fsm-listen-win32.c b/compat/fsmonitor/fsm-listen-win32.c index 98b73f63170b16..a3fb331a2c05bd 100644 --- a/compat/fsmonitor/fsm-listen-win32.c +++ b/compat/fsmonitor/fsm-listen-win32.c @@ -741,6 +741,12 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state) start_rdcw_watch(data->watch_gitdir) == -1) goto force_error_stop; + /* + * Now that we've established the rdcw watches, we can start + * serving clients. + */ + ipc_server_start_async(state->ipc_server_data); + for (;;) { dwWait = WaitForMultipleObjects(data->nr_listener_handles, data->hListener, From e8922bd79e30feff2b44ffefd868bb32caf32281 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 7 Oct 2024 06:38:15 +0200 Subject: [PATCH 05/10] cache-tree: refactor verification to return error codes The function `cache_tree_verify()` will `BUG()` whenever it finds that the cache-tree extension of the index is corrupt. The function is thus inherently untestable because the resulting call to `abort()` will be detected by our testing framework and labelled an error. And rightfully so: it shouldn't ever be possible to hit bugs, as they should indicate a programming error rather than corruption of on-disk state. Refactor the function to instead return error codes. This also ensures that the function can be used e.g. by git-fsck(1) without the whole process dying. Furthermore, this refactoring plugs some memory leaks when returning early by creating a common exit path. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- cache-tree.c | 97 +++++++++++++++++++++++++++++++++++--------------- cache-tree.h | 2 +- read-cache.c | 5 +-- unpack-trees.c | 10 ++++-- 4 files changed, 79 insertions(+), 35 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index b482167a69ae6a..a28863278d4b23 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -1,6 +1,7 @@ #define USE_THE_REPOSITORY_VARIABLE #include "git-compat-util.h" +#include "gettext.h" #include "hex.h" #include "lockfile.h" #include "tree.h" @@ -865,15 +866,15 @@ int cache_tree_matches_traversal(struct cache_tree *root, return 0; } -static void verify_one_sparse(struct index_state *istate, - struct strbuf *path, - int pos) +static int verify_one_sparse(struct index_state *istate, + struct strbuf *path, + int pos) { struct cache_entry *ce = istate->cache[pos]; - if (!S_ISSPARSEDIR(ce->ce_mode)) - BUG("directory '%s' is present in index, but not sparse", - path->buf); + return error(_("directory '%s' is present in index, but not sparse"), + path->buf); + return 0; } /* @@ -882,6 +883,7 @@ static void verify_one_sparse(struct index_state *istate, * 1 - Restart verification - a call to ensure_full_index() freed the cache * tree that is being verified and verification needs to be restarted from * the new toplevel cache tree. + * -1 - Verification failed. */ static int verify_one(struct repository *r, struct index_state *istate, @@ -891,18 +893,23 @@ static int verify_one(struct repository *r, int i, pos, len = path->len; struct strbuf tree_buf = STRBUF_INIT; struct object_id new_oid; + int ret; for (i = 0; i < it->subtree_nr; i++) { strbuf_addf(path, "%s/", it->down[i]->name); - if (verify_one(r, istate, it->down[i]->cache_tree, path)) - return 1; + ret = verify_one(r, istate, it->down[i]->cache_tree, path); + if (ret) + goto out; + strbuf_setlen(path, len); } if (it->entry_count < 0 || /* no verification on tests (t7003) that replace trees */ - lookup_replace_object(r, &it->oid) != &it->oid) - return 0; + lookup_replace_object(r, &it->oid) != &it->oid) { + ret = 0; + goto out; + } if (path->len) { /* @@ -912,12 +919,14 @@ static int verify_one(struct repository *r, */ int is_sparse = istate->sparse_index; pos = index_name_pos(istate, path->buf, path->len); - if (is_sparse && !istate->sparse_index) - return 1; + if (is_sparse && !istate->sparse_index) { + ret = 1; + goto out; + } if (pos >= 0) { - verify_one_sparse(istate, path, pos); - return 0; + ret = verify_one_sparse(istate, path, pos); + goto out; } pos = -pos - 1; @@ -935,16 +944,23 @@ static int verify_one(struct repository *r, unsigned mode; int entlen; - if (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE)) - BUG("%s with flags 0x%x should not be in cache-tree", - ce->name, ce->ce_flags); + if (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE)) { + ret = error(_("%s with flags 0x%x should not be in cache-tree"), + ce->name, ce->ce_flags); + goto out; + } + name = ce->name + path->len; slash = strchr(name, '/'); if (slash) { entlen = slash - name; + sub = find_subtree(it, ce->name + path->len, entlen, 0); - if (!sub || sub->cache_tree->entry_count < 0) - BUG("bad subtree '%.*s'", entlen, name); + if (!sub || sub->cache_tree->entry_count < 0) { + ret = error(_("bad subtree '%.*s'"), entlen, name); + goto out; + } + oid = &sub->cache_tree->oid; mode = S_IFDIR; i += sub->cache_tree->entry_count; @@ -957,27 +973,50 @@ static int verify_one(struct repository *r, strbuf_addf(&tree_buf, "%o %.*s%c", mode, entlen, name, '\0'); strbuf_add(&tree_buf, oid->hash, r->hash_algo->rawsz); } + hash_object_file(r->hash_algo, tree_buf.buf, tree_buf.len, OBJ_TREE, &new_oid); - if (!oideq(&new_oid, &it->oid)) - BUG("cache-tree for path %.*s does not match. " - "Expected %s got %s", len, path->buf, - oid_to_hex(&new_oid), oid_to_hex(&it->oid)); + + if (!oideq(&new_oid, &it->oid)) { + ret = error(_("cache-tree for path %.*s does not match. " + "Expected %s got %s"), len, path->buf, + oid_to_hex(&new_oid), oid_to_hex(&it->oid)); + goto out; + } + + ret = 0; +out: strbuf_setlen(path, len); strbuf_release(&tree_buf); - return 0; + return ret; } -void cache_tree_verify(struct repository *r, struct index_state *istate) +int cache_tree_verify(struct repository *r, struct index_state *istate) { struct strbuf path = STRBUF_INIT; + int ret; - if (!istate->cache_tree) - return; - if (verify_one(r, istate, istate->cache_tree, &path)) { + if (!istate->cache_tree) { + ret = 0; + goto out; + } + + ret = verify_one(r, istate, istate->cache_tree, &path); + if (ret < 0) + goto out; + if (ret > 0) { strbuf_reset(&path); - if (verify_one(r, istate, istate->cache_tree, &path)) + + ret = verify_one(r, istate, istate->cache_tree, &path); + if (ret < 0) + goto out; + if (ret > 0) BUG("ensure_full_index() called twice while verifying cache tree"); } + + ret = 0; + +out: strbuf_release(&path); + return ret; } diff --git a/cache-tree.h b/cache-tree.h index faae88be63c2ce..b82c4963e7c805 100644 --- a/cache-tree.h +++ b/cache-tree.h @@ -33,7 +33,7 @@ struct cache_tree *cache_tree_read(const char *buffer, unsigned long size); int cache_tree_fully_valid(struct cache_tree *); int cache_tree_update(struct index_state *, int); -void cache_tree_verify(struct repository *, struct index_state *); +int cache_tree_verify(struct repository *, struct index_state *); /* bitmasks to write_index_as_tree flags */ #define WRITE_TREE_MISSING_OK 1 diff --git a/read-cache.c b/read-cache.c index 3624f8c1d56371..1683c4be1a1ec5 100644 --- a/read-cache.c +++ b/read-cache.c @@ -3347,8 +3347,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, int new_shared_index, ret, test_split_index_env; struct split_index *si = istate->split_index; - if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0)) - cache_tree_verify(the_repository, istate); + if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0) && + cache_tree_verify(the_repository, istate) < 0) + return -1; if ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) { if (flags & COMMIT_LOCK) diff --git a/unpack-trees.c b/unpack-trees.c index 701eec295f9fde..eb6efe80bbbf3d 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2072,9 +2072,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options if (o->dst_index) { move_index_extensions(&o->internal.result, o->src_index); if (!ret) { - if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0)) - cache_tree_verify(the_repository, - &o->internal.result); + if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0) && + cache_tree_verify(the_repository, + &o->internal.result) < 0) { + ret = -1; + goto done; + } + if (!o->skip_cache_tree_update && !cache_tree_fully_valid(o->internal.result.cache_tree)) cache_tree_update(&o->internal.result, From ce5ee8d2b1354ea60b68a31c01dfba610304d561 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 7 Oct 2024 06:38:18 +0200 Subject: [PATCH 06/10] cache-tree: detect mismatching number of index entries In t4058 we have some tests that exercise git-read-tree(1) when used with a tree that contains duplicate entries. While the expectation is that we fail, we ideally should fail gracefully without a segfault. But that is not the case: we never check that the number of entries in the cache-tree is less than or equal to the number of entries in the index. This can lead to an out-of-bounds read as we unconditionally access `istate->cache[idx]`, where `idx` is controlled by the number of cache-tree entries and the current position therein. The result is a segfault. Fix this segfault by adding a sanity check for the number of index entries before dereferencing them. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- cache-tree.c | 5 +++++ t/t4058-diff-duplicates.sh | 12 ++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index a28863278d4b23..c595e8612044c7 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -934,6 +934,11 @@ static int verify_one(struct repository *r, pos = 0; } + if (it->entry_count + pos > istate->cache_nr) { + ret = error(_("corrupted cache-tree has entries not present in index")); + goto out; + } + i = 0; while (i < it->entry_count) { struct cache_entry *ce = istate->cache[pos + i]; diff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh index 2501c89c1c91ec..3f602adb055b20 100755 --- a/t/t4058-diff-duplicates.sh +++ b/t/t4058-diff-duplicates.sh @@ -132,15 +132,15 @@ test_expect_success 'create a few commits' ' rm commit_id up final ' -test_expect_failure 'git read-tree does not segfault' ' - test_when_finished rm .git/index.lock && - test_might_fail git read-tree --reset base +test_expect_success 'git read-tree does not segfault' ' + test_must_fail git read-tree --reset base 2>err && + test_grep "error: corrupted cache-tree has entries not present in index" err ' -test_expect_failure 'reset --hard does not segfault' ' - test_when_finished rm .git/index.lock && +test_expect_success 'reset --hard does not segfault' ' git checkout base && - test_might_fail git reset --hard + test_must_fail git reset --hard 2>err && + test_grep "error: corrupted cache-tree has entries not present in index" err ' test_expect_failure 'git diff HEAD does not segfault' ' From 7959bb658d8e0fc852973b13bf32c4afb8a6fc92 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 7 Oct 2024 06:38:21 +0200 Subject: [PATCH 07/10] unpack-trees: detect mismatching number of cache-tree/index entries Same as the preceding commit, we unconditionally dereference the index's cache entries depending on the number of cache-tree entries, which can lead to a segfault when the cache-tree is corrupted. Fix this bug. This also makes t4058 pass with the leak sanitizer enabled. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/t4058-diff-duplicates.sh | 7 +++++-- unpack-trees.c | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh index 3f602adb055b20..18e5ac88c31174 100755 --- a/t/t4058-diff-duplicates.sh +++ b/t/t4058-diff-duplicates.sh @@ -10,6 +10,8 @@ # that the diff output isn't wildly unreasonable. test_description='test tree diff when trees have duplicate entries' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # make_tree_entry @@ -143,11 +145,12 @@ test_expect_success 'reset --hard does not segfault' ' test_grep "error: corrupted cache-tree has entries not present in index" err ' -test_expect_failure 'git diff HEAD does not segfault' ' +test_expect_success 'git diff HEAD does not segfault' ' git checkout base && GIT_TEST_CHECK_CACHE_TREE=false && git reset --hard && - test_might_fail git diff HEAD + test_must_fail git diff HEAD 2>err && + test_grep "error: corrupted cache-tree has entries not present in index" err ' test_expect_failure 'can switch to another branch when status is empty' ' diff --git a/unpack-trees.c b/unpack-trees.c index eb6efe80bbbf3d..2c08aad168b847 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -808,6 +808,8 @@ static int traverse_by_cache_tree(int pos, int nr_entries, int nr_names, if (!o->merge) BUG("We need cache-tree to do this optimization"); + if (nr_entries + pos > o->src_index->cache_nr) + return error(_("corrupted cache-tree has entries not present in index")); /* * Do what unpack_callback() and unpack_single_entry() normally From 9abda5e7e968cc8141edf8e7bb070cb397e54862 Mon Sep 17 00:00:00 2001 From: Philippe Blain Date: Fri, 11 Oct 2024 17:29:47 +0000 Subject: [PATCH 08/10] Makefile: fix dependency for $(UNIT_TEST_DIR)/clar/clar.o The clar source file '$(UNIT_TEST_DIR)/clar/clar.c' includes the generated 'clar.suite', but this dependency is not taken into account by our Makefile, so that it is possible for a parallel build to fail if Make tries to build 'clar.o' before 'clar.suite' is generated. Correctly specify the dependency. Signed-off-by: Philippe Blain Signed-off-by: Junio C Hamano --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index aea9b14eb8b079..161cb2b6f978f1 100644 --- a/Makefile +++ b/Makefile @@ -3975,6 +3975,7 @@ $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUI done >$@ $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h $(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite +$(UNIT_TEST_DIR)/clar/clar.o: $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h $(CLAR_TEST_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GITLIBS) GIT-LDFLAGS From 6e0b224a728a4dbd548efd6e8297c1010cce8964 Mon Sep 17 00:00:00 2001 From: Daniel Black Date: Wed, 9 Oct 2024 14:32:54 +1100 Subject: [PATCH 09/10] submodule: correct remote name with fetch The code fetches the submodules remote based on the superproject remote name instead of the submodule remote name[1]. Instead of grabbing the default remote of the superproject repository, ask the default remote of the submodule we are going to run 'git fetch' in. 1. https://lore.kernel.org/git/ZJR5SPDj4Wt_gmRO@pweza/ Signed-off-by: Daniel Black Signed-off-by: Junio C Hamano --- builtin/submodule--helper.c | 9 ++++++++- t/t5572-pull-submodule.sh | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index dc89488a7da335..b6b5f1ebde7c2e 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2333,7 +2333,14 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, strvec_pushf(&cp.args, "--depth=%d", depth); if (oid) { char *hex = oid_to_hex(oid); - char *remote = get_default_remote(); + char *remote; + int code; + + code = get_default_remote_submodule(module_path, &remote); + if (code) { + child_process_clear(&cp); + return code; + } strvec_pushl(&cp.args, remote, hex, NULL); free(remote); diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh index 916e58c16696ba..9b6cf8d88b9fbc 100755 --- a/t/t5572-pull-submodule.sh +++ b/t/t5572-pull-submodule.sh @@ -230,6 +230,7 @@ test_expect_success 'branch has no merge base with remote-tracking counterpart' test_create_repo a-submodule && test_commit -C a-submodule foo && + test_commit -C a-submodule bar && test_create_repo parent && git -C parent submodule add "$(pwd)/a-submodule" && @@ -246,4 +247,23 @@ test_expect_success 'branch has no merge base with remote-tracking counterpart' git -C child pull --recurse-submodules --rebase ' +test_expect_success 'fetch submodule remote of different name from superproject' ' + git -C child remote rename origin o1 && + git -C child submodule update --init && + + # Needs to create unreachable commit from current master branch. + git -C a-submodule checkout -b newmain HEAD^ && + test_commit -C a-submodule echo && + test_commit -C a-submodule moreecho && + subc=$(git -C a-submodule rev-parse --short HEAD) && + + git -C parent/a-submodule fetch && + git -C parent/a-submodule checkout "$subc" && + git -C parent commit -m "update submodule" a-submodule && + git -C a-submodule reset --hard HEAD^^ && + + git -C child pull --no-recurse-submodules && + git -C child submodule update +' + test_done From a7a3ceae011ca4a6321ae837e082c9429de24bc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6ren=20Krecker?= Date: Thu, 17 Oct 2024 19:18:20 +0200 Subject: [PATCH 10/10] mingw.c: Fix complier warnings for a 64 bit msvc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove some complier warnings from msvc in compat/mingw.c for value truncation from 64 bit to 32 bit integers. Compiling compat/mingw.c under a 64 bit version of msvc produces warnings. An "int" is 32 bit, and ssize_t or size_t should be 64 bit long. Prepare compat/vcbuild/include/unistd.h to have a 64 bit type _ssize_t, when _WIN64 is defined and 32 bit otherwise. Further down in this include file, as before, ssize_t is defined as _ssize_t, if needed. Use size_t instead of int for all variables that hold the result of strlen() or wcslen() (which cannot be negative). Use ssize_t to hold the return value of read(). Signed-off-by: Sören Krecker Signed-off-by: Taylor Blau --- compat/compiler.h | 4 ++-- compat/mingw.c | 23 ++++++++++++++--------- compat/vcbuild/include/unistd.h | 4 ++++ 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/compat/compiler.h b/compat/compiler.h index e9ad9db84f2279..e12e426404ab0c 100644 --- a/compat/compiler.h +++ b/compat/compiler.h @@ -9,7 +9,7 @@ static inline void get_compiler_info(struct strbuf *info) { - int len = info->len; + size_t len = info->len; #ifdef __clang__ strbuf_addf(info, "clang: %s\n", __clang_version__); #elif defined(__GNUC__) @@ -27,7 +27,7 @@ static inline void get_compiler_info(struct strbuf *info) static inline void get_libc_info(struct strbuf *info) { - int len = info->len; + size_t len = info->len; #ifdef __GLIBC__ strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); diff --git a/compat/mingw.c b/compat/mingw.c index c1030e40f227d2..be0c7a7a24621e 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1049,7 +1049,7 @@ int mingw_chmod(const char *filename, int mode) */ static int has_valid_directory_prefix(wchar_t *wfilename) { - int n = wcslen(wfilename); + size_t n = wcslen(wfilename); while (n > 0) { wchar_t c = wfilename[--n]; @@ -1628,7 +1628,8 @@ static const char *parse_interpreter(const char *cmd) { static char buf[MAX_PATH]; char *p, *opt; - int n, fd; + ssize_t n; /* read() can return negative values */ + int fd; /* don't even try a .exe */ n = strlen(cmd); @@ -1752,7 +1753,7 @@ static char *path_lookup(const char *cmd, int exe_only) { const char *path; char *prog = NULL; - int len = strlen(cmd); + size_t len = strlen(cmd); int isexe = len >= 4 && !strcasecmp(cmd+len-4, ".exe"); if (strpbrk(cmd, "/\\")) @@ -2389,7 +2390,7 @@ char *mingw_getenv(const char *name) #define GETENV_MAX_RETAIN 64 static char *values[GETENV_MAX_RETAIN]; static int value_counter; - int len_key, len_value; + size_t len_key, len_value; wchar_t *w_key; char *value; wchar_t w_value[32768]; @@ -2401,7 +2402,8 @@ char *mingw_getenv(const char *name) /* We cannot use xcalloc() here because that uses getenv() itself */ w_key = calloc(len_key, sizeof(wchar_t)); if (!w_key) - die("Out of memory, (tried to allocate %u wchar_t's)", len_key); + die("Out of memory, (tried to allocate %"PRIuMAX" wchar_t's)", + (uintmax_t)len_key); xutftowcs(w_key, name, len_key); /* GetEnvironmentVariableW() only sets the last error upon failure */ SetLastError(ERROR_SUCCESS); @@ -2416,7 +2418,8 @@ char *mingw_getenv(const char *name) /* We cannot use xcalloc() here because that uses getenv() itself */ value = calloc(len_value, sizeof(char)); if (!value) - die("Out of memory, (tried to allocate %u bytes)", len_value); + die("Out of memory, (tried to allocate %"PRIuMAX" bytes)", + (uintmax_t)len_value); xwcstoutf(value, w_value, len_value); /* @@ -2434,7 +2437,7 @@ char *mingw_getenv(const char *name) int mingw_putenv(const char *namevalue) { - int size; + size_t size; wchar_t *wide, *equal; BOOL result; @@ -2444,7 +2447,8 @@ int mingw_putenv(const char *namevalue) size = strlen(namevalue) * 2 + 1; wide = calloc(size, sizeof(wchar_t)); if (!wide) - die("Out of memory, (tried to allocate %u wchar_t's)", size); + die("Out of memory, (tried to allocate %" PRIuMAX " wchar_t's)", + (uintmax_t)size); xutftowcs(wide, namevalue, size); equal = wcschr(wide, L'='); if (!equal) @@ -4072,7 +4076,8 @@ static BOOL WINAPI handle_ctrl_c(DWORD ctrl_type) */ int wmain(int argc, const wchar_t **wargv) { - int i, maxlen, exit_status; + int i, exit_status; + size_t maxlen; char *buffer, **save; const char **argv; diff --git a/compat/vcbuild/include/unistd.h b/compat/vcbuild/include/unistd.h index 3a959d124ca794..a261a925b7f850 100644 --- a/compat/vcbuild/include/unistd.h +++ b/compat/vcbuild/include/unistd.h @@ -14,7 +14,11 @@ typedef _mode_t mode_t; #ifndef _SSIZE_T_ #define _SSIZE_T_ +#ifdef _WIN64 +typedef __int64 _ssize_t; +#else typedef long _ssize_t; +#endif /* _WIN64 */ #ifndef _OFF_T_ #define _OFF_T_