{"thread":{"id":"52693","subject":"[PATCH 0/4] more clang/sanitizer fixes","startedAt":"2020-01-25T05:35:45Z","lastAt":"2020-01-29T05:46:50Z","messageCount":17,"participants":["Jeff King","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"390444","messageId":"20200125053542.GA744596@coredump.intra.peff.net","threadId":"52693","inReplyTo":null,"subject":"[PATCH 0/4] more clang/sanitizer fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-25T05:35:42Z","receivedAt":"2020-01-25T05:35:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are some more sanitizer fixes, this time from trying with clang-11.\nThese should go on top of jk/asan-build-fix if you want to get a\nsuccessful run with \"make CC=clang-11 SANITIZE=address,undefined test\".\n\nI don't think any of them is indicative of a current bug in practice,\nbut the UBSan stuff makes me worried that an aggressive compiler might\ndo the wrong thing in some case.\n\n [1/4]: merge-recursive: silence -Wxor-used-as-pow warning\n [2/4]: avoid computing zero offsets from NULL pointer\n [3/4]: xdiff: avoid computing non-zero offset from NULL pointer\n [4/4]: obstack: avoid computing offsets from NULL pointer\n\n compat/obstack.h | 6 ++++--\n merge-recursive.c | 19 ++++++++++++++-----\n sequencer.c | 6 +++---\n unpack-trees.c | 2 +-\n xdiff-interface.c | 12 ++++++++----\n 5 files changed, 30 insertions(+), 15 deletions(-)\n\n-Peff\n"},{"id":"390445","messageId":"20200125053723.GA744673@coredump.intra.peff.net","threadId":"52693","inReplyTo":"20200125053542.GA744596@coredump.intra.peff.net","subject":"[PATCH 1/4] merge-recursive: silence -Wxor-used-as-pow warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-25T05:37:23Z","receivedAt":"2020-01-25T05:37:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The merge-recursive code uses stage number constants like this:\n\n add = &ci->ren1->dst_entry->stages[2 ^ 1];\n ...\n add = &ci->ren2->dst_entry->stages[3 ^ 1];\n\nThe xor has the effect of flipping the \"1\" bit, so that \"2 ^ 1\" becomes\n\"3\" and \"3 ^ 1\" becomes \"2\", which correspond to the \"ours\" and \"theirs\"\nstages respectively.\n\nUnfortunately, clang-10 and up issue a warning for this code:\n\n merge-recursive.c:1759:40: error: result of '2 ^ 1' is 3; did you mean '1 << 1' (2)? [-Werror,-Wxor-used-as-pow]\n add = &ci->ren1->dst_entry->stages[2 ^ 1];\n ~~^~~\n 1 << 1\n merge-recursive.c:1759:40: note: replace expression with '0x2 ^ 1' to silence this warning\n\nWe could silence it by using 0x2, as the compiler mentions. Or by just\nusing the constants \"2\" and \"3\" directly. But after digging into it, I\ndo think this bit-flip is telling us something. If we just wrote:\n\n add = &ci->ren2->dst_entry->stages[2];\n\nfor the second one, you might think that \"ren2\" and \"2\" correspond. But\nthey don't. The logic is: ren2 is theirs, which is stage 3, but we\nare interested in the opposite side's stage, so flip it to 2.\n\nSo let's keep the bit-flipping, but let's also put it behind a named\nfunction, which will make its purpose a bit clearer. This also has the\nside effect of suppressing the warning (and an optimizing compiler\nshould be able to easily turn it into a constant as before).\n\nSigned-off-by: Jeff King \n---\nIt might be more readable still to #define STAGE_OURS and STAGE_THEIRS,\nbut the use of bare 2/3 is pervasive throughout the file. We'd probably\nwant to change it consistently, and perhaps call \"ren1\" \"ren_ours\" or\nsomething like that. I'm not overly familiar with this code, so I tried\nto keep it to what I found an obvious improvement. But I'm also happy to\ngo the \"0x2 ^ 1\" route if merge-recursive experts prefer that.\n\n merge-recursive.c | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 10dca5644b..e6aedd3cab 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1712,6 +1712,15 @@ static char *find_path_for_conflict(struct merge_options *opt,\n \treturn new_path;\n }\n \n+/*\n+ * Toggle the stage number between \"ours\" and \"theirs\" (2 and 3) by flipping\n+ * the 1-bit.\n+ */\n+static inline int flip_stage(int stage)\n+{\n+\treturn stage ^ 1;\n+}\n+\n static int handle_rename_rename_1to2(struct merge_options *opt,\n \t\t\t\t struct rename_conflict_info *ci)\n {\n@@ -1756,14 +1765,14 @@ static int handle_rename_rename_1to2(struct merge_options *opt,\n \t\t * such cases, we should keep the added file around,\n \t\t * resolving the conflict at that path in its favor.\n \t\t */\n-\t\tadd = &ci->ren1->dst_entry->stages[2 ^ 1];\n+\t\tadd = &ci->ren1->dst_entry->stages[flip_stage(2)];\n \t\tif (is_valid(add)) {\n \t\t\tif (update_file(opt, 0, add, a->path))\n \t\t\t\treturn -1;\n \t\t}\n \t\telse\n \t\t\tremove_file_from_index(opt->repo->index, a->path);\n-\t\tadd = &ci->ren2->dst_entry->stages[3 ^ 1];\n+\t\tadd = &ci->ren2->dst_entry->stages[flip_stage(3)];\n \t\tif (is_valid(add)) {\n \t\t\tif (update_file(opt, 0, add, b->path))\n \t\t\t\treturn -1;\n@@ -1776,7 +1785,7 @@ static int handle_rename_rename_1to2(struct merge_options *opt,\n \t\t * rename/add collision. If not, we can write the file out\n \t\t * to the specified location.\n \t\t */\n-\t\tadd = &ci->ren1->dst_entry->stages[2 ^ 1];\n+\t\tadd = &ci->ren1->dst_entry->stages[flip_stage(2)];\n \t\tif (is_valid(add)) {\n \t\t\tadd->path = mfi.blob.path = a->path;\n \t\t\tif (handle_file_collision(opt, a->path,\n@@ -1797,7 +1806,7 @@ static int handle_rename_rename_1to2(struct merge_options *opt,\n \t\t\t\treturn -1;\n \t\t}\n \n-\t\tadd = &ci->ren2->dst_entry->stages[3 ^ 1];\n+\t\tadd = &ci->ren2->dst_entry->stages[flip_stage(3)];\n \t\tif (is_valid(add)) {\n \t\t\tadd->path = mfi.blob.path = b->path;\n \t\t\tif (handle_file_collision(opt, b->path,\n@@ -1846,7 +1855,7 @@ static int handle_rename_rename_2to1(struct merge_options *opt,\n \tpath_side_1_desc = xstrfmt(\"version of %s from %s\", path, a->path);\n \tpath_side_2_desc = xstrfmt(\"version of %s from %s\", path, b->path);\n \tostage1 = ci->ren1->branch == opt->branch1 ? 3 : 2;\n-\tostage2 = ostage1 ^ 1;\n+\tostage2 = flip_stage(ostage1);\n \tci->ren1->src_entry->stages[ostage1].path = a->path;\n \tci->ren2->src_entry->stages[ostage2].path = b->path;\n \tif (merge_mode_and_contents(opt, a, c1,\n-- \n2.25.0.421.gb74d19af79\n\n"},{"id":"390446","messageId":"20200125053834.GB744673@coredump.intra.peff.net","threadId":"52693","inReplyTo":"20200125053542.GA744596@coredump.intra.peff.net","subject":"[PATCH 2/4] avoid computing zero offsets from NULL pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-25T05:38:34Z","receivedAt":"2020-01-25T05:38:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The Undefined Behavior Sanitizer in clang-11 seems to have learned a new\ntrick: it complains about computing offsets from a NULL pointer, even if\nthat offset is 0. This causes numerous test failures. For example, from\nt1090:\n\n unpack-trees.c:1355:41: runtime error: applying zero offset to null pointer\n ...\n not ok 6 - in partial clone, sparse checkout only fetches needed blobs\n\nThe code in question looks like this:\n\n struct cache_entry **cache_end = cache + nr;\n ...\n while (cache != cache_end)\n\nand we sometimes pass in a NULL and 0 for \"cache\" and \"nr\". This is\nconceptually fine, as \"cache_end\" would be equal to \"cache\" in this\ncase, and we wouldn't enter the loop at all. But computing even a zero\noffset violates the C standard. And given the fact that UBSan is\nnoticing this behavior, this might be a potential problem spot if the\ncompiler starts making unexpected assumptions based on undefined\nbehavior.\n\nSo let's just avoid it, which is pretty easy. In some cases we can just\nswitch to iterating with a numeric index (as we do in sequencer.c here).\nIn other cases (like the cache_end one) the use of an end pointer is\nmore natural; we can keep that by just explicitly checking for NULL when\nassigning the end pointer.\n\nSigned-off-by: Jeff King \n---\n sequencer.c | 6 +++---\n unpack-trees.c | 2 +-\n xdiff-interface.c | 4 ++--\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b9dbf1adb0..4d31ec3296 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -588,7 +588,7 @@ static int do_recursive_merge(struct repository *r,\n \tstruct merge_options o;\n \tstruct tree *next_tree, *base_tree, *head_tree;\n \tint clean;\n-\tchar **xopt;\n+\tint i;\n \tstruct lock_file index_lock = LOCK_INIT;\n \n \tif (repo_hold_locked_index(r, &index_lock, LOCK_REPORT_ON_ERROR) < 0)\n@@ -608,8 +608,8 @@ static int do_recursive_merge(struct repository *r,\n \tnext_tree = next ? get_commit_tree(next) : empty_tree(r);\n \tbase_tree = base ? get_commit_tree(base) : empty_tree(r);\n \n-\tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n-\t\tparse_merge_opt(&o, *xopt);\n+\tfor (i = 0; i < opts->xopts_nr; i++)\n+\t\tparse_merge_opt(&o, opts->xopts[i]);\n \n \tclean = merge_trees(&o,\n \t\t\t head_tree,\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex d5f4d997da..b4292d2be8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1352,7 +1352,7 @@ static int clear_ce_flags_1(struct index_state *istate,\n \t\t\t enum pattern_match_result default_match,\n \t\t\t int progress_nr)\n {\n-\tstruct cache_entry **cache_end = cache + nr;\n+\tstruct cache_entry **cache_end = cache ? cache + nr : cache;\n \n \t/*\n \t * Process all entries that have the given prefix and meet\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 8509f9ea22..2f1fe48512 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -84,8 +84,8 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)\n {\n \tconst int blk = 1024;\n \tlong trimmed = 0, recovered = 0;\n-\tchar *ap = a->ptr + a->size;\n-\tchar *bp = b->ptr + b->size;\n+\tchar *ap = a->ptr ? a->ptr + a->size : a->ptr;\n+\tchar *bp = b->ptr ? b->ptr + b->size : b->ptr;\n \tlong smaller = (a->size < b->size) ? a->size : b->size;\n \n \twhile (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {\n-- \n2.25.0.421.gb74d19af79\n\n"},{"id":"390447","messageId":"20200125053929.GC744673@coredump.intra.peff.net","threadId":"52693","inReplyTo":"20200125053542.GA744596@coredump.intra.peff.net","subject":"[PATCH 3/4] xdiff: avoid computing non-zero offset from NULL pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-25T05:39:29Z","receivedAt":"2020-01-25T05:39:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As with the previous commit, clang-11's UBSan complains about computing\noffsets from a NULL pointer, causing some tests to fail. In this case,\nthough, we're actually computing a non-zero offset, which is even more\ndubious. From t7810:\n\n xdiff-interface.c:268:14: runtime error: applying non-zero offset 1 to null pointer\n ...\n not ok 131 - grep -p with userdiff\n\nThe problem is our parsing of the funcname config. We count the number\nof lines in the string, allocate an array, and then loop over our\nallocated entries, parsing each line and moving our cursor to one past\nthe trailing newline for the next iteration.\n\nBut the final line will not generally have a trailing newline (since\nit's a config value), and hence we go to one past NULL. In practice this\nis OK, since our loop should terminate before we look at the value. But\neven computing such an invalid pointer technically violates the\nstandard.\n\nWe can fix it by leaving the pointer at NULL if we're at the end, rather\nthan one-past. And while we're thinking about it, we can also document\nthe variant by asserting that our initial line-count matches the\nsecond-pass of parsing.\n\nSigned-off-by: Jeff King \n---\n xdiff-interface.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 2f1fe48512..3cd2ac2855 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -250,9 +250,13 @@ void xdiff_set_find_func(xdemitconf_t *xecfg, const char *value, int cflags)\n \tALLOC_ARRAY(regs->array, regs->nr);\n \tfor (i = 0; i < regs->nr; i++) {\n \t\tstruct ff_reg *reg = regs->array + i;\n-\t\tconst char *ep = strchr(value, '\\n'), *expression;\n+\t\tconst char *ep, *expression;\n \t\tchar *buffer = NULL;\n \n+\t\tif (!value)\n+\t\t\tBUG(\"mismatch between line count and parsing\");\n+\t\tep = strchr(value, '\\n');\n+\n \t\treg->negate = (*value == '!');\n \t\tif (reg->negate && i == regs->nr - 1)\n \t\t\tdie(\"Last expression must not be negated: %s\", value);\n@@ -265,7 +269,7 @@ void xdiff_set_find_func(xdemitconf_t *xecfg, const char *value, int cflags)\n \t\tif (regcomp(®->re, expression, cflags))\n \t\t\tdie(\"Invalid regexp to look for hunk header: %s\", expression);\n \t\tfree(buffer);\n-\t\tvalue = ep + 1;\n+\t\tvalue = ep ? ep + 1 : NULL;\n \t}\n }\n \n-- \n2.25.0.421.gb74d19af79\n\n"},{"id":"390448","messageId":"20200125054117.GD744673@coredump.intra.peff.net","threadId":"52693","inReplyTo":"20200125053542.GA744596@coredump.intra.peff.net","subject":"[PATCH 4/4] obstack: avoid computing offsets from NULL pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-25T05:41:17Z","receivedAt":"2020-01-25T05:41:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As with the previous two commits, UBSan with clang-11 complains about\ncomputing offsets from a NULL pointer. The failures in t4013 (and\nelsewhere) look like this:\n\n kwset.c:102:23: runtime error: applying non-zero offset 107820859019600 to null pointer\n ...\n not ok 79 - git log -SF master # magic is (not used)\n\nThat line of kwset.c is not enlightening:\n\n ... = obstack_alloc(&kwset->obstack, sizeof (struct trie));\n\nbecause obstack is implemented almost entirely in macros, and the actual\nproblem is five macros deep (I temporarily converted them to inline\nfunctions to get better compiler errors, which was tedious but worked\nreasonably well).\n\nThe actual problem is in these pointer-alignment macros:\n\n /* If B is the base of an object addressed by P, return the result of\n aligning P to the next multiple of A + 1. B and P must be of type\n char *. A + 1 must be a power of 2. */\n\n #define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A)))\n\n /* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case\n where pointers can be converted to integers, aligned as integers,\n and converted back again. If PTR_INT_TYPE is narrower than a\n pointer (e.g., the AS/400), play it safe and compute the alignment\n relative to B. Otherwise, use the faster strategy of computing the\n alignment relative to 0. */\n\n #define __PTR_ALIGN(B, P, A) \\\n __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \\\n P, A)\n\nIf we have a sufficiently-large integer pointer type, then we do the\ncomputation using a NULL pointer constant. That turns __BPTR_ALIGN()\ninto something like:\n\n NULL + (P - NULL + A) & ~A\n\nand UBSan is complaining about adding the full value of P to that\ninitial NULL. We can fix this by doing our math as an integer type, and\nthen casting the result back to a pointer. The problem case only happens\nwhen we know that the integer type is large enough, so there should be\nno issue with truncation.\n\nAnother option would be just simplify out all the 0's from\n__BPTR_ALIGN() for the NULL-pointer case. That probably wouldn't work\nfor a platform where the NULL pointer isn't all-zeroes, but Git already\nwouldn't work on such a platform (due to our use of memset to set\npointers in structs to NULL). But I tried here to keep as close to the\noriginal as possible.\n\nSigned-off-by: Jeff King \n---\n compat/obstack.h | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/obstack.h b/compat/obstack.h\nindex 01e7c81840..fbcf68c67c 100644\n--- a/compat/obstack.h\n+++ b/compat/obstack.h\n@@ -135,8 +135,10 @@ extern \"C\" {\n alignment relative to 0. */\n \n #define __PTR_ALIGN(B, P, A)\t\t\t\t\t\t \\\n- __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \\\n-\t\tP, A)\n+ (sizeof (PTR_INT_TYPE) < sizeof(void *) ? \\\n+ __BPTR_ALIGN((B), (P), (A)) : \\\n+ (void *)__BPTR_ALIGN((PTR_INT_TYPE)0, (PTR_INT_TYPE)(P), (A)) \\\n+ )\n \n #include \n \n-- \n2.25.0.421.gb74d19af79\n"},{"id":"390449","messageId":"20200125054429.GA745003@coredump.intra.peff.net","threadId":"52693","inReplyTo":"20200125054117.GD744673@coredump.intra.peff.net","subject":"[PATCH v2 4/4] obstack: avoid computing offsets from NULL pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-25T05:44:29Z","receivedAt":"2020-01-25T05:44:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 25, 2020 at 12:41:18AM -0500, Jeff King wrote:\n\n> diff --git a/compat/obstack.h b/compat/obstack.h\n> index 01e7c81840..fbcf68c67c 100644\n> --- a/compat/obstack.h\n> +++ b/compat/obstack.h\n> @@ -135,8 +135,10 @@ extern \"C\" {\n> alignment relative to 0. */\n> \n> #define __PTR_ALIGN(B, P, A)\t\t\t\t\t\t \\\n> - __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \\\n> -\t\tP, A)\n> + (sizeof (PTR_INT_TYPE) < sizeof(void *) ? \\\n> + __BPTR_ALIGN((B), (P), (A)) : \\\n> + (void *)__BPTR_ALIGN((PTR_INT_TYPE)0, (PTR_INT_TYPE)(P), (A)) \\\n> + )\n\nOops, I added in one change at the last minute but failed to commit it.\nThe first argument to __BPTR_ALIGN() should be:\n\n (PTR_INT_TYPE)(void *)0\n\nto ensure that it's a NULL pointer, even if the platform doesn't have\nan all-bits-zero NULL. Again, this is probably academic for Git's\ncodebase, but I was trying to keep as close to the original as possible.\n\nHere's a fixed patch.\n\n-- >8 --\nSubject: obstack: avoid computing offsets from NULL pointer\n\nAs with the previous two commits, UBSan with clang-11 complains about\ncomputing offsets from a NULL pointer. The failures in t4013 (and\nelsewhere) look like this:\n\n kwset.c:102:23: runtime error: applying non-zero offset 107820859019600 to null pointer\n ...\n not ok 79 - git log -SF master # magic is (not used)\n\nThat line is not enlightening:\n\n ... = obstack_alloc(&kwset->obstack, sizeof (struct trie));\n\nbecause obstack is implemented almost entirely in macros, and the actual\nproblem is five macros deep (I temporarily converted them to inline\nfunctions to get better compiler errors, which was tedious but worked\nreasonably well).\n\nThe actual problem is in these pointer-alignment macros:\n\n /* If B is the base of an object addressed by P, return the result of\n aligning P to the next multiple of A + 1. B and P must be of type\n char *. A + 1 must be a power of 2. */\n\n #define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A)))\n\n /* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case\n where pointers can be converted to integers, aligned as integers,\n and converted back again. If PTR_INT_TYPE is narrower than a\n pointer (e.g., the AS/400), play it safe and compute the alignment\n relative to B. Otherwise, use the faster strategy of computing the\n alignment relative to 0. */\n\n #define __PTR_ALIGN(B, P, A) \\\n __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \\\n P, A)\n\nIf we have a sufficiently-large integer pointer type, then we do the\ncomputation using a NULL pointer constant. That turns __BPTR_ALIGN()\ninto something like:\n\n NULL + (P - NULL + A) & ~A\n\nand UBSan is complaining about adding the full value of P to that\ninitial NULL. We can fix this by doing our math as an integer type, and\nthen casting the result back to a pointer. The problem case only happens\nwhen we know that the integer type is large enough, so there should be\nno issue with truncation.\n\nAnother option would be just simplify out all the 0's from\n__BPTR_ALIGN() for the NULL-pointer case. That probably wouldn't work\nfor a platform where the NULL pointer isn't all-zeroes, but Git already\nwouldn't work on such a platform (due to our use of memset to set\npointers in structs to NULL). But I tried here to keep as close to the\noriginal as possible.\n\nSigned-off-by: Jeff King \n---\n compat/obstack.h | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/obstack.h b/compat/obstack.h\nindex 01e7c81840..f90a46d9b9 100644\n--- a/compat/obstack.h\n+++ b/compat/obstack.h\n@@ -135,8 +135,10 @@ extern \"C\" {\n alignment relative to 0. */\n \n #define __PTR_ALIGN(B, P, A)\t\t\t\t\t\t \\\n- __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \\\n-\t\tP, A)\n+ (sizeof (PTR_INT_TYPE) < sizeof(void *) ? \\\n+ __BPTR_ALIGN((B), (P), (A)) : \\\n+ (void *)__BPTR_ALIGN((PTR_INT_TYPE)(void *)0, (PTR_INT_TYPE)(P), (A)) \\\n+ )\n \n #include \n \n-- \n2.25.0.421.gb74d19af79\n\n"},{"id":"390459","messageId":"xmqq4kwjcupj.fsf@gitster-ct.c.googlers.com","threadId":"52693","inReplyTo":"20200125053723.GA744673@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] merge-recursive: silence -Wxor-used-as-pow warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-25T17:27:36Z","receivedAt":"2020-01-25T17:29:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The merge-recursive code uses stage number constants like this:\n>\n> add = &ci->ren1->dst_entry->stages[2 ^ 1];\n> ...\n> add = &ci->ren2->dst_entry->stages[3 ^ 1];\n>\n> The xor has the effect of flipping the \"1\" bit, so that \"2 ^ 1\" becomes\n> \"3\" and \"3 ^ 1\" becomes \"2\", which correspond to the \"ours\" and \"theirs\"\n> stages respectively.\n>\n> Unfortunately, clang-10 and up issue a warning for this code:\n>\n> merge-recursive.c:1759:40: error: result of '2 ^ 1' is 3; did you mean '1 << 1' (2)? [-Werror,-Wxor-used-as-pow]\n> add = &ci->ren1->dst_entry->stages[2 ^ 1];\n> ~~^~~\n> 1 << 1\n> merge-recursive.c:1759:40: note: replace expression with '0x2 ^ 1' to silence this warning\n>\n> We could silence it by using 0x2, as the compiler mentions. Or by just\n> using the constants \"2\" and \"3\" directly. But after digging into it, I\n> do think this bit-flip is telling us something. If we just wrote:\n>\n> add = &ci->ren2->dst_entry->stages[2];\n>\n> for the second one, you might think that \"ren2\" and \"2\" correspond. But\n> they don't. The logic is: ren2 is theirs, which is stage 3, but we\n> are interested in the opposite side's stage, so flip it to 2.\n\nSo, the logical name for \"^1\" operator applied to 2 (ours) and 3\n(theirs) is \"the_other_side()\"? the_other_side(theirs) == ours.\n\nI would have written (5 - side) instead of (side ^ 1) if I were\nwriting this, though.\n\n> So let's keep the bit-flipping, but let's also put it behind a named\n> function, which will make its purpose a bit clearer. This also has the\n> side effect of suppressing the warning (and an optimizing compiler\n> should be able to easily turn it into a constant as before).\n\nOK. Now I see you named it flip_stage(), which is even better than\n\"the-other-side\" above. Makes sense.\n\nI still think ((2 + 3) - two_or_three_to_be_flipped) easier to\nreason about than the bit flipping, as the implementation detail,\nthough.\n\nThanks.\n"},{"id":"390466","messageId":"20200125195515.GB5519@coredump.intra.peff.net","threadId":"52693","inReplyTo":"xmqq4kwjcupj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/4] merge-recursive: silence -Wxor-used-as-pow warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-25T19:55:15Z","receivedAt":"2020-01-25T19:55:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 25, 2020 at 09:27:36AM -0800, Junio C Hamano wrote:\n\n> > So let's keep the bit-flipping, but let's also put it behind a named\n> > function, which will make its purpose a bit clearer. This also has the\n> > side effect of suppressing the warning (and an optimizing compiler\n> > should be able to easily turn it into a constant as before).\n> \n> OK. Now I see you named it flip_stage(), which is even better than\n> \"the-other-side\" above. Makes sense.\n> \n> I still think ((2 + 3) - two_or_three_to_be_flipped) easier to\n> reason about than the bit flipping, as the implementation detail,\n> though.\n\nYeah, the existing one relies on the coincidence that the two stages\ndiffer by a single bit (in another universe, they could well be stages\n\"3\" and \"4\").\n\nI don't overly care on the implementation either way, since it's now\nhidden in the helper. I mostly chose the bit-flip to match the existing\ncode, but I'd be happy to change it. Other people who actually work on\nmerge-recursive may have other opinions, though.\n\n-Peff\n"},{"id":"390468","messageId":"CABPp-BFp3VrYjD8KTqnOff0-CDY7qO3Au7GeUDCuA46ofv1GSg@mail.gmail.com","threadId":"52693","inReplyTo":"20200125195515.GB5519@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] merge-recursive: silence -Wxor-used-as-pow warning","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-25T20:50:38Z","receivedAt":"2020-01-25T20:50:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 25, 2020 at 11:55 AM Jeff King wrote:\n>\n> On Sat, Jan 25, 2020 at 09:27:36AM -0800, Junio C Hamano wrote:\n>\n> > > So let's keep the bit-flipping, but let's also put it behind a named\n> > > function, which will make its purpose a bit clearer. This also has the\n> > > side effect of suppressing the warning (and an optimizing compiler\n> > > should be able to easily turn it into a constant as before).\n> >\n> > OK. Now I see you named it flip_stage(), which is even better than\n> > \"the-other-side\" above. Makes sense.\n> >\n> > I still think ((2 + 3) - two_or_three_to_be_flipped) easier to\n> > reason about than the bit flipping, as the implementation detail,\n> > though.\n>\n> Yeah, the existing one relies on the coincidence that the two stages\n> differ by a single bit (in another universe, they could well be stages\n> \"3\" and \"4\").\n>\n> I don't overly care on the implementation either way, since it's now\n> hidden in the helper. I mostly chose the bit-flip to match the existing\n> code, but I'd be happy to change it. Other people who actually work on\n> merge-recursive may have other opinions, though.\n\nInteresting. In merge-ort (my in-development attempt at a replacement\nfor merge-recursive), I'm currently storing the stages in an array\nwith indices 0-2 rather than the 1-3 used by merge-recursive. This\nremoves the empty/unused array entry at the beginning, and also works\na bit better with the masks that traverse_trees() returns (as 1< Interesting. In merge-ort (my in-development attempt at a replacement\n> for merge-recursive), I'm currently storing the stages in an array\n> with indices 0-2 rather than the 1-3 used by merge-recursive. This\n> removes the empty/unused array entry at the beginning, and also works\n> a bit better with the masks that traverse_trees() returns (as 1< corresponds to the bit in the mask from the traverse_trees()\n> callback). In that scheme, bitflip won't work, but the subtraction\n> idea still does. So, I'd tend to agree with Junio, but I think the\n> helper you added here is probably the more important improvement.\n\nOK, that's three people who think the subtraction is more obvious. I\nagree it's not that big a deal now that it's hidden in the helper (and\nthe code may eventually go away anyway), but it's easy enough to fix it\nwhile we're thinking about it.\n\nPatch is below (as an incremental on top attributed to Junio, who\nproposed it; but it would be fine to squash it in with a Helped-by,\ntoo).\n\n> [1] merge-ort still isn't functional yet other than in extremely\n> narrow circumstances, I'm still experimenting with the data\n> structures, and I've written several hundred lines of code and then\n> thrown it all away at least once -- and may do so again. Whenever I\n> find a useful patch I can separate and submit upstream, I have been\n> doing so, but until the risk of another complete rewrite goes down,\n> there's no point in me sending my half-baked ideas in for review.\n> They need to be at least three-quarters baked first. :-)\n\nThank you, by the way, for all of the work you have done on\nmerge-recursive. I generally find it one of the scariest bits of the\ncode to look at, so I am happy to be able to punt it off to you. :)\n\n-- >8 --\nFrom: Junio C Hamano \nSubject: [PATCH] merge-recursive: use subtraction to flip stage\n\nThe flip_stage() helper uses a bit-flipping xor to switch between \"2\"\nand \"3\". While clever, this relies on a property of those two numbers\nthat is mostly coincidence. Let's write it as a subtraction; that's more\nclear and would extend to other numbers if somebody copies the logic.\n\nSigned-off-by: Jeff King \n---\n merge-recursive.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e6aedd3cab..aee1769a7a 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1713,12 +1713,11 @@ static char *find_path_for_conflict(struct merge_options *opt,\n }\n \n /*\n- * Toggle the stage number between \"ours\" and \"theirs\" (2 and 3) by flipping\n- * the 1-bit.\n+ * Toggle the stage number between \"ours\" and \"theirs\" (2 and 3).\n */\n static inline int flip_stage(int stage)\n {\n-\treturn stage ^ 1;\n+\treturn (2 + 3) - stage;\n }\n \n static int handle_rename_rename_1to2(struct merge_options *opt,\n-- \n2.25.0.430.g8dfc7de6f7\n\n"},{"id":"390576","messageId":"xmqqblqobtf8.fsf@gitster-ct.c.googlers.com","threadId":"52693","inReplyTo":"20200125195515.GB5519@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] merge-recursive: silence -Wxor-used-as-pow warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-27T19:17:31Z","receivedAt":"2020-01-27T19:17:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't overly care on the implementation either way, since it's now\n> hidden in the helper. I mostly chose the bit-flip to match the existing\n> code, but I'd be happy to change it.\n\nI don't, either, and as a step in this series, it makes perfect\nsense to keep the implementation detail the same in the new helper.\n\nMaking it more readable would be a separate issue.\n\nThanks.\n"},{"id":"390579","messageId":"xmqq7e1cbr9w.fsf@gitster-ct.c.googlers.com","threadId":"52693","inReplyTo":"20200125053834.GB744673@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] avoid computing zero offsets from NULL pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-27T20:03:55Z","receivedAt":"2020-01-27T20:04:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> switch to iterating with a numeric index (as we do in sequencer.c here).\n> In other cases (like the cache_end one) the use of an end pointer is\n> more natural; we can keep that by just explicitly checking for NULL when\n> assigning the end pointer.\n>\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index 8509f9ea22..2f1fe48512 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -84,8 +84,8 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)\n> {\n> \tconst int blk = 1024;\n> \tlong trimmed = 0, recovered = 0;\n> -\tchar *ap = a->ptr + a->size;\n> -\tchar *bp = b->ptr + b->size;\n> +\tchar *ap = a->ptr ? a->ptr + a->size : a->ptr;\n> +\tchar *bp = b->ptr ? b->ptr + b->size : b->ptr;\n> \tlong smaller = (a->size < b->size) ? a->size : b->size;\n> \n> \twhile (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {\n\nIsn't it a bug for a->ptr or b->ptr to be NULL here? Even if we\nmanage to assign ap = a->ptr = NULL without complaints, how would\nthat memcmp work?\n\nIs it that the corresponding .size would always be 0 if .ptr is NULL\nthat protects us?\n\nA bit puzzled.\n"},{"id":"390583","messageId":"20200127211933.GA3794@coredump.intra.peff.net","threadId":"52693","inReplyTo":"xmqq7e1cbr9w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] avoid computing zero offsets from NULL pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-27T21:19:33Z","receivedAt":"2020-01-27T21:19:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 27, 2020 at 12:03:55PM -0800, Junio C Hamano wrote:\n\n> > diff --git a/xdiff-interface.c b/xdiff-interface.c\n> > index 8509f9ea22..2f1fe48512 100644\n> > --- a/xdiff-interface.c\n> > +++ b/xdiff-interface.c\n> > @@ -84,8 +84,8 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)\n> > {\n> > \tconst int blk = 1024;\n> > \tlong trimmed = 0, recovered = 0;\n> > -\tchar *ap = a->ptr + a->size;\n> > -\tchar *bp = b->ptr + b->size;\n> > +\tchar *ap = a->ptr ? a->ptr + a->size : a->ptr;\n> > +\tchar *bp = b->ptr ? b->ptr + b->size : b->ptr;\n> > \tlong smaller = (a->size < b->size) ? a->size : b->size;\n> > \n> > \twhile (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {\n> \n> Isn't it a bug for a->ptr or b->ptr to be NULL here? Even if we\n> manage to assign ap = a->ptr = NULL without complaints, how would\n> that memcmp work?\n> \n> Is it that the corresponding .size would always be 0 if .ptr is NULL\n> that protects us?\n> \n> A bit puzzled.\n\nYes, that's what's happening; all of the cases in this first patch are\ndealing with \"NULL + 0\". Which isn't to say somebody couldn't pass in an\nmmfile_t with NULL and a non-zero size, but obviously that would be a\nbug. Before my patch that would be a segfault, but afterwards we'd\nquietly treat it as if the size were zero.\n\nIf we want to be more defensive, we could do something like this:\n\n /* dual inline/macro magic to avoid evaluating ptr twice but knowing\n * enough about the type of *ptr to get the size. */\n #define SAFE_END_PTR(ptr, len) safe_end_ptr(ptr, len, sizeof(*ptr))\n static inline void *safe_end_ptr(void *ptr, size_t nr, size_t elem_size)\n {\n\tif (!ptr) {\n\t\tif (nr)\n\t\t\tBUG(\"non-zero size coupled with NULL pointer\");\n\t\treturn NULL;\n\t}\n\treturn (char *)ptr + nr * elem_size;\n }\n\n ...\n char *ap = SAFE_END_PTR(a->ptr, a->size);\n\nI'm not sure if it's worth it, though.\n\nYet another alternative is to consider it a bug to use an mmfile_t with\na NULL pointer, figure out where that's being set up, and fix it.\n\nAs an aside, I also wondered whether we could run into problems with\n\"memcmp(NULL, ..., 0)\", which is also undefined behavior. But we don't\nhere because the first half of the while() condition wouldn't trigger.\n\n-Peff\n"},{"id":"390643","messageId":"xmqqtv4f8nlm.fsf@gitster-ct.c.googlers.com","threadId":"52693","inReplyTo":"20200127211933.GA3794@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] avoid computing zero offsets from NULL pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-28T18:03:49Z","receivedAt":"2020-01-28T18:04:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > \tconst int blk = 1024;\n>> > \tlong trimmed = 0, recovered = 0;\n>> > -\tchar *ap = a->ptr + a->size;\n>> > -\tchar *bp = b->ptr + b->size;\n>> > +\tchar *ap = a->ptr ? a->ptr + a->size : a->ptr;\n>> > +\tchar *bp = b->ptr ? b->ptr + b->size : b->ptr;\n>> > \tlong smaller = (a->size < b->size) ? a->size : b->size;\n>> > \n>> > \twhile (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {\n>> \n>> Isn't it a bug for a->ptr or b->ptr to be NULL here? Even if we\n>> manage to assign ap = a->ptr = NULL without complaints, how would\n>> that memcmp work?\n>> \n>> Is it that the corresponding .size would always be 0 if .ptr is NULL\n>> that protects us?\n>\n> If we want to be more defensive, we could do something like this:\n>\n> /* dual inline/macro magic to avoid evaluating ptr twice but knowing\n> * enough about the type of *ptr to get the size. */\n> #define SAFE_END_PTR(ptr, len) safe_end_ptr(ptr, len, sizeof(*ptr))\n> static inline void *safe_end_ptr(void *ptr, size_t nr, size_t elem_size)\n> {\n> \tif (!ptr) {\n> \t\tif (nr)\n> \t\t\tBUG(\"non-zero size coupled with NULL pointer\");\n> \t\treturn NULL;\n> \t}\n> \treturn (char *)ptr + nr * elem_size;\n> }\n>\n> ...\n> char *ap = SAFE_END_PTR(a->ptr, a->size);\n>\n> I'm not sure if it's worth it, though.\n\nAs long as we make it clear to those who add new callers that\nany mmfile_t with .ptr==NULL must come with .size==0, it is fine.\n\n> Yet another alternative is to consider it a bug to use an mmfile_t with\n> a NULL pointer, figure out where that's being set up, and fix it.\n\nBut that would still require us to make it clear to those who add\nnew callers that mmfile_t with .ptr==NULL is a bug, and the current\ncallers must be using that as it is convenient for them, I presume,\nso I think a simple comment should probably be sufficient.\n\n> As an aside, I also wondered whether we could run into problems with\n> \"memcmp(NULL, ..., 0)\", which is also undefined behavior. But we don't\n> here because the first half of the while() condition wouldn't trigger.\n\nYes, although the details slightly differ ;-)\n\nWhat is problematic actually is \"memcmp(NULL - 1024, ..., 1024)\",\nwhich is guarded with \"1024 + trimmed <= smaller &&\" that will never\nbe true as long as \"mmfile_t with .ptr==NULL must have .size==0\"\nholds true, right?\n\nThanks.\n"},{"id":"390682","messageId":"20200129023146.GA596379@coredump.intra.peff.net","threadId":"52693","inReplyTo":"xmqqtv4f8nlm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] avoid computing zero offsets from NULL pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-29T02:31:46Z","receivedAt":"2020-01-29T02:31:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 28, 2020 at 10:03:49AM -0800, Junio C Hamano wrote:\n\n> > I'm not sure if it's worth it, though.\n> \n> As long as we make it clear to those who add new callers that\n> any mmfile_t with .ptr==NULL must come with .size==0, it is fine.\n\nTBH, I'm not sure it _is_ fine. The concept that it's safe for a ptr/len\npair to use NULL/0 is true in a lot of places, but the mmfile struct\ngets used in a lot of places, much of which is xdiff code we didn't\nwrite. I have no idea if that assumption holds everywhere.\n\nWe'd be fixing this one spot, and that's enough to make the tests happy\nwith UBSan. But I don't know if it's something we ought to be\nrecommending.\n\n> > Yet another alternative is to consider it a bug to use an mmfile_t with\n> > a NULL pointer, figure out where that's being set up, and fix it.\n> \n> But that would still require us to make it clear to those who add\n> new callers that mmfile_t with .ptr==NULL is a bug, and the current\n> callers must be using that as it is convenient for them, I presume,\n> so I think a simple comment should probably be sufficient.\n\nYep, but it's not much different than the hundreds of other function\ninterfaces we have where sometimes you can pass NULL and sometimes not.\n\nSo anyway. What do we want to do here? The fix I have? Something more\nelaborate and reusable? Or perhaps just switch it to:\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 3cd2ac2855..4d20069302 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -84,8 +84,8 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)\n {\n \tconst int blk = 1024;\n \tlong trimmed = 0, recovered = 0;\n-\tchar *ap = a->ptr ? a->ptr + a->size : a->ptr;\n-\tchar *bp = b->ptr ? b->ptr + b->size : b->ptr;\n+\tchar *ap = a->size ? a->ptr + a->size : a->ptr;\n+\tchar *bp = b->size ? b->ptr + b->size : b->ptr;\n \tlong smaller = (a->size < b->size) ? a->size : b->size;\n \n \twhile (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {\n\nBy checking \"size\" instead of \"ptr\", then we know that the addition is a\nnoop. And we'd continue to catch a NULL pointer mixed with a non-zero\nlength (as a segfault). And a non-NULL pointer with a zero length does\nthe right thing.\n\n> > As an aside, I also wondered whether we could run into problems with\n> > \"memcmp(NULL, ..., 0)\", which is also undefined behavior. But we don't\n> > here because the first half of the while() condition wouldn't trigger.\n> \n> Yes, although the details slightly differ ;-)\n> \n> What is problematic actually is \"memcmp(NULL - 1024, ..., 1024)\",\n> which is guarded with \"1024 + trimmed <= smaller &&\" that will never\n> be true as long as \"mmfile_t with .ptr==NULL must have .size==0\"\n> holds true, right?\n\nYes, because \"smaller\" would always be \"0\". And that part of the code\nalways uses a 1024-size blk, so it would never have passed \"0\" to memcmp\nanyway.\n\n-Peff\n"},{"id":"390696","messageId":"xmqqblqm7sg8.fsf@gitster-ct.c.googlers.com","threadId":"52693","inReplyTo":"20200129023146.GA596379@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] avoid computing zero offsets from NULL pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-29T05:16:39Z","receivedAt":"2020-01-29T05:16:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yep, but it's not much different than the hundreds of other function\n> interfaces we have where sometimes you can pass NULL and sometimes not.\n>\n> So anyway. What do we want to do here? The fix I have? Something more\n> elaborate and reusable? Or perhaps just switch it to:\n\nMy preference was to take the patch as-is, as it was clear enough,\nbefore seeing this one ...\n\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index 3cd2ac2855..4d20069302 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -84,8 +84,8 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)\n> {\n> \tconst int blk = 1024;\n> \tlong trimmed = 0, recovered = 0;\n> -\tchar *ap = a->ptr ? a->ptr + a->size : a->ptr;\n> -\tchar *bp = b->ptr ? b->ptr + b->size : b->ptr;\n> +\tchar *ap = a->size ? a->ptr + a->size : a->ptr;\n> +\tchar *bp = b->size ? b->ptr + b->size : b->ptr;\n> \tlong smaller = (a->size < b->size) ? a->size : b->size;\n> \n> \twhile (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {\n>\n> By checking \"size\" instead of \"ptr\", then we know that the addition is a\n> noop. And we'd continue to catch a NULL pointer mixed with a non-zero\n> length (as a segfault). And a non-NULL pointer with a zero length does\n> the right thing.\n\nwhich makes quite a lot of sense.\n"},{"id":"390699","messageId":"20200129054647.GA611490@coredump.intra.peff.net","threadId":"52693","inReplyTo":"xmqqblqm7sg8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] avoid computing zero offsets from NULL pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-29T05:46:47Z","receivedAt":"2020-01-29T05:46:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 28, 2020 at 09:16:39PM -0800, Junio C Hamano wrote:\n\n> My preference was to take the patch as-is, as it was clear enough,\n> before seeing this one ...\n> [..]\n> which makes quite a lot of sense.\n\nYeah, about an hour after I wrote it, I thought \"gee, that's clearly\nbetter than the other way; I should have done that from the start\".\n\nSo here's a re-roll of the patch using that technique, with a note in\nthe commit message.\n\n-- >8 --\nSubject: [PATCH] avoid computing zero offsets from NULL pointer\n\nThe Undefined Behavior Sanitizer in clang-11 seems to have learned a new\ntrick: it complains about computing offsets from a NULL pointer, even if\nthat offset is 0. This causes numerous test failures. For example, from\nt1090:\n\n unpack-trees.c:1355:41: runtime error: applying zero offset to null pointer\n ...\n not ok 6 - in partial clone, sparse checkout only fetches needed blobs\n\nThe code in question looks like this:\n\n struct cache_entry **cache_end = cache + nr;\n ...\n while (cache != cache_end)\n\nand we sometimes pass in a NULL and 0 for \"cache\" and \"nr\". This is\nconceptually fine, as \"cache_end\" would be equal to \"cache\" in this\ncase, and we wouldn't enter the loop at all. But computing even a zero\noffset violates the C standard. And given the fact that UBSan is\nnoticing this behavior, this might be a potential problem spot if the\ncompiler starts making unexpected assumptions based on undefined\nbehavior.\n\nSo let's just avoid it, which is pretty easy. In some cases we can just\nswitch to iterating with a numeric index (as we do in sequencer.c here).\nIn other cases (like the cache_end one) the use of an end pointer is\nmore natural; we can keep that by just explicitly checking for the\nNULL/0 case when assigning the end pointer.\n\nNote that there are two ways you can write this latter case, checking\nfor the pointer:\n\n cache_end = cache ? cache + nr : cache;\n\nor the size:\n\n cache_end = nr ? cache + nr : cache;\n\nFor the case of a NULL/0 ptr/len combo, they are equivalent. But writing\nit the second way (as this patch does) has the property that if somebody\nwere to incorrectly pass a NULL pointer with a non-zero length, we'd\ncontinue to notice and segfault, rather than silently pretending the\nlength was zero.\n\nSigned-off-by: Jeff King \n---\n sequencer.c | 6 +++---\n unpack-trees.c | 2 +-\n xdiff-interface.c | 4 ++--\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b9dbf1adb0..4d31ec3296 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -588,7 +588,7 @@ static int do_recursive_merge(struct repository *r,\n \tstruct merge_options o;\n \tstruct tree *next_tree, *base_tree, *head_tree;\n \tint clean;\n-\tchar **xopt;\n+\tint i;\n \tstruct lock_file index_lock = LOCK_INIT;\n \n \tif (repo_hold_locked_index(r, &index_lock, LOCK_REPORT_ON_ERROR) < 0)\n@@ -608,8 +608,8 @@ static int do_recursive_merge(struct repository *r,\n \tnext_tree = next ? get_commit_tree(next) : empty_tree(r);\n \tbase_tree = base ? get_commit_tree(base) : empty_tree(r);\n \n-\tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n-\t\tparse_merge_opt(&o, *xopt);\n+\tfor (i = 0; i < opts->xopts_nr; i++)\n+\t\tparse_merge_opt(&o, opts->xopts[i]);\n \n \tclean = merge_trees(&o,\n \t\t\t head_tree,\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex d5f4d997da..a027504b56 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1352,7 +1352,7 @@ static int clear_ce_flags_1(struct index_state *istate,\n \t\t\t enum pattern_match_result default_match,\n \t\t\t int progress_nr)\n {\n-\tstruct cache_entry **cache_end = cache + nr;\n+\tstruct cache_entry **cache_end = nr ? cache + nr : cache;\n \n \t/*\n \t * Process all entries that have the given prefix and meet\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 8509f9ea22..99661d43c6 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -84,8 +84,8 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)\n {\n \tconst int blk = 1024;\n \tlong trimmed = 0, recovered = 0;\n-\tchar *ap = a->ptr + a->size;\n-\tchar *bp = b->ptr + b->size;\n+\tchar *ap = a->size ? a->ptr + a->size : a->ptr;\n+\tchar *bp = b->size ? b->ptr + b->size : b->ptr;\n \tlong smaller = (a->size < b->size) ? a->size : b->size;\n \n \twhile (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {\n-- \n2.25.0.515.g69a699b7aa\n\n"}]}