On 07/03/2022 15:27, Ævar Arnfjörð Bjarmason wrote:
> Change the "nr" and "alloc" members of "struct string_list" to use
> "size_t" instead of "nr". On some platforms the size of an "unsigned
> int" will be smaller than a "size_t", e.g. a 32 bit unsigned v.s. 64
> bit unsigned. As "struct string_list" is a generic API we use in a lot
> of places this might cause overflows.
>
> As one example: code in "refs.c" keeps track of the number of refs
> with a "size_t", and auxiliary code in builtin/remote.c in
> get_ref_states() appends those to a "struct string_list".
>
> While we're at it split the "nr" and "alloc" in string-list.h across
> two lines, which is the case for most such struct member
> declarations (e.g. in "strbuf.h" and "strvec.h").
>
> Changing e.g. "int i" to "size_t i" in run_and_feed_hook() isn't
> strictly necessary, and there are a lot more cases where we'll use a
> local "int", "unsigned int" etc. variable derived from the "nr" in the
> "struct string_list". But in that case as well as
> add_wrapped_shortlog_msg() in builtin/shortlog.c we need to adjust the
> printf format referring to "nr" anyway, so let's also change the other
> variables referring to it.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> builtin/receive-pack.c | 9 +++++----
> builtin/shortlog.c | 10 +++++-----
> bundle.c | 8 ++++----
> commit-graph.c | 6 +++---
> mailmap.c | 7 ++++---
> merge-ort.c | 4 ++--
> string-list.h | 3 ++-
> t/helper/test-run-command.c | 7 ++++---
> wt-status.c | 12 ++++++------
> 9 files changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index d10aeb7e78f..fc948a27c4f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -813,13 +813,14 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
> proc.trace2_hook_name = hook_name;
>
> if (feed_state->push_options) {
> - int i;
> + size_t i;
> for (i = 0; i < feed_state->push_options->nr; i++)
> strvec_pushf(&proc.env_array,
> - "GIT_PUSH_OPTION_%d=%s", i,
> + "GIT_PUSH_OPTION_%"PRIuMAX"=%s",
> + (uintmax_t)i,
> feed_state->push_options->items[i].string);
> - strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%d",
> - feed_state->push_options->nr);
> + strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%"PRIuMAX"",
> + (uintmax_t)feed_state->push_options->nr);
> } else
> strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
>
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 228d782754a..26c5c0cf935 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -435,7 +435,7 @@ static void add_wrapped_shortlog_msg(struct strbuf *sb, const char *s,
>
> void shortlog_output(struct shortlog *log)
> {
> - int i, j;
> + size_t i, j;
> struct strbuf sb = STRBUF_INIT;
>
> if (log->sort_by_number)
> @@ -448,10 +448,10 @@ void shortlog_output(struct shortlog *log)
> (int)UTIL_TO_INT(item), item->string);
> } else {
> struct string_list *onelines = item->util;
> - fprintf(log->file, "%s (%d):\n",
> - item->string, onelines->nr);
> - for (j = onelines->nr - 1; j >= 0; j--) {
> - const char *msg = onelines->items[j].string;
> + fprintf(log->file, "%s (%"PRIuMAX"):\n",
> + item->string, (uintmax_t)onelines->nr);
> + for (j = onelines->nr; j >= 1; j--) {
> + const char *msg = onelines->items[j - 1].string;
Does this change of iteration limits and j's offset need a commit
message comment?
Philip
>
> if (log->wrap_lines) {
> strbuf_reset(&sb);
> diff --git a/bundle.c b/bundle.c
> index a0bb687b0f4..7608701a51e 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -255,18 +255,18 @@ int verify_bundle(struct repository *r,
>
> r = &header->references;
> printf_ln(Q_("The bundle contains this ref:",
> - "The bundle contains these %d refs:",
> + "The bundle contains these %"PRIuMAX" refs:",
> r->nr),
> - r->nr);
> + (uintmax_t)r->nr);
> list_refs(r, 0, NULL);
> r = &header->prerequisites;
> if (!r->nr) {
> printf_ln(_("The bundle records a complete history."));
> } else {
> printf_ln(Q_("The bundle requires this ref:",
> - "The bundle requires these %d refs:",
> + "The bundle requires these %"PRIuMAX" refs:",
> r->nr),
> - r->nr);
> + (uintmax_t)r->nr);
> list_refs(r, 0, NULL);
> }
> }
> diff --git a/commit-graph.c b/commit-graph.c
> index 265c010122e..e7731db8f40 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1690,10 +1690,10 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
> dirlen = packname.len;
> if (ctx->report_progress) {
> strbuf_addf(&progress_title,
> - Q_("Finding commits for commit graph in %d pack",
> - "Finding commits for commit graph in %d packs",
> + Q_("Finding commits for commit graph in %"PRIuMAX" pack",
> + "Finding commits for commit graph in %"PRIuMAX" packs",
> pack_indexes->nr),
> - pack_indexes->nr);
> + (uintmax_t)pack_indexes->nr);
> ctx->progress = start_delayed_progress(progress_title.buf, 0);
> ctx->progress_done = 0;
> }
> diff --git a/mailmap.c b/mailmap.c
> index 40ce152024d..7befdc5e483 100644
> --- a/mailmap.c
> +++ b/mailmap.c
> @@ -43,8 +43,8 @@ static void free_mailmap_info(void *p, const char *s)
> static void free_mailmap_entry(void *p, const char *s)
> {
> struct mailmap_entry *me = (struct mailmap_entry *)p;
> - debug_mm("mailmap: removing entries for <%s>, with %d sub-entries\n",
> - s, me->namemap.nr);
> + debug_mm("mailmap: removing entries for <%s>, with %"PRIuMAX" sub-entries\n",
> + s, (uintmax_t)me->namemap.nr);
> debug_mm("mailmap: - simple: '%s' <%s>\n",
> debug_str(me->name), debug_str(me->email));
>
> @@ -250,7 +250,8 @@ int read_mailmap(struct string_list *map)
>
> void clear_mailmap(struct string_list *map)
> {
> - debug_mm("mailmap: clearing %d entries...\n", map->nr);
> + debug_mm("mailmap: clearing %"PRIuMAX" entries...\n",
> + (uintmax_t)map->nr);
> map->strdup_strings = 1;
> string_list_clear_func(map, free_mailmap_entry);
> debug_mm("mailmap: cleared\n");
> diff --git a/merge-ort.c b/merge-ort.c
> index 55decb2587e..bb2be2a2758 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4064,8 +4064,8 @@ static void process_entries(struct merge_options *opt,
> trace2_region_enter("merge", "process_entries cleanup", opt->repo);
> if (dir_metadata.offsets.nr != 1 ||
> (uintptr_t)dir_metadata.offsets.items[0].util != 0) {
> - printf("dir_metadata.offsets.nr = %d (should be 1)\n",
> - dir_metadata.offsets.nr);
> + printf("dir_metadata.offsets.nr = %"PRIuMAX" (should be 1)\n",
> + (uintmax_t)dir_metadata.offsets.nr);
> printf("dir_metadata.offsets.items[0].util = %u (should be 0)\n",
> (unsigned)(uintptr_t)dir_metadata.offsets.items[0].util);
> fflush(stdout);
> diff --git a/string-list.h b/string-list.h
> index 267d6e5769d..d5a744e1438 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -86,7 +86,8 @@ typedef int (*compare_strings_fn)(const char *, const char *);
> */
> struct string_list {
> struct string_list_item *items;
> - unsigned int nr, alloc;
> + size_t nr;
> + size_t alloc;
> unsigned int strdup_strings:1;
> compare_strings_fn cmp; /* NULL uses strcmp() */
> };
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 913775a14b7..eabd28defc5 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -180,15 +180,16 @@ static int testsuite(int argc, const char **argv)
> if (max_jobs > suite.tests.nr)
> max_jobs = suite.tests.nr;
>
> - fprintf(stderr, "Running %d tests (%d at a time)\n",
> - suite.tests.nr, max_jobs);
> + fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n",
> + (uintmax_t)suite.tests.nr, max_jobs);
>
> ret = run_processes_parallel(max_jobs, next_test, test_failed,
> test_finished, &suite);
>
> if (suite.failed.nr > 0) {
> ret = 1;
> - fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
> + fprintf(stderr, "%"PRIuMAX" tests failed:\n\n",
> + (uintmax_t)suite.failed.nr);
> for (i = 0; i < suite.failed.nr; i++)
> fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
> }
> diff --git a/wt-status.c b/wt-status.c
> index 335e723a71e..bd54cf59894 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1374,10 +1374,10 @@ static void show_rebase_information(struct wt_status *s,
> status_printf_ln(s, color, _("No commands done."));
> else {
> status_printf_ln(s, color,
> - Q_("Last command done (%d command done):",
> - "Last commands done (%d commands done):",
> + Q_("Last command done (%"PRIuMAX" command done):",
> + "Last commands done (%"PRIuMAX" commands done):",
> have_done.nr),
> - have_done.nr);
> + (uintmax_t)have_done.nr);
> for (i = (have_done.nr > nr_lines_to_show)
> ? have_done.nr - nr_lines_to_show : 0;
> i < have_done.nr;
> @@ -1393,10 +1393,10 @@ static void show_rebase_information(struct wt_status *s,
> _("No commands remaining."));
> else {
> status_printf_ln(s, color,
> - Q_("Next command to do (%d remaining command):",
> - "Next commands to do (%d remaining commands):",
> + Q_("Next command to do (%"PRIuMAX" remaining command):",
> + "Next commands to do (%"PRIuMAX" remaining commands):",
> yet_to_do.nr),
> - yet_to_do.nr);
> + (uintmax_t)yet_to_do.nr);
> for (i = 0; i < nr_lines_to_show && i < yet_to_do.nr; i++)
> status_printf_ln(s, color, " %s", yet_to_do.items[i].string);
> if (s->hints)
