{"thread":{"id":"63923","subject":"[PATCH] git-compat-util: introduce `count_t` typedef","startedAt":"2025-08-07T09:23:05Z","lastAt":"2025-08-07T22:07:12Z","messageCount":6,"participants":["Patrick Steinhardt","Matthias Aßhauer","Phillip Wood","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"523743","messageId":"20250807-pks-introduce-count-t-v1-1-e96be52d8db1@pks.im","threadId":"63923","inReplyTo":null,"subject":"[PATCH] git-compat-util: introduce `count_t` typedef","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-07T09:22:56Z","receivedAt":"2025-08-07T09:23:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Historically, Git has been very lenient with its use of integer types\nand didn't really give much thought into which type to use in what\nsituation. We interchangeably mix and match signed and unsigned types\nand often times blindly convert them. This use has led to several\nout-of-bounds reads and writes in the past, some of which could be\nturned into arbitrary code execution.\n\nAs a counter measure we have eventually enabled \"-Wsign-compare\"\nwarnings. Most of our code base generates heaps of warnings, which is\nwhy we have a macro `DISABLE_SIGN_COMPARE_WARNINGS` defined for every\nsuch file. The expectation is that slowly but surely we'll convert our\ncode base to have better hygiene around signedness, and new code that is\nbeing added handles types correctly from the start.\n\nThere are regular discussions around whether or not these warnings are\nsensible to have in the first place. My (biased) opinion with having\nfixed several out-of-bounds reads and writes is that they are senisble,\nas they would have provided warnings around code sites that had those\nissues. And arguably, we still have _lots_ of sites that are susceptible\nto using the wrong type, and more likely than not some of those will be\nexploitable.\n\nFurthermore, I would claim that the question of whether or not those\nwarnings are helpful wouldn't have come up if we had the warnings\nenabled from the inception of Git. The churn caused by the fixes for\nsuch warnings is real, and they need to be done with a lot of care. But\nsince we have removed this project from our microprojects page we don't\nsee \"random\" contributions in this area anymore.\n\nSo overall, the conversions are on the painful side, but in the long\nterm they will help us to protect against introducing new exploits.\n\nA discussion that regularly comes up in this context though is what\ntypes to use for counting entities:\n\n  - One question is whether the type should be signed or unsigned.\n    Arguably, the answer should be to use unsigned types as long as we\n    know that we never need a negative value, e.g. as a sentinel. This\n    helps guide the reader and explicitly conveys the sense that such a\n    counter is only ever going to be a non-negative number. Otherwise,\n    code would need to be more careful as it may hold negative values.\n\n  - Another question is what type to use. In lots of situations we have\n    used `size_t`, but this is conflating semantics. `size_t` is used to\n    count bytes, not entities.\n\nIntroduce a new typedef for `count_t` that is of type `uintptr_t` to\ngive clear guidance what type to use for counting entities. This type\nwas chosen because in the worst case, an entity may be a single byte and\nwe fill all of our memory with these entities. As `uintptr_t` is\nguaranteed to hold at least the value of a pointer, we know that it\ncould be used to index into every single such entity.\n\nAmend the coding guidelines to state when to use `size_t` and when to\nuse `count_t`. Convert an example file to use the new type.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/CodingGuidelines |  3 +++\n builtin/rm.c                   | 25 ++++++++++++-------------\n git-compat-util.h              | 15 +++++++++++++++\n 3 files changed, 30 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 224f0978a8..2e9f3c07ff 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -238,6 +238,9 @@ For shell scripts specifically (not exhaustive):\n \n For C programs:\n \n+ - We use `size_t` to count the number of bytes and `count_t` to count the\n+   number of entities of a given type.\n+\n  - We use tabs to indent, and interpret tabs as taking up to\n    8 spaces.\n \ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 05d89e98c3..99b845cf34 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -33,11 +33,11 @@ static const char * const builtin_rm_usage[] = {\n };\n \n static struct {\n-\tint nr, alloc;\n \tstruct {\n \t\tconst char *name;\n \t\tchar is_submodule;\n \t} *entry;\n+\tcount_t entry_nr, entry_alloc;\n } list;\n \n static int get_ours_cache_pos(const char *path, unsigned int pos)\n@@ -73,8 +73,7 @@ static void print_error_files(struct string_list *files_list,\n \n static void submodules_absorb_gitdir_if_needed(void)\n {\n-\tint i;\n-\tfor (i = 0; i < list.nr; i++) {\n+\tfor (count_t i = 0; i < list.entry_nr; i++) {\n \t\tconst char *name = list.entry[i].name;\n \t\tint pos;\n \t\tconst struct cache_entry *ce;\n@@ -106,14 +105,14 @@ static int check_local_mod(struct object_id *head, int index_only)\n \t * lazy, and who cares if removal of files is a tad\n \t * slower than the theoretical maximum speed?\n \t */\n-\tint i, no_head;\n+\tint no_head;\n \tint errs = 0;\n \tstruct string_list files_staged = STRING_LIST_INIT_NODUP;\n \tstruct string_list files_cached = STRING_LIST_INIT_NODUP;\n \tstruct string_list files_local = STRING_LIST_INIT_NODUP;\n \n \tno_head = is_null_oid(head);\n-\tfor (i = 0; i < list.nr; i++) {\n+\tfor (count_t i = 0; i < list.entry_nr; i++) {\n \t\tstruct stat st;\n \t\tint pos;\n \t\tconst struct cache_entry *ce;\n@@ -268,7 +267,7 @@ int cmd_rm(int argc,\n \t   struct repository *repo UNUSED)\n {\n \tstruct lock_file lock_file = LOCK_INIT;\n-\tint i, ret = 0;\n+\tint ret = 0;\n \tstruct pathspec pathspec;\n \tchar *seen;\n \n@@ -321,10 +320,10 @@ int cmd_rm(int argc,\n \t\t\tcontinue;\n \t\tif (!ce_path_match(the_repository->index, ce, &pathspec, seen))\n \t\t\tcontinue;\n-\t\tALLOC_GROW(list.entry, list.nr + 1, list.alloc);\n-\t\tlist.entry[list.nr].name = xstrdup(ce->name);\n-\t\tlist.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);\n-\t\tif (list.entry[list.nr++].is_submodule &&\n+\t\tALLOC_GROW(list.entry, list.entry_nr + 1, list.entry_alloc);\n+\t\tlist.entry[list.entry_nr].name = xstrdup(ce->name);\n+\t\tlist.entry[list.entry_nr].is_submodule = S_ISGITLINK(ce->ce_mode);\n+\t\tif (list.entry[list.entry_nr++].is_submodule &&\n \t\t    !is_staging_gitmodules_ok(the_repository->index))\n \t\t\tdie(_(\"please stage your changes to .gitmodules or stash them to proceed\"));\n \t}\n@@ -335,7 +334,7 @@ int cmd_rm(int argc,\n \t\tchar *skip_worktree_seen = NULL;\n \t\tstruct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;\n \n-\t\tfor (i = 0; i < pathspec.nr; i++) {\n+\t\tfor (int i = 0; i < pathspec.nr; i++) {\n \t\t\toriginal = pathspec.items[i].original;\n \t\t\tif (seen[i])\n \t\t\t\tseen_any = 1;\n@@ -390,7 +389,7 @@ int cmd_rm(int argc,\n \t * First remove the names from the index: we won't commit\n \t * the index unless all of them succeed.\n \t */\n-\tfor (i = 0; i < list.nr; i++) {\n+\tfor (count_t i = 0; i < list.entry_nr; i++) {\n \t\tconst char *path = list.entry[i].name;\n \t\tif (!quiet)\n \t\t\tprintf(\"rm '%s'\\n\", path);\n@@ -414,7 +413,7 @@ int cmd_rm(int argc,\n \t\tint removed = 0, gitmodules_modified = 0;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint flag = force ? REMOVE_DIR_PURGE_ORIGINAL_CWD : 0;\n-\t\tfor (i = 0; i < list.nr; i++) {\n+\t\tfor (count_t i = 0; i < list.entry_nr; i++) {\n \t\t\tconst char *path = list.entry[i].name;\n \t\t\tif (list.entry[i].is_submodule) {\n \t\t\t\tstrbuf_reset(&buf);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9408f463e3..e9c30d59e8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -610,6 +610,21 @@ static inline bool strip_suffix(const char *str, const char *suffix,\n int git_open_cloexec(const char *name, int flags);\n #define git_open(name) git_open_cloexec(name, O_RDONLY)\n \n+/*\n+ * The type used to count the number of entities, e.g. in an array. We have\n+ * historically used `size_t` for this, but `size_t` is expected to count the\n+ * maximum number of _bytes_, not entities.\n+ *\n+ * The counter is unsigned. If you need to store sentinel values like `-1` you\n+ * should use a different type.\n+ *\n+ * Note that we pick `uintptr_t` because in the theoretical worst case, every\n+ * entity is a single byte and we populate the entire address space with them.\n+ * As `uintptr_t` is able to point to every addressable byte it would also be\n+ * able to count them all.\n+ */\n+typedef uintptr_t count_t;\n+\n static inline size_t st_add(size_t a, size_t b)\n {\n \tif (unsigned_add_overflows(a, b))\n\n---\nbase-commit: 64cbe5e2e8a7b0f92c780b210e602496bd5cad0f\nchange-id: 20250807-pks-introduce-count-t-0f4499f80221\n\n"},{"id":"523744","messageId":"DB9P250MB0692BAB252D4D291F1CE981DA52CA@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM","threadId":"63923","inReplyTo":"20250807-pks-introduce-count-t-v1-1-e96be52d8db1@pks.im","subject":"Re: [PATCH] git-compat-util: introduce `count_t` typedef","fromName":"Matthias Aßhauer","fromEmail":"mha1993@live.de","sentAt":"2025-08-07T11:00:24Z","receivedAt":"2025-08-07T11:00:33Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"\n\nOn Thu, 7 Aug 2025, Patrick Steinhardt wrote:\n\n> Historically, Git has been very lenient with its use of integer types\n> and didn't really give much thought into which type to use in what\n> situation. We interchangeably mix and match signed and unsigned types\n> and often times blindly convert them. This use has led to several\n> out-of-bounds reads and writes in the past, some of which could be\n> turned into arbitrary code execution.\n>\n> As a counter measure we have eventually enabled \"-Wsign-compare\"\n> warnings. Most of our code base generates heaps of warnings, which is\n> why we have a macro `DISABLE_SIGN_COMPARE_WARNINGS` defined for every\n> such file. The expectation is that slowly but surely we'll convert our\n> code base to have better hygiene around signedness, and new code that is\n> being added handles types correctly from the start.\n>\n> There are regular discussions around whether or not these warnings are\n> sensible to have in the first place. My (biased) opinion with having\n> fixed several out-of-bounds reads and writes is that they are senisble,\n> as they would have provided warnings around code sites that had those\n> issues. And arguably, we still have _lots_ of sites that are susceptible\n> to using the wrong type, and more likely than not some of those will be\n> exploitable.\n>\n> Furthermore, I would claim that the question of whether or not those\n> warnings are helpful wouldn't have come up if we had the warnings\n> enabled from the inception of Git. The churn caused by the fixes for\n> such warnings is real, and they need to be done with a lot of care. But\n> since we have removed this project from our microprojects page we don't\n> see \"random\" contributions in this area anymore.\n>\n> So overall, the conversions are on the painful side, but in the long\n> term they will help us to protect against introducing new exploits.\n>\n> A discussion that regularly comes up in this context though is what\n> types to use for counting entities:\n>\n>  - One question is whether the type should be signed or unsigned.\n>    Arguably, the answer should be to use unsigned types as long as we\n>    know that we never need a negative value, e.g. as a sentinel. This\n>    helps guide the reader and explicitly conveys the sense that such a\n>    counter is only ever going to be a non-negative number. Otherwise,\n>    code would need to be more careful as it may hold negative values.\n>\n>  - Another question is what type to use. In lots of situations we have\n>    used `size_t`, but this is conflating semantics. `size_t` is used to\n>    count bytes, not entities.\n>\n> Introduce a new typedef for `count_t` that is of type `uintptr_t` to\n> give clear guidance what type to use for counting entities. This type\n> was chosen because in the worst case, an entity may be a single byte and\n> we fill all of our memory with these entities. As `uintptr_t` is\n> guaranteed to hold at least the value of a pointer, we know that it\n> could be used to index into every single such entity.\n>\n> Amend the coding guidelines to state when to use `size_t` and when to\n> use `count_t`. Convert an example file to use the new type.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> Documentation/CodingGuidelines |  3 +++\n> builtin/rm.c                   | 25 ++++++++++++-------------\n> git-compat-util.h              | 15 +++++++++++++++\n> 3 files changed, 30 insertions(+), 13 deletions(-)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 224f0978a8..2e9f3c07ff 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -238,6 +238,9 @@ For shell scripts specifically (not exhaustive):\n>\n> For C programs:\n>\n> + - We use `size_t` to count the number of bytes and `count_t` to count the\n> +   number of entities of a given type.\n> +\n>  - We use tabs to indent, and interpret tabs as taking up to\n>    8 spaces.\n>\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 05d89e98c3..99b845cf34 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -33,11 +33,11 @@ static const char * const builtin_rm_usage[] = {\n> };\n>\n> static struct {\n> -\tint nr, alloc;\n> \tstruct {\n> \t\tconst char *name;\n> \t\tchar is_submodule;\n> \t} *entry;\n> +\tcount_t entry_nr, entry_alloc;\n> } list;\n>\n> static int get_ours_cache_pos(const char *path, unsigned int pos)\n> @@ -73,8 +73,7 @@ static void print_error_files(struct string_list *files_list,\n>\n> static void submodules_absorb_gitdir_if_needed(void)\n> {\n> -\tint i;\n> -\tfor (i = 0; i < list.nr; i++) {\n> +\tfor (count_t i = 0; i < list.entry_nr; i++) {\n> \t\tconst char *name = list.entry[i].name;\n> \t\tint pos;\n> \t\tconst struct cache_entry *ce;\n> @@ -106,14 +105,14 @@ static int check_local_mod(struct object_id *head, int index_only)\n> \t * lazy, and who cares if removal of files is a tad\n> \t * slower than the theoretical maximum speed?\n> \t */\n> -\tint i, no_head;\n> +\tint no_head;\n> \tint errs = 0;\n> \tstruct string_list files_staged = STRING_LIST_INIT_NODUP;\n> \tstruct string_list files_cached = STRING_LIST_INIT_NODUP;\n> \tstruct string_list files_local = STRING_LIST_INIT_NODUP;\n>\n> \tno_head = is_null_oid(head);\n> -\tfor (i = 0; i < list.nr; i++) {\n> +\tfor (count_t i = 0; i < list.entry_nr; i++) {\n> \t\tstruct stat st;\n> \t\tint pos;\n> \t\tconst struct cache_entry *ce;\n> @@ -268,7 +267,7 @@ int cmd_rm(int argc,\n> \t   struct repository *repo UNUSED)\n> {\n> \tstruct lock_file lock_file = LOCK_INIT;\n> -\tint i, ret = 0;\n> +\tint ret = 0;\n> \tstruct pathspec pathspec;\n> \tchar *seen;\n>\n> @@ -321,10 +320,10 @@ int cmd_rm(int argc,\n> \t\t\tcontinue;\n> \t\tif (!ce_path_match(the_repository->index, ce, &pathspec, seen))\n> \t\t\tcontinue;\n> -\t\tALLOC_GROW(list.entry, list.nr + 1, list.alloc);\n> -\t\tlist.entry[list.nr].name = xstrdup(ce->name);\n> -\t\tlist.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);\n> -\t\tif (list.entry[list.nr++].is_submodule &&\n> +\t\tALLOC_GROW(list.entry, list.entry_nr + 1, list.entry_alloc);\n> +\t\tlist.entry[list.entry_nr].name = xstrdup(ce->name);\n> +\t\tlist.entry[list.entry_nr].is_submodule = S_ISGITLINK(ce->ce_mode);\n> +\t\tif (list.entry[list.entry_nr++].is_submodule &&\n> \t\t    !is_staging_gitmodules_ok(the_repository->index))\n> \t\t\tdie(_(\"please stage your changes to .gitmodules or stash them to proceed\"));\n> \t}\n\nThis hunk doesn't deal with count_t at all. Should we split the renaming \nof nr and alloc into a separate patch?\n> @@ -335,7 +334,7 @@ int cmd_rm(int argc,\n> \t\tchar *skip_worktree_seen = NULL;\n> \t\tstruct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;\n>\n> -\t\tfor (i = 0; i < pathspec.nr; i++) {\n> +\t\tfor (int i = 0; i < pathspec.nr; i++) {\n\nIs this i intentionally still an int?\n\n> \t\t\toriginal = pathspec.items[i].original;\n> \t\t\tif (seen[i])\n> \t\t\t\tseen_any = 1;\n> @@ -390,7 +389,7 @@ int cmd_rm(int argc,\n> \t * First remove the names from the index: we won't commit\n> \t * the index unless all of them succeed.\n> \t */\n> -\tfor (i = 0; i < list.nr; i++) {\n> +\tfor (count_t i = 0; i < list.entry_nr; i++) {\n> \t\tconst char *path = list.entry[i].name;\n> \t\tif (!quiet)\n> \t\t\tprintf(\"rm '%s'\\n\", path);\n> @@ -414,7 +413,7 @@ int cmd_rm(int argc,\n> \t\tint removed = 0, gitmodules_modified = 0;\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \t\tint flag = force ? REMOVE_DIR_PURGE_ORIGINAL_CWD : 0;\n> -\t\tfor (i = 0; i < list.nr; i++) {\n> +\t\tfor (count_t i = 0; i < list.entry_nr; i++) {\n> \t\t\tconst char *path = list.entry[i].name;\n> \t\t\tif (list.entry[i].is_submodule) {\n> \t\t\t\tstrbuf_reset(&buf);\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 9408f463e3..e9c30d59e8 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -610,6 +610,21 @@ static inline bool strip_suffix(const char *str, const char *suffix,\n> int git_open_cloexec(const char *name, int flags);\n> #define git_open(name) git_open_cloexec(name, O_RDONLY)\n>\n> +/*\n> + * The type used to count the number of entities, e.g. in an array. We have\n> + * historically used `size_t` for this, but `size_t` is expected to count the\n> + * maximum number of _bytes_, not entities.\n> + *\n> + * The counter is unsigned. If you need to store sentinel values like `-1` you\n> + * should use a different type.\n\nDo we want to make a recommendation of a \"different type\" here to keep \nthings consistent?\n> + *\n> + * Note that we pick `uintptr_t` because in the theoretical worst case, every\n> + * entity is a single byte and we populate the entire address space with them.\n> + * As `uintptr_t` is able to point to every addressable byte it would also be\n> + * able to count them all.\n> + */\n> +typedef uintptr_t count_t;\n> +\n> static inline size_t st_add(size_t a, size_t b)\n> {\n> \tif (unsigned_add_overflows(a, b))\n>\n> ---\n> base-commit: 64cbe5e2e8a7b0f92c780b210e602496bd5cad0f\n> change-id: 20250807-pks-introduce-count-t-0f4499f80221\n>\n>\n\nBest regards\n\nMatthias\n"},{"id":"523749","messageId":"582e8e75-c6eb-4845-8f3b-62f234f0964f@gmail.com","threadId":"63923","inReplyTo":"20250807-pks-introduce-count-t-v1-1-e96be52d8db1@pks.im","subject":"Re: [PATCH] git-compat-util: introduce `count_t` typedef","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-07T14:17:14Z","receivedAt":"2025-08-07T14:17:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Patrick\n\nOn 07/08/2025 10:22, Patrick Steinhardt wrote:\n> Historically, Git has been very lenient with its use of integer types\n> and didn't really give much thought into which type to use in what\n> situation. We interchangeably mix and match signed and unsigned types\n> and often times blindly convert them. This use has led to several\n> out-of-bounds reads and writes in the past, some of which could be\n> turned into arbitrary code execution.\n\nMy feeling is that one of the main problems has been using different \ntypes for loop indexes and loop limits. If we mandated that the loop \nindex had to be the same type as the limit that would improve things \nconsiderably and without mandating a particular type.\n\n> A discussion that regularly comes up in this context though is what\n> types to use for counting entities:\n> \n>    - One question is whether the type should be signed or unsigned.\n>      Arguably, the answer should be to use unsigned types as long as we\n>      know that we never need a negative value, e.g. as a sentinel. This\n>      helps guide the reader and explicitly conveys the sense that such a\n>      counter is only ever going to be a non-negative number. Otherwise,\n>      code would need to be more careful as it may hold negative values.\n\nThe counter argument to this is that it is easy to write incorrect loops \nwhen counting down if the loop variable is unsigned. Using a typedef \nthat hides the actual type makes that harder to spot as it is not \nimmediately obvious whether the loop index is signed or not. As we have \ncases that do need to store a negative value then we're still left with \nusing a mix of signed and unsigned types for counting in our code base.\n\n> Introduce a new typedef for `count_t` that is of type `uintptr_t` to\n> give clear guidance what type to use for counting entities. This type\n> was chosen because in the worst case, an entity may be a single byte and\n> we fill all of our memory with these entities. As `uintptr_t` is\n> guaranteed to hold at least the value of a pointer, we know that it\n> could be used to index into every single such entity.\n\nHow many sites actually allocate anything like that number of \nentities?Generally we use ALLOC_GROW() or ALLOC_GROW_BY() which means \nthat we're not normally counting bytes. ALLOC_GROW_BY() assumes the \nnumber of entities fits into a size_t so should be be changing that to \nuse count_t? If we're worried about overflows then maybe we should look \nat alloc_nr() which calculates the new allocation with\n\n     (nr + 16) * 3 / 2\n\nwhich which will start overflowing long before we starting allocating \nUINTPTR_MAX single byte entities.\n\nThanks\n\nPhillip\n\n"},{"id":"523762","messageId":"xmqqa54bqe7d.fsf@gitster.g","threadId":"63923","inReplyTo":"20250807-pks-introduce-count-t-v1-1-e96be52d8db1@pks.im","subject":"Re: [PATCH] git-compat-util: introduce `count_t` typedef","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-07T16:38:46Z","receivedAt":"2025-08-07T16:38:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>  For C programs:\n>  \n> + - We use `size_t` to count the number of bytes and `count_t` to count the\n> +   number of entities of a given type.\n\nI am not interested in this specific implementation at all for a\nnumber of reasons, but I am excited to see people thinking about the\nissues.  The following is a random list of things, both positive and\nnegative, that came to my mind after skimming the changes.\n\n * We do not want to pretend that one size fits all.  If it were a\n   good idea for developers to express \"This variable is a simple\n   counter that counts up from 0 and never goes negative\" by using\n   an unsigned type (which is dubious), it should be equally, or not\n   more, a good idea to allow them to say \"We will not have more\n   than 256 fan-out directories under .git/objects/ and this is a\n   counter to count them, so I know 'unsigned short' is big enough\n   on any platforms\".\n\n * As far as I can tell, the patch does not seem to address the\n   biggest concern of unsigned integer wraparound.  We often see\n\n\tALLOC_GROW(thing.entry, thing.nr + 1, thing.alloc);\n    \n   with the arithmetic \"thing.nr + 1\" checked by nobody.\n   ALLOC_GROW_BY() is slightly better in this regard, but nobody\n   uses it with only small exceptions.  And of course, alloc_nr()\n   does even riskier arithmetic that is unchecked.\n\n * Standardising the names used for <item[], item_nr, item_alloc>\n   somehow is very much welcome (we can see an example in the change\n   to builtin/rm.c below).  Such a naming convention would allow us\n   to write\n\n\t#define ALLOC_INCR(thing) ALLOC_INCR_BY(thing, 1)\n\tALLOC_INCR_BY(thing, increment)\n\n   that do ALLOC_GROW(thing, thing_nr + increment, thing_alloc) more\n   safely than what the current code does, perhaps?  Also, we should\n   be able to use any unsigned integral type and perform sensible\n   bound checking with typeof().\n\n * The codebase avoids inventing a new type with typedef, with the\n   exception of callback function type, following old tradition we\n   inherited from the Linux kernel project.  And even when we create\n   a new type, of course, we do not want to give it a name that ends\n   with \"_t\".\n\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 05d89e98c3..99b845cf34 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -33,11 +33,11 @@ static const char * const builtin_rm_usage[] = {\n>  };\n>  \n>  static struct {\n> -\tint nr, alloc;\n>  \tstruct {\n>  \t\tconst char *name;\n>  \t\tchar is_submodule;\n>  \t} *entry;\n> +\tcount_t entry_nr, entry_alloc;\n>  } list;\n"},{"id":"523763","messageId":"xmqq4iujqdzm.fsf@gitster.g","threadId":"63923","inReplyTo":"582e8e75-c6eb-4845-8f3b-62f234f0964f@gmail.com","subject":"Re: [PATCH] git-compat-util: introduce `count_t` typedef","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-07T16:43:25Z","receivedAt":"2025-08-07T16:43:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Hi Patrick\n>\n> On 07/08/2025 10:22, Patrick Steinhardt wrote:\n>> Historically, Git has been very lenient with its use of integer types\n>> and didn't really give much thought into which type to use in what\n>> situation. We interchangeably mix and match signed and unsigned types\n>> and often times blindly convert them. This use has led to several\n>> out-of-bounds reads and writes in the past, some of which could be\n>> turned into arbitrary code execution.\n>\n> My feeling is that one of the main problems has been using different\n> types for loop indexes and loop limits. If we mandated that the loop\n> index had to be the same type as the limit that would improve things\n> considerably and without mandating a particular type.\n\nYup.  And the limit being unsigned would force the counter to be\nalso unsigned, which can introduce buggy constructs (like counting\ndown).\n\n>> A discussion that regularly comes up in this context though is what\n>> types to use for counting entities:\n>>    - One question is whether the type should be signed or unsigned.\n>>      Arguably, the answer should be to use unsigned types as long as we\n>>      know that we never need a negative value, e.g. as a sentinel. This\n>>      helps guide the reader and explicitly conveys the sense that such a\n>>      counter is only ever going to be a non-negative number. Otherwise,\n>>      code would need to be more careful as it may hold negative values.\n>\n> The counter argument to this is that it is easy to write incorrect\n> loops when counting down if the loop variable is unsigned. Using a\n> typedef that hides the actual type makes that harder to spot as it is\n> not immediately obvious whether the loop index is signed or not.\n\nThis is very valid argument against typedef for something trivial\nlike an integer.  Use of proposed count_t loses both size and\nsignedness information.\n\nThanks.\n"},{"id":"523778","messageId":"aJUjh0Uu3/UU5bVg@nand.local","threadId":"63923","inReplyTo":"xmqqa54bqe7d.fsf@gitster.g","subject":"Re: [PATCH] git-compat-util: introduce `count_t` typedef","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-08-07T22:07:03Z","receivedAt":"2025-08-07T22:07:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 07, 2025 at 09:38:46AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n> >  For C programs:\n> >\n> > + - We use `size_t` to count the number of bytes and `count_t` to count the\n> > +   number of entities of a given type.\n>\n> I am not interested in this specific implementation at all for a\n> number of reasons, but I am excited to see people thinking about the\n> issues.  The following is a random list of things, both positive and\n> negative, that came to my mind after skimming the changes.\n>\n>  * We do not want to pretend that one size fits all.  If it were a\n>    good idea for developers to express \"This variable is a simple\n>    counter that counts up from 0 and never goes negative\" by using\n>    an unsigned type (which is dubious), it should be equally, or not\n>    more, a good idea to allow them to say \"We will not have more\n>    than 256 fan-out directories under .git/objects/ and this is a\n>    counter to count them, so I know 'unsigned short' is big enough\n>    on any platforms\".\n\nThis to me is the most compelling argument against a \"count_t\" typedef\nor something similar. Different callers have different needs (the ones\nyou pointed out above are the ones that I thought of as most relevant),\nand we shouldn't force them to all use the same type, or pretend that\none type is best for all of them.\n\n>  * As far as I can tell, the patch does not seem to address the\n>    biggest concern of unsigned integer wraparound.  We often see\n>\n> \tALLOC_GROW(thing.entry, thing.nr + 1, thing.alloc);\n>\n>    with the arithmetic \"thing.nr + 1\" checked by nobody.\n>    ALLOC_GROW_BY() is slightly better in this regard, but nobody\n>    uses it with only small exceptions.  And of course, alloc_nr()\n>    does even riskier arithmetic that is unchecked.\n\nI wonder if we should push more people towards ALLOC_GROW_BY() for that\nreason. We could do something like recommend that callers use\nALLOC_GROW_BY() instead of ALLOC_GROW() in cases like:\n\n    @@\n    expression array, nr, n, alloc;\n    @@\n    - ALLOC_GROW(array, nr + n, alloc)\n    + ALLOC_GROW_BY(array, nr, n, alloc)\n\n, but I'm not sure that's a good idea as a blanket rule, since it's\nchanging the behavior away from using alloc_nr() to instead grow by a\nfixed amount.\n\nWe have definitely talked before about adding overflow checks to\nalloc_nr() before, but I think the slow-down made it a non-starter\n(IIRC). I wonder if something like this:\n\n    diff --git a/git-compat-util.h b/git-compat-util.h\n    index 9408f463e31..22b8701b40d 100644\n    --- a/git-compat-util.h\n    +++ b/git-compat-util.h\n    @@ -852,11 +852,14 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)\n      */\n     #define ALLOC_GROW(x, nr, alloc) \\\n      do { \\\n    +\t\tsize_t __alloc__ = alloc; \\\n        if ((nr) > alloc) { \\\n          if (alloc_nr(alloc) < (nr)) \\\n            alloc = (nr); \\\n          else \\\n            alloc = alloc_nr(alloc); \\\n    +\t\t\tif (alloc < __alloc__) \\\n    +\t\t\t\tBUG(\"negative growth in ALLOC_GROW\"); \\\n          REALLOC_ARRAY(x, alloc); \\\n        } \\\n      } while (0)\n\nwould be a reasonable compromise? It's not quite as careful as checking\neach step of the computation done by alloc_nr(), but it's better than\nnot checking at all.\n\nSo perhaps we should do some combination of the two ;-).\n\n>  * Standardising the names used for <item[], item_nr, item_alloc>\n>    somehow is very much welcome (we can see an example in the change\n>    to builtin/rm.c below).  Such a naming convention would allow us\n>    to write\n>\n> \t#define ALLOC_INCR(thing) ALLOC_INCR_BY(thing, 1)\n> \tALLOC_INCR_BY(thing, increment)\n>\n>    that do ALLOC_GROW(thing, thing_nr + increment, thing_alloc) more\n>    safely than what the current code does, perhaps?  Also, we should\n>    be able to use any unsigned integral type and perform sensible\n>    bound checking with typeof().\n\n...meaning that ALLOC_INCR() and ALLOC_INCR_BY() would use thing##_nr? I\ndo like the idea of standardizing on that naming scheme, but the\nthing##_nr approach is a bit magical for my taste.\n\nThanks,\nTaylor\n"}]}