{"thread":{"id":"62662","subject":"[PATCH] git: use U to denote unsigned to prevent UB","startedAt":"2024-12-18T02:22:33Z","lastAt":"2025-01-02T21:34:01Z","messageCount":3,"participants":["AreaZR via GitGitGadget","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509265","messageId":"pull.1849.git.git.1734488549111.gitgitgadget@gmail.com","threadId":"62662","inReplyTo":null,"subject":"[PATCH] git: use U to denote unsigned to prevent UB","fromName":"AreaZR via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-12-18T02:22:28Z","receivedAt":"2024-12-18T02:22:33Z","isPatch":true,"sender":{"key":"name:AreaZR","avatar":null},"body":"From: Seija Kijin <doremylover123@gmail.com>\n\n1 << can be UB if 1 ends up overflowing and\nbeing assigned to an unsigned int or long.\n\nSigned-off-by: Seija Kijin <doremylover123@gmail.com>\n---\n    git: use U to denote unsigned to prevent UB\n    \n    1 << can be UB if 1 ends up overflowing and being assigned to an\n    unsigned int or long.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1849%2FAreaZR%2F1U-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1849/AreaZR/1U-v1\nPull-Request: https://github.com/git/git/pull/1849\n\n builtin/checkout.c     |  2 +-\n builtin/merge-tree.c   |  4 ++--\n builtin/receive-pack.c |  2 +-\n color.c                |  4 ++--\n delta-islands.c        |  2 +-\n diff-delta.c           |  2 +-\n diff.c                 |  2 +-\n help.c                 |  2 +-\n imap-send.c            |  2 +-\n merge-ort.c            | 18 +++++++++---------\n xdiff/xhistogram.c     |  2 +-\n xdiff/xprepare.c       |  4 ++--\n 12 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5e5afa0f267..a636e71e05c 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -223,7 +223,7 @@ static int check_stages(unsigned stages, const struct cache_entry *ce, int pos)\n \t\tce = the_repository->index->cache[pos];\n \t\tif (strcmp(name, ce->name))\n \t\t\tbreak;\n-\t\tseen |= (1 << ce_stage(ce));\n+\t\tseen |= (1U << ce_stage(ce));\n \t\tpos++;\n \t}\n \tif ((stages & seen) != stages)\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex c5ed472967a..d0104dfa0c7 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -270,13 +270,13 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])\n \tunsigned dirmask = 0, mask = 0;\n \n \tfor (i = 0; i < 3; i++) {\n-\t\tmask |= (1 << i);\n+\t\tmask |= (1U << i);\n \t\t/*\n \t\t * Treat missing entries as directories so that we return\n \t\t * after unresolved_directory has handled this.\n \t\t */\n \t\tif (!n[i].mode || S_ISDIR(n[i].mode))\n-\t\t\tdirmask |= (1 << i);\n+\t\t\tdirmask |= (1U << i);\n \t}\n \n \tunresolved_directory(info, n);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9d2c07f68da..b958eeee8fe 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1306,7 +1306,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \tstruct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;\n \tstruct oid_array extra = OID_ARRAY_INIT;\n \tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n-\tuint32_t mask = 1 << (cmd->index % 32);\n+\tuint32_t mask = 1U << (cmd->index % 32);\n \tint i;\n \n \ttrace_printf_key(&trace_shallow,\ndiff --git a/color.c b/color.c\nindex 227a5ab2f42..ab9a3d2a097 100644\n--- a/color.c\n+++ b/color.c\n@@ -317,7 +317,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)\n \t\t}\n \t\tval = parse_attr(word, wordlen);\n \t\tif (0 <= val)\n-\t\t\tattr |= (1 << val);\n+\t\t\tattr |= (1U << val);\n \t\telse\n \t\t\tgoto bad;\n \t}\n@@ -340,7 +340,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)\n \t\t\tsep++;\n \n \t\tfor (i = 0; attr; i++) {\n-\t\t\tunsigned bit = (1 << i);\n+\t\t\tunsigned bit = (1U << i);\n \t\t\tif (!(attr & bit))\n \t\t\t\tcontinue;\n \t\t\tattr &= ~bit;\ndiff --git a/delta-islands.c b/delta-islands.c\nindex 84435512593..a041cfa1ab3 100644\n--- a/delta-islands.c\n+++ b/delta-islands.c\n@@ -78,7 +78,7 @@ static int island_bitmap_is_subset(struct island_bitmap *self,\n }\n \n #define ISLAND_BITMAP_BLOCK(x) (x / 32)\n-#define ISLAND_BITMAP_MASK(x) (1 << (x % 32))\n+#define ISLAND_BITMAP_MASK(x) (1U << (x % 32))\n \n static void island_bitmap_set(struct island_bitmap *self, uint32_t i)\n {\ndiff --git a/diff-delta.c b/diff-delta.c\nindex 77fea08dfb0..fbdfec7037f 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -156,7 +156,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n \t}\n \thsize = entries / 4;\n \tfor (i = 4; (1u << i) < hsize; i++);\n-\thsize = 1 << i;\n+\thsize = 1u << i;\n \thmask = hsize - 1;\n \n \t/* allocate lookup index */\ndiff --git a/diff.c b/diff.c\nindex 266ddf18e73..021df059e0b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4815,7 +4815,7 @@ static void prepare_filter_bits(void)\n \n \tif (!filter_bit[DIFF_STATUS_ADDED]) {\n \t\tfor (i = 0; diff_status_letters[i]; i++)\n-\t\t\tfilter_bit[(int) diff_status_letters[i]] = (1 << i);\n+\t\t\tfilter_bit[(int) diff_status_letters[i]] = (1U << i);\n \t}\n }\n \ndiff --git a/help.c b/help.c\nindex 8a830ba35c6..839596156fe 100644\n--- a/help.c\n+++ b/help.c\n@@ -394,7 +394,7 @@ void list_cmds_by_category(struct string_list *list,\n \n \tfor (i = 0; category_names[i]; i++) {\n \t\tif (!strcmp(cat, category_names[i])) {\n-\t\t\tcat_id = 1UL << i;\n+\t\t\tcat_id = 1U << i;\n \t\t\tbreak;\n \t\t}\n \t}\ndiff --git a/imap-send.c b/imap-send.c\nindex 25c68fd90d7..fdb9e658e70 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -644,7 +644,7 @@ static void parse_capability(struct imap *imap, char *cmd)\n \twhile ((arg = next_arg(&cmd)))\n \t\tfor (i = 0; i < ARRAY_SIZE(cap_list); i++)\n \t\t\tif (!strcmp(cap_list[i], arg))\n-\t\t\t\timap->caps |= 1 << i;\n+\t\t\t\timap->caps |= 1U << i;\n \timap->rcaps = imap->caps;\n }\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex 11029c10be3..5a99bec7a04 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1218,7 +1218,7 @@ static void collect_rename_info(struct merge_options *opt,\n \t\treturn;\n \n \tfor (side = MERGE_SIDE1; side <= MERGE_SIDE2; ++side) {\n-\t\tunsigned side_mask = (1 << side);\n+\t\tunsigned side_mask = (1U << side);\n \n \t\t/* Check for deletion on side */\n \t\tif ((filemask & 1) && !(filemask & side_mask))\n@@ -2026,7 +2026,7 @@ static void initialize_attr_index(struct merge_options *opt)\n \n \t\tASSIGN_AND_VERIFY_CI(ci, mi);\n \t\tfor (stage = 0; stage < 3; stage++) {\n-\t\t\tunsigned stage_mask = (1 << stage);\n+\t\t\tunsigned stage_mask = (1U << stage);\n \n \t\t\tif (!(ci->filemask & stage_mask))\n \t\t\t\tcontinue;\n@@ -2362,7 +2362,7 @@ static char *handle_path_level_conflicts(struct merge_options *opt,\n \t */\n \tif (c_info->reported_already) {\n \t\tclean = 0;\n-\t} else if (path_in_way(&opt->priv->paths, new_path, 1 << side_index)) {\n+\t} else if (path_in_way(&opt->priv->paths, new_path, 1U << side_index)) {\n \t\tc_info->reported_already = 1;\n \t\tstrbuf_add_separated_string_list(&collision_paths, \", \",\n \t\t\t\t\t\t &c_info->source_files);\n@@ -2747,7 +2747,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n \t\tci->filemask = 0;\n \t\tci->merged.clean = 1;\n \t\tfor (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {\n-\t\t\tif (ci->dirmask & (1 << i))\n+\t\t\tif (ci->dirmask & (1U << i))\n \t\t\t\tcontinue;\n \t\t\t/* zero out any entries related to files */\n \t\t\tci->stages[i].mode = 0;\n@@ -2915,7 +2915,7 @@ static int process_renames(struct merge_options *opt,\n \t\t\t\tassert(side1 == side2);\n \t\t\t\tmemcpy(&side1->stages[0], &base->stages[0],\n \t\t\t\t       sizeof(merged));\n-\t\t\t\tside1->filemask |= (1 << MERGE_BASE);\n+\t\t\t\tside1->filemask |= (1U << MERGE_BASE);\n \t\t\t\t/* Mark base as resolved by removal */\n \t\t\t\tbase->merged.is_null = 1;\n \t\t\t\tbase->merged.clean = 1;\n@@ -3002,7 +3002,7 @@ static int process_renames(struct merge_options *opt,\n \t\ttarget_index = pair->score; /* from collect_renames() */\n \t\tassert(target_index == 1 || target_index == 2);\n \t\tother_source_index = 3 - target_index;\n-\t\told_sidemask = (1 << other_source_index); /* 2 or 4 */\n+\t\told_sidemask = (1U << other_source_index); /* 2 or 4 */\n \t\tsource_deleted = (oldinfo->filemask == 1);\n \t\tcollision = ((newinfo->filemask & old_sidemask) != 0);\n \t\ttype_changed = !source_deleted &&\n@@ -3116,7 +3116,7 @@ static int process_renames(struct merge_options *opt,\n \t\t\t */\n \t\t\tmemcpy(&newinfo->stages[0], &oldinfo->stages[0],\n \t\t\t       sizeof(newinfo->stages[0]));\n-\t\t\tnewinfo->filemask |= (1 << MERGE_BASE);\n+\t\t\tnewinfo->filemask |= (1U << MERGE_BASE);\n \t\t\tnewinfo->pathnames[0] = oldpath;\n \t\t\tif (type_changed) {\n \t\t\t\t/* rename vs. typechange */\n@@ -3139,7 +3139,7 @@ static int process_renames(struct merge_options *opt,\n \t\t\t\tmemcpy(&newinfo->stages[other_source_index],\n \t\t\t\t       &oldinfo->stages[other_source_index],\n \t\t\t\t       sizeof(newinfo->stages[0]));\n-\t\t\t\tnewinfo->filemask |= (1 << other_source_index);\n+\t\t\t\tnewinfo->filemask |= (1U << other_source_index);\n \t\t\t\tnewinfo->pathnames[other_source_index] = oldpath;\n \t\t\t}\n \t\t}\n@@ -3990,7 +3990,7 @@ static int process_entry(struct merge_options *opt,\n \t\tci->match_mask = (ci->match_mask & ~ci->dirmask);\n \t\tci->dirmask = 0;\n \t\tfor (i = MERGE_BASE; i <= MERGE_SIDE2; i++) {\n-\t\t\tif (ci->filemask & (1 << i))\n+\t\t\tif (ci->filemask & (1U << i))\n \t\t\t\tcontinue;\n \t\t\tci->stages[i].mode = 0;\n \t\t\toidcpy(&ci->stages[i].oid, null_oid());\ndiff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c\nindex 16a8fe2f3f3..18a037a3ba8 100644\n--- a/xdiff/xhistogram.c\n+++ b/xdiff/xhistogram.c\n@@ -265,7 +265,7 @@ static int find_lcs(xpparam_t const *xpp, xdfenv_t *env,\n \tindex.rcha.head = NULL;\n \n \tindex.table_bits = xdl_hashbits(count1);\n-\tindex.records_size = 1 << index.table_bits;\n+\tindex.records_size = 1U << index.table_bits;\n \tif (!XDL_CALLOC_ARRAY(index.records, index.records_size))\n \t\tgoto cleanup;\n \ndiff --git a/xdiff/xprepare.c b/xdiff/xprepare.c\nindex c84549f6c50..18c176462ec 100644\n--- a/xdiff/xprepare.c\n+++ b/xdiff/xprepare.c\n@@ -72,7 +72,7 @@ static int xdl_init_classifier(xdlclassifier_t *cf, long size, long flags) {\n \tcf->flags = flags;\n \n \tcf->hbits = xdl_hashbits((unsigned int) size);\n-\tcf->hsize = 1 << cf->hbits;\n+\tcf->hsize = 1U << cf->hbits;\n \n \tif (xdl_cha_init(&cf->ncha, sizeof(xdlclass_t), size / 4 + 1) < 0) {\n \n@@ -174,7 +174,7 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_\n \t\tgoto abort;\n \n \thbits = xdl_hashbits((unsigned int) narec);\n-\thsize = 1 << hbits;\n+\thsize = 1U << hbits;\n \tif (!XDL_CALLOC_ARRAY(rhash, hsize))\n \t\tgoto abort;\n \n\nbase-commit: 063bcebf0c917140ca0e705cbe0fdea127e90086\n-- \ngitgitgadget\n"},{"id":"509807","messageId":"Z3a0LzChuIzmr7jw@google.com","threadId":"62662","inReplyTo":"pull.1849.git.git.1734488549111.gitgitgadget@gmail.com","subject":"Re: [PATCH] git: use U to denote unsigned to prevent UB","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2025-01-02T15:43:43Z","receivedAt":"2025-01-02T15:43:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSeija Kijin wrote:\n\n> 1 << can be UB if 1 ends up overflowing and\n> being assigned to an unsigned int or long.\n>\n> Signed-off-by: Seija Kijin <doremylover123@gmail.com>\n> ---\n>  builtin/checkout.c     |  2 +-\n>  builtin/merge-tree.c   |  4 ++--\n>  builtin/receive-pack.c |  2 +-\n>  color.c                |  4 ++--\n>  delta-islands.c        |  2 +-\n>  diff-delta.c           |  2 +-\n>  diff.c                 |  2 +-\n>  help.c                 |  2 +-\n>  imap-send.c            |  2 +-\n>  merge-ort.c            | 18 +++++++++---------\n>  xdiff/xhistogram.c     |  2 +-\n>  xdiff/xprepare.c       |  4 ++--\n>  12 files changed, 23 insertions(+), 23 deletions(-)\n\nThat said, most of these don't overflow, so it's not obvious this\nresults in higher quality or more readable code than before the patch.\n\nBy \"not obvious\" I don't mean that it _doesn't_, by the way, but just\nthat we don't have enough information to evaluate it here.  What\nmotivated writing this patch?  Is there a style guideline about it\nthat will remind us not to backslide in the future, for example?  Or\nis there a tool that notices?  Was there an example you ran into that\nled you to look for more examples?\n\nThis kind of information about context will make it easier for other\nin the project to ensure the patch does what it intends, and even more\nimportantly, to see if there are additional checks to add or other\ninstances that also need updating.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"509814","messageId":"xmqq5xmwuchl.fsf@gitster.g","threadId":"62662","inReplyTo":"pull.1849.git.git.1734488549111.gitgitgadget@gmail.com","subject":"Re: [PATCH] git: use U to denote unsigned to prevent UB","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-02T21:33:58Z","receivedAt":"2025-01-02T21:34:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"AreaZR via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Seija Kijin <doremylover123@gmail.com>\n>\n> 1 << can be UB if 1 ends up overflowing and\n> being assigned to an unsigned int or long.\n\n * Spell out what you meant by \"UB\".\n\n * \"1 ends up overflowing\"?  One is one; as long as you have two\n   bits, it won't overflow.  This needs rewriting.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 5e5afa0f267..a636e71e05c 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -223,7 +223,7 @@ static int check_stages(unsigned stages, const struct cache_entry *ce, int pos)\n>  \t\tce = the_repository->index->cache[pos];\n>  \t\tif (strcmp(name, ce->name))\n>  \t\t\tbreak;\n> -\t\tseen |= (1 << ce_stage(ce));\n> +\t\tseen |= (1U << ce_stage(ce));\n\nHere \"seen\" is \"unsigned\" initialized to 0.  Matching the type of\nthe value that is assigned with an explicit U does make sense, but\nas Jonathan already pointed out elsewhere, as ce_stage() cannot be\nmore than 3, this would never overflow.\n\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index c5ed472967a..d0104dfa0c7 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -270,13 +270,13 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])\n>  \tunsigned dirmask = 0, mask = 0;\n>  \n>  \tfor (i = 0; i < 3; i++) {\n> -\t\tmask |= (1 << i);\n> +\t\tmask |= (1U << i);\n\nDitto.\n\n>  \t\t/*\n>  \t\t * Treat missing entries as directories so that we return\n>  \t\t * after unresolved_directory has handled this.\n>  \t\t */\n>  \t\tif (!n[i].mode || S_ISDIR(n[i].mode))\n> -\t\t\tdirmask |= (1 << i);\n> +\t\t\tdirmask |= (1U << i);\n\nDitto.\n\nThere are a few instances that left shifts by 31-bit, which requires\n(1 * 2 ** 31) to be representable in the result type of signed int\nif we want to avoid an undefined behaviour,, but even if your signed\nint were wider than 32-bit, it is a good hygiene to write your bit\nshift as (1U << shift_count).\n\nSo I am not opposed to these changes.  The guiding principle should\nprobably be \"bit patterns should by unsigned by default, unless you\nhave a strong and valid reason to use signed\" (and the only single\nplausible reason being when you take advantage of fact that the sign\nbit is propagated if you shift right); as most of the changed code\npaths do deal with a signed result that is representable and does\nnot risk any undefined behaviour, it is an inappropriate rationale\nto justify this particular patch, I would think.\n"}]}