From fe170014508a2f275b3df484a26d2103ec53676e 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. Let's instead avoid the subtraction altogether. 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 2f459682221d6a..566d7eef5231e3 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 3203c5e07543b4d1b2cb364d952f8287ebd61b4d 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 95fd0018b981fb..ecb38c3b5845a1 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2570,6 +2570,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 1532ac0f36ff530653fbed43efe0180242b585bf 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 | 140 ++++++++++++++++++++++++------------------------- 1 file changed, 69 insertions(+), 71 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 1021ccb983d4ee..8a9e47ed18eba8 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2508,7 +2508,7 @@ 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; uint32_t i; int res = 0; int replace = 0; @@ -2530,16 +2530,16 @@ 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; + memset(&ctx, 0, sizeof(ctx)); + 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", @@ -2548,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; @@ -2564,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) @@ -2585,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 d50283c410f0ca1339b3fe6208858863261f78fc 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 7498b45e2e1e21..6e0a51a9178f3d 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1781,16 +1781,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(ALL_FLAGS); @@ -1799,9 +1799,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) { @@ -1812,27 +1812,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); @@ -1842,9 +1842,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 d6402ce50854516e4e410ab860250705aa1c3ae4 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 e678c13e8f15e2..91f10df2c9b78a 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 && len_eq_last == 0) + /* + * 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 d7974b4f75b5f5b72eec3c9f2e4599b33255e8e3 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 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index ecb38c3b5845a1..3738ec858b5144 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1755,10 +1755,10 @@ static int do_fetch(struct transport *transport, } else { struct branch *branch = branch_get(NULL); - if (transport->remote->fetch.nr) + if (branch && transport->remote->fetch.nr) refspec_ref_prefixes(&transport->remote->fetch, &transport_ls_refs_options.ref_prefixes); - 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 e081ff509469d274c6dd2895163c3407576088f6 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 c54bd9918a5be8..017edfd5f3c3c1 100644 --- a/help.c +++ b/help.c @@ -213,7 +213,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 d457b425501a74..e6b10675828a1e 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 c8a9cfdd0fc736f91f235ef33d7cedfef0daca47 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 6c1450aadb2525bf3b9b77dacff187fbab01dada 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 8a9e47ed18eba8..5e4183e42d505e 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 647a57b66d93dd9f89bde4c27c5cf19b594487d2 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 744257c49c1328..fa5f8a5832cff0 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 325f86dcf072e96ad5401fbbfbf49189c16c28e8 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 ad0ab75c8d4dd7..f0c6d772088153 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2609,9 +2609,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)