From 767b1e74fc70d78c7e3c84a2dc5eb7b607252f4f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 27 Oct 2022 21:18:23 +0200 Subject: [PATCH 01/11] commit: simplify code The difference of two unsigned integers is defined to be unsigned, and therefore it is misleading to check whether it is greater than zero (instead, the more natural way would be to check whether the difference is zero or not). Let's instead avoid the subtraction altogether, and compare the two operands directly, which makes the code more obvious as a side effect. Pointed out by CodeQL's rule with the ID `cpp/unsigned-difference-expression-compared-zero`. Signed-off-by: Johannes Schindelin --- builtin/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/commit.c b/builtin/commit.c index 66bd91fd523dd7..fba0dded64a718 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1022,7 +1022,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, for (i = 0; i < the_repository->index->cache_nr; i++) if (ce_intent_to_add(the_repository->index->cache[i])) ita_nr++; - committable = the_repository->index->cache_nr - ita_nr > 0; + committable = the_repository->index->cache_nr > ita_nr; } else { /* * Unless the user did explicitly request a submodule From c66eaee64ad5484120f1905ce5501fd8b75b2266 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 14 Dec 2022 22:04:59 +0100 Subject: [PATCH 02/11] fetch: carefully clear local variable's address after use As pointed out by CodeQL, it is a potentially dangerous practice to store local variables' addresses in non-local structs. Yet this is exactly what happens with the `acked_commits` attribute that is used in `cmd_fetch()`: The pointer to a local variable is assigned to it. Now, it is Git's convention that `cmd_*()` functions are essentially only returning just before exiting the process, therefore there is little danger that this attribute is used after the code flow returns from that function. However, code in `cmd_*()` function is often so useful that it gets lifted into a library function, at which point this issue could become a real problem. Let's make sure to clear the `acked_commits` attribute out after it was used, and before the function returns (at which point the address would go stale). Signed-off-by: Johannes Schindelin --- builtin/fetch.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/fetch.c b/builtin/fetch.c index cda6eaf1fd6edc..c1a1434c709625 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2560,6 +2560,7 @@ int cmd_fetch(int argc, if (server_options.nr) gtransport->server_options = &server_options; result = transport_fetch_refs(gtransport, NULL); + gtransport->smart_options->acked_commits = NULL; oidset_iter_init(&acked_commits, &iter); while ((oid = oidset_iter_next(&iter))) From 5a3a8880a68f8c69c2af39d2e32ebb56eb5fa483 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 14 Dec 2022 22:53:56 +0100 Subject: [PATCH 03/11] commit-graph: avoid malloc'ing a local variable We do need a context to write the commit graph, but that context is only needed during the life time of `commit_graph_write()`, therefore it can easily be a stack variable. This also helps CodeQL recognize that it is safe to assign the address of other local variables to the context's fields. Signed-off-by: Johannes Schindelin --- commit-graph.c | 141 ++++++++++++++++++++++++------------------------- 1 file changed, 69 insertions(+), 72 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 6394752b0b0868..9f0115dac9b528 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2509,7 +2509,17 @@ int write_commit_graph(struct object_directory *odb, const struct commit_graph_opts *opts) { struct repository *r = the_repository; - struct write_commit_graph_context *ctx; + struct write_commit_graph_context ctx = { + .r = r, + .odb = odb, + .append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0, + .report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0, + .split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0, + .opts = opts, + .total_bloom_filter_data_size = 0, + .write_generation_data = (get_configured_generation_version(r) == 2), + .num_generation_data_overflows = 0, + }; uint32_t i; int res = 0; int replace = 0; @@ -2531,17 +2541,6 @@ int write_commit_graph(struct object_directory *odb, return 0; } - CALLOC_ARRAY(ctx, 1); - ctx->r = r; - ctx->odb = odb; - ctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0; - ctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0; - ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0; - ctx->opts = opts; - ctx->total_bloom_filter_data_size = 0; - ctx->write_generation_data = (get_configured_generation_version(r) == 2); - ctx->num_generation_data_overflows = 0; - bloom_settings.hash_version = r->settings.commit_graph_changed_paths_version; bloom_settings.bits_per_entry = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY", bloom_settings.bits_per_entry); @@ -2549,14 +2548,14 @@ int write_commit_graph(struct object_directory *odb, bloom_settings.num_hashes); bloom_settings.max_changed_paths = git_env_ulong("GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS", bloom_settings.max_changed_paths); - ctx->bloom_settings = &bloom_settings; + ctx.bloom_settings = &bloom_settings; init_topo_level_slab(&topo_levels); - ctx->topo_levels = &topo_levels; + ctx.topo_levels = &topo_levels; - prepare_commit_graph(ctx->r); - if (ctx->r->objects->commit_graph) { - struct commit_graph *g = ctx->r->objects->commit_graph; + prepare_commit_graph(ctx.r); + if (ctx.r->objects->commit_graph) { + struct commit_graph *g = ctx.r->objects->commit_graph; while (g) { g->topo_levels = &topo_levels; @@ -2565,15 +2564,15 @@ int write_commit_graph(struct object_directory *odb, } if (flags & COMMIT_GRAPH_WRITE_BLOOM_FILTERS) - ctx->changed_paths = 1; + ctx.changed_paths = 1; if (!(flags & COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS)) { struct commit_graph *g; - g = ctx->r->objects->commit_graph; + g = ctx.r->objects->commit_graph; /* We have changed-paths already. Keep them in the next graph */ if (g && g->bloom_filter_settings) { - ctx->changed_paths = 1; + ctx.changed_paths = 1; /* don't propagate the hash_version unless unspecified */ if (bloom_settings.hash_version == -1) @@ -2586,116 +2585,114 @@ int write_commit_graph(struct object_directory *odb, bloom_settings.hash_version = bloom_settings.hash_version == 2 ? 2 : 1; - if (ctx->split) { - struct commit_graph *g = ctx->r->objects->commit_graph; + if (ctx.split) { + struct commit_graph *g = ctx.r->objects->commit_graph; while (g) { - ctx->num_commit_graphs_before++; + ctx.num_commit_graphs_before++; g = g->base_graph; } - if (ctx->num_commit_graphs_before) { - ALLOC_ARRAY(ctx->commit_graph_filenames_before, ctx->num_commit_graphs_before); - i = ctx->num_commit_graphs_before; - g = ctx->r->objects->commit_graph; + if (ctx.num_commit_graphs_before) { + ALLOC_ARRAY(ctx.commit_graph_filenames_before, ctx.num_commit_graphs_before); + i = ctx.num_commit_graphs_before; + g = ctx.r->objects->commit_graph; while (g) { - ctx->commit_graph_filenames_before[--i] = xstrdup(g->filename); + ctx.commit_graph_filenames_before[--i] = xstrdup(g->filename); g = g->base_graph; } } - if (ctx->opts) - replace = ctx->opts->split_flags & COMMIT_GRAPH_SPLIT_REPLACE; + if (ctx.opts) + replace = ctx.opts->split_flags & COMMIT_GRAPH_SPLIT_REPLACE; } - ctx->approx_nr_objects = repo_approximate_object_count(the_repository); + ctx.approx_nr_objects = repo_approximate_object_count(the_repository); - if (ctx->append && ctx->r->objects->commit_graph) { - struct commit_graph *g = ctx->r->objects->commit_graph; + if (ctx.append && ctx.r->objects->commit_graph) { + struct commit_graph *g = ctx.r->objects->commit_graph; for (i = 0; i < g->num_commits; i++) { struct object_id oid; oidread(&oid, g->chunk_oid_lookup + st_mult(g->hash_len, i), the_repository->hash_algo); - oid_array_append(&ctx->oids, &oid); + oid_array_append(&ctx.oids, &oid); } } if (pack_indexes) { - ctx->order_by_pack = 1; - if ((res = fill_oids_from_packs(ctx, pack_indexes))) + ctx.order_by_pack = 1; + if ((res = fill_oids_from_packs(&ctx, pack_indexes))) goto cleanup; } if (commits) { - if ((res = fill_oids_from_commits(ctx, commits))) + if ((res = fill_oids_from_commits(&ctx, commits))) goto cleanup; } if (!pack_indexes && !commits) { - ctx->order_by_pack = 1; - fill_oids_from_all_packs(ctx); + ctx.order_by_pack = 1; + fill_oids_from_all_packs(&ctx); } - close_reachable(ctx); + close_reachable(&ctx); - copy_oids_to_commits(ctx); + copy_oids_to_commits(&ctx); - if (ctx->commits.nr >= GRAPH_EDGE_LAST_MASK) { + if (ctx.commits.nr >= GRAPH_EDGE_LAST_MASK) { error(_("too many commits to write graph")); res = -1; goto cleanup; } - if (!ctx->commits.nr && !replace) + if (!ctx.commits.nr && !replace) goto cleanup; - if (ctx->split) { - split_graph_merge_strategy(ctx); + if (ctx.split) { + split_graph_merge_strategy(&ctx); if (!replace) - merge_commit_graphs(ctx); + merge_commit_graphs(&ctx); } else - ctx->num_commit_graphs_after = 1; + ctx.num_commit_graphs_after = 1; - ctx->trust_generation_numbers = validate_mixed_generation_chain(ctx->r->objects->commit_graph); + ctx.trust_generation_numbers = validate_mixed_generation_chain(ctx.r->objects->commit_graph); - compute_topological_levels(ctx); - if (ctx->write_generation_data) - compute_generation_numbers(ctx); + compute_topological_levels(&ctx); + if (ctx.write_generation_data) + compute_generation_numbers(&ctx); - if (ctx->changed_paths) - compute_bloom_filters(ctx); + if (ctx.changed_paths) + compute_bloom_filters(&ctx); - res = write_commit_graph_file(ctx); + res = write_commit_graph_file(&ctx); - if (ctx->changed_paths) + if (ctx.changed_paths) deinit_bloom_filters(); - if (ctx->split) - mark_commit_graphs(ctx); + if (ctx.split) + mark_commit_graphs(&ctx); - expire_commit_graphs(ctx); + expire_commit_graphs(&ctx); cleanup: - free(ctx->graph_name); - free(ctx->base_graph_name); - free(ctx->commits.list); - oid_array_clear(&ctx->oids); + free(ctx.graph_name); + free(ctx.base_graph_name); + free(ctx.commits.list); + oid_array_clear(&ctx.oids); clear_topo_level_slab(&topo_levels); - for (i = 0; i < ctx->num_commit_graphs_before; i++) - free(ctx->commit_graph_filenames_before[i]); - free(ctx->commit_graph_filenames_before); + for (i = 0; i < ctx.num_commit_graphs_before; i++) + free(ctx.commit_graph_filenames_before[i]); + free(ctx.commit_graph_filenames_before); - for (i = 0; i < ctx->num_commit_graphs_after; i++) { - free(ctx->commit_graph_filenames_after[i]); - free(ctx->commit_graph_hash_after[i]); + for (i = 0; i < ctx.num_commit_graphs_after; i++) { + free(ctx.commit_graph_filenames_after[i]); + free(ctx.commit_graph_hash_after[i]); } - free(ctx->commit_graph_filenames_after); - free(ctx->commit_graph_hash_after); - - free(ctx); + free(ctx.commit_graph_filenames_after); + free(ctx.commit_graph_hash_after); return res; } From 8d712a0ebc8d5161bdc80bdd7f4d05a967c66a09 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 14 Dec 2022 23:49:33 +0100 Subject: [PATCH 04/11] upload-pack: rename `enum` to reflect the operation While 3145ea957d (upload-pack: introduce fetch server command, 2018-03-15) added support for the `fetch` command, from the server's point of view it is an upload, and hence the `enum` should really be called `upload_state` instead of `fetch_state`. Likewise, rename its values. This also helps unconfuse CodeQL which would otherwise be at sixes or sevens about having _two_ non-local definitions of the same `enum` with the same values. Signed-off-by: Johannes Schindelin --- upload-pack.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/upload-pack.c b/upload-pack.c index 956da5b061a0e5..26f29b85b551c1 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1780,16 +1780,16 @@ static void send_shallow_info(struct upload_pack_data *data) packet_delim(1); } -enum fetch_state { - FETCH_PROCESS_ARGS = 0, - FETCH_SEND_ACKS, - FETCH_SEND_PACK, - FETCH_DONE, +enum upload_state { + UPLOAD_PROCESS_ARGS = 0, + UPLOAD_SEND_ACKS, + UPLOAD_SEND_PACK, + UPLOAD_DONE, }; int upload_pack_v2(struct repository *r, struct packet_reader *request) { - enum fetch_state state = FETCH_PROCESS_ARGS; + enum upload_state state = UPLOAD_PROCESS_ARGS; struct upload_pack_data data; clear_object_flags(the_repository, ALL_FLAGS); @@ -1798,9 +1798,9 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) data.use_sideband = LARGE_PACKET_MAX; get_upload_pack_config(r, &data); - while (state != FETCH_DONE) { + while (state != UPLOAD_DONE) { switch (state) { - case FETCH_PROCESS_ARGS: + case UPLOAD_PROCESS_ARGS: process_args(request, &data); if (!data.want_obj.nr && !data.wait_for_done) { @@ -1811,27 +1811,27 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) * to just send 'have's without 'want's); guess * they didn't want anything. */ - state = FETCH_DONE; + state = UPLOAD_DONE; } else if (data.seen_haves) { /* * Request had 'have' lines, so lets ACK them. */ - state = FETCH_SEND_ACKS; + state = UPLOAD_SEND_ACKS; } else { /* * Request had 'want's but no 'have's so we can * immediately go to construct and send a pack. */ - state = FETCH_SEND_PACK; + state = UPLOAD_SEND_PACK; } break; - case FETCH_SEND_ACKS: + case UPLOAD_SEND_ACKS: if (process_haves_and_send_acks(&data)) - state = FETCH_SEND_PACK; + state = UPLOAD_SEND_PACK; else - state = FETCH_DONE; + state = UPLOAD_DONE; break; - case FETCH_SEND_PACK: + case UPLOAD_SEND_PACK: send_wanted_ref_info(&data); send_shallow_info(&data); @@ -1841,9 +1841,9 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request) packet_writer_write(&data.writer, "packfile\n"); create_pack_file(&data, NULL); } - state = FETCH_DONE; + state = UPLOAD_DONE; break; - case FETCH_DONE: + case UPLOAD_DONE: continue; } } From 80422a5770ded04993c73c657b363ddad45e2f4a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 22:41:00 +0100 Subject: [PATCH 05/11] has_dir_name(): make code more obvious One thing that might be non-obvious to readers (or to analyzers like CodeQL) is that the function essentially does nothing when the Git index is empty, and in particular that it does not look at the value of `len_eq_last` (which would be uninitialized at that point). Let's make this much easier to understand, by returning early if the Git index is empty, and by avoiding empty `else` blocks. This commit changes indentation and is hence best viewed using `--ignore-space-change`. Signed-off-by: Johannes Schindelin --- read-cache.c | 55 +++++++++++++--------------------------------------- 1 file changed, 13 insertions(+), 42 deletions(-) diff --git a/read-cache.c b/read-cache.c index 73f83a7e7a113e..c0bb760ad473ef 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1117,48 +1117,19 @@ static int has_dir_name(struct index_state *istate, * * Compare the entry's full path with the last path in the index. */ - if (istate->cache_nr > 0) { - cmp_last = strcmp_offset(name, - istate->cache[istate->cache_nr - 1]->name, - &len_eq_last); - if (cmp_last > 0) { - if (name[len_eq_last] != '/') { - /* - * The entry sorts AFTER the last one in the - * index. - * - * If there were a conflict with "file", then our - * name would start with "file/" and the last index - * entry would start with "file" but not "file/". - * - * The next character after common prefix is - * not '/', so there can be no conflict. - */ - return retval; - } else { - /* - * The entry sorts AFTER the last one in the - * index, and the next character after common - * prefix is '/'. - * - * Either the last index entry is a file in - * conflict with this entry, or it has a name - * which sorts between this entry and the - * potential conflicting file. - * - * In both cases, we fall through to the loop - * below and let the regular search code handle it. - */ - } - } else if (cmp_last == 0) { - /* - * The entry exactly matches the last one in the - * index, but because of multiple stage and CE_REMOVE - * items, we fall through and let the regular search - * code handle it. - */ - } - } + if (!istate->cache_nr) + return 0; + + cmp_last = strcmp_offset(name, + istate->cache[istate->cache_nr - 1]->name, + &len_eq_last); + if (cmp_last > 0 && name[len_eq_last] != '/') + /* + * The entry sorts AFTER the last one in the + * index and their paths have no common prefix, + * so there cannot be a F/D conflict. + */ + return 0; for (;;) { size_t len; From dff0a3ec886cdccf199bdc3881094ec0ab8b3bca Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:14:33 +0100 Subject: [PATCH 06/11] fetch: avoid unnecessary work when there is no current branch As pointed out by CodeQL, `branch_get()` may return `NULL`, in which case `branch_has_merge_config()` would return early, but we can even avoid enumerating the refs prefixes in that case, saving even more CPU cycles. Technically, we should enclose these two statements in an `if (branch) {...}` block, but the indentation is already quite deep, therefore I refrained from doing that. Signed-off-by: Johannes Schindelin --- builtin/fetch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index c1a1434c709625..40a0e8d24434f2 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1728,7 +1728,7 @@ static int do_fetch(struct transport *transport, if (transport->remote->follow_remote_head != FOLLOW_REMOTE_NEVER) do_set_head = 1; } - if (branch_has_merge_config(branch) && + if (branch && branch_has_merge_config(branch) && !strcmp(branch->remote_name, transport->remote->name)) { int i; for (i = 0; i < branch->merge_nr; i++) { From 7d92e08b0c06a546ffd937c333b97d2fb6cd9817 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 17 Dec 2022 00:17:31 +0100 Subject: [PATCH 07/11] Avoid redundant conditions While `if (i <= 0) ... else if (i > 0) ...` is technically equivalent to `if (i <= 0) ... else ...`, the latter is vastly easier to read because it avoids writing out a condition that is unnecessary. Let's drop such unnecessary conditions. Pointed out by CodeQL. Signed-off-by: Johannes Schindelin --- help.c | 2 +- transport-helper.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/help.c b/help.c index 6ef90838f128af..21b778707a6a65 100644 --- a/help.c +++ b/help.c @@ -214,7 +214,7 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) else if (cmp == 0) { ei++; free(cmds->names[ci++]); - } else if (cmp > 0) + } else ei++; } diff --git a/transport-helper.c b/transport-helper.c index 69391ee7d28e11..0789e5bca53282 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -1437,7 +1437,7 @@ static int udt_do_read(struct unidirectional_transfer *t) transfer_debug("%s EOF (with %i bytes in buffer)", t->src_name, (int)t->bufuse); t->state = SSTATE_FLUSHING; - } else if (bytes > 0) { + } else { t->bufuse += bytes; transfer_debug("Read %i bytes from %s (buffer now at %i)", (int)bytes, t->src_name, (int)t->bufuse); From a3f60183633eb17fb300309cf1a629234588418f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 17 Dec 2022 13:20:25 +0100 Subject: [PATCH 08/11] trace2: avoid "futile conditional" CodeQL reports empty `if` blocks that only contain a comment as "futile conditional". The comment talks about potential plans to turn this into a warning, but that seems not to have been necessary. Replace the entire construct with a concise comment. Signed-off-by: Johannes Schindelin --- trace2/tr2_tmr.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c index 51f564b07a4091..038181ad9be05b 100644 --- a/trace2/tr2_tmr.c +++ b/trace2/tr2_tmr.c @@ -102,25 +102,11 @@ void tr2_update_final_timers(void) struct tr2_timer *t_final = &final_timer_block.timer[tid]; struct tr2_timer *t = &ctx->timer_block.timer[tid]; - if (t->recursion_count) { - /* - * The current thread is exiting with - * timer[tid] still running. - * - * Technically, this is a bug, but I'm going - * to ignore it. - * - * I don't think it is worth calling die() - * for. I don't think it is worth killing the - * process for this bookkeeping error. We - * might want to call warning(), but I'm going - * to wait on that. - * - * The downside here is that total_ns won't - * include the current open interval (now - - * start_ns). I can live with that. - */ - } + /* + * `t->recursion_count` could technically be non-zero, which + * would constitute a bug. Reporting the bug would potentially + * cause an infinite recursion, though, so let's ignore it. + */ if (!t->interval_count) continue; /* this timer was not used by this thread */ From 077bcab206f5bfc9fc10a28ad7b726a6ec16c2bb Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 17 Dec 2022 22:21:22 +0100 Subject: [PATCH 09/11] commit-graph: avoid using stale stack addresses The code is a bit too hard to reason about to fully assess whether the `fill_commit_graph_info()` function is called at all after `write_commit_graph()` returns (and hence the stack variable `topo_levels` goes out of context). Let's simply make sure that the stack address is no longer used at that stage, thereby making the code quite a bit easier to reason about. Signed-off-by: Johannes Schindelin --- commit-graph.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/commit-graph.c b/commit-graph.c index 9f0115dac9b528..d052c1bf15c513 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2683,6 +2683,15 @@ int write_commit_graph(struct object_directory *odb, oid_array_clear(&ctx.oids); clear_topo_level_slab(&topo_levels); + if (ctx.r->objects->commit_graph) { + struct commit_graph *g = ctx.r->objects->commit_graph; + + while (g) { + g->topo_levels = NULL; + g = g->base_graph; + } + } + for (i = 0; i < ctx.num_commit_graphs_before; i++) free(ctx.commit_graph_filenames_before[i]); free(ctx.commit_graph_filenames_before); From 4dc3e2335afb42e5006ead7b9b18d33bdae7238f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 23 Mar 2023 11:09:21 +0100 Subject: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()` In c429bed102 (bundle-uri: store fetch.bundleCreationToken, 2023-01-31) code was introduced that assumes that an `sscanf()` call leaves its output variables unchanged unless the return value indicates success. However, the POSIX documentation makes no such guarantee: https://pubs.opengroup.org/onlinepubs/9699919799/functions/sscanf.html So let's make sure that the output variable `maxCreationToken` is always well-defined. Signed-off-by: Johannes Schindelin --- bundle-uri.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/bundle-uri.c b/bundle-uri.c index 96d2ba726d9909..13a42f92387ea5 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -532,11 +532,13 @@ static int fetch_bundles_by_token(struct repository *r, */ if (!repo_config_get_value(r, "fetch.bundlecreationtoken", - &creationTokenStr) && - sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) == 1 && - bundles.items[0]->creationToken <= maxCreationToken) { - free(bundles.items); - return 0; + &creationTokenStr)) { + if (sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) != 1) + maxCreationToken = 0; + if (bundles.items[0]->creationToken <= maxCreationToken) { + free(bundles.items); + return 0; + } } /* From 7a54005bd26ac17cb6d99a2e18932f97575d4aca Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 21 Mar 2025 10:47:00 +0000 Subject: [PATCH 11/11] sequencer: stop pretending that an assignment is a condition In 3e81bccdf3 (sequencer: factor out todo command name parsing, 2019-06-27), a `return` statement was introduced that basically was a long sequence of conditions, combined with `&&`, except for the last condition which is not really a condition but an assignment. The point of this construct was to return 1 (i.e. `true`) from the function if all of those conditions held true, and also assign the `bol` pointer to the end of the parsed command. Some static analyzers are really unhappy about such constructs. And human readers are at least puzzled, if not confused, by seeing a single `=` inside a chain of conditions where they would have expected to see `==` instead and, based on experience, immediately suspect a typo. Let's help all of this by turning this into the more verbose, more readable form of an `if` construct that both assigns the pointer as well as returns 1 if all of the conditions hold true. Signed-off-by: Johannes Schindelin --- sequencer.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index b5c4043757e948..e5e3bc6fa5ea5d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2600,9 +2600,12 @@ static int is_command(enum todo_command command, const char **bol) const char nick = todo_command_info[command].c; const char *p = *bol; - return (skip_prefix(p, str, &p) || (nick && *p++ == nick)) && - (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p) && - (*bol = p); + if ((skip_prefix(p, str, &p) || (nick && *p++ == nick)) && + (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r' || !*p)) { + *bol = p; + return 1; + } + return 0; } static int check_label_or_ref_arg(enum todo_command command, const char *arg)