{"thread":{"id":"45768","subject":"[PATCH] cache-tree: reject entries with null sha1","startedAt":"2017-04-21T19:13:07Z","lastAt":"2017-05-03T19:48:47Z","messageCount":10,"participants":["Jeff King","Duy Nguyen","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"317510","messageId":"20170421184617.lc5bioa5px6ninrj@sigill.intra.peff.net","threadId":"45768","inReplyTo":null,"subject":"[PATCH] cache-tree: reject entries with null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-21T18:46:17Z","receivedAt":"2017-04-21T19:13:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generally disallow null sha1s from entering the index,\ndue to 4337b5856 (do not write null sha1s to on-disk index,\n2012-07-28). However, we loosened that in 83bd7437c\n(write_index: optionally allow broken null sha1s,\n2013-08-27) so that tools like filter-branch could be used\nto repair broken history.\n\nHowever, we should make sure that these broken entries do\nnot get propagated into new trees. For most entries, we'd\ncatch them with the missing-object check (since presumably\nthe null sha1 does not exist in our object database). But\ngitlink entries do not need reachability, so we may blindly\ncopy the entry into a bogus tree.\n\nThis patch rejects all null sha1s (with the same \"invalid\nentry\" message that missing objects get) when building trees\nfrom the index. It does so even for non-gitlinks, and even\nwhen \"write-tree\" is given the --missing-ok flag. The null\nsha1 is a special sentinel value that is already rejected in\ntrees by fsck; whether the object exists or not, it is an\nerror to put it in a tree.\n\nNote that for this to work, we must also avoid reusing an\nexisting cache-tree that contains the null sha1. This patch\ndoes so by just refusing to write out any cache tree when\nthe index contains a null sha1. This is blunter than we need\nto be; we could just reject the subtree that contains the\noffending entry. But it's not worth the complexity. The\nbehavior is unchanged unless you have a broken index entry,\nand even then we'd refuse the whole index write unless the\nemergency GIT_ALLOW_NULL_SHA1 is in use. And even then the\nend result is only a performance drop (any write-tree will\nhave to generate the whole cache-tree from scratch).\n\nThe tests bear some explanation.\n\nThe existing test in t7009 doesn't catch this problem,\nbecause our index-filter runs \"git rm --cached\", which will\ntry to rewrite the updated index and barf on the bogus\nentry. So we never even make it to write-tree. The new test\nthere adds a noop index-filter, which does show the problem.\n\nThe new tests in t1601 are slightly redundant with what\nfilter-branch is doing under the hood in t7009. But as\nthey're much more direct, they're easier to reason about.\nAnd should filter-branch ever change or go away, we'd want\nto make sure that these plumbing commands behave sanely.\n\nSigned-off-by: Jeff King \n---\nWhen merged to pu, this fixes the existing test breakage in t7009 when\nGIT_TEST_SPLIT_INDEX is used (because the split index didn't rewrite the\nwhole index, \"git rm --cached\" didn't always barf). But I think it's\nworth doing on its own merits, as demonstrated by the new tests.\n\nThe one thing I haven't figured out it is why the new test in t7009\nfails with the split-index. And even more curiously, the new tests in\nt1601 _don't_ fail with it, even if I instrument the fake index to have\nmore entries (making it more likely to split).\n\n cache-tree.c | 4 +++-\n read-cache.c | 5 ++++-\n t/t1601-index-bogus.sh | 22 ++++++++++++++++++++++\n t/t7009-filter-branch-null-sha1.sh | 6 ++++++\n 4 files changed, 35 insertions(+), 2 deletions(-)\n create mode 100755 t/t1601-index-bogus.sh\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 345ea3596..34baa6d85 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -354,7 +354,9 @@ static int update_one(struct cache_tree *it,\n \t\t\tentlen = pathlen - baselen;\n \t\t\ti++;\n \t\t}\n-\t\tif (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {\n+\n+\t\tif (is_null_sha1(sha1) ||\n+\t\t (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))) {\n \t\t\tstrbuf_release(&buffer);\n \t\t\tif (expected_missing)\n \t\t\t\treturn -1;\ndiff --git a/read-cache.c b/read-cache.c\nindex e44775182..6851de892 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2054,6 +2054,7 @@ static int do_write_index(struct index_state *istate, int newfd,\n \tint entries = istate->cache_nr;\n \tstruct stat st;\n \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n+\tint drop_cache_tree = 0;\n \n \tfor (i = removed = extended = 0; i < entries; i++) {\n \t\tif (cache[i]->ce_flags & CE_REMOVE)\n@@ -2104,6 +2105,8 @@ static int do_write_index(struct index_state *istate, int newfd,\n \t\t\t\twarning(msg, ce->name);\n \t\t\telse\n \t\t\t\treturn error(msg, ce->name);\n+\n+\t\t\tdrop_cache_tree = 1;\n \t\t}\n \t\tif (ce_write_entry(&c, newfd, ce, previous_name) < 0)\n \t\t\treturn -1;\n@@ -2122,7 +2125,7 @@ static int do_write_index(struct index_state *istate, int newfd,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n-\tif (!strip_extensions && istate->cache_tree) {\n+\tif (!strip_extensions && !drop_cache_tree && istate->cache_tree) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tcache_tree_write(&sb, istate->cache_tree);\ndiff --git a/t/t1601-index-bogus.sh b/t/t1601-index-bogus.sh\nnew file mode 100755\nindex 000000000..73cc9323c\n--- /dev/null\n+++ b/t/t1601-index-bogus.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+\n+test_description='test handling of bogus index entries'\n+. ./test-lib.sh\n+\n+test_expect_success 'create tree with null sha1' '\n+\ttree=$(printf \"160000 commit $_z40\\\\tbroken\\\\n\" | git mktree)\n+'\n+\n+test_expect_success 'read-tree refuses to read null sha1' '\n+\ttest_must_fail git read-tree $tree\n+'\n+\n+test_expect_success 'GIT_ALLOW_NULL_SHA1 overrides refusal' '\n+\tGIT_ALLOW_NULL_SHA1=1 git read-tree $tree\n+'\n+\n+test_expect_success 'git write-tree refuses to write null sha1' '\n+\ttest_must_fail git write-tree\n+'\n+\n+test_done\ndiff --git a/t/t7009-filter-branch-null-sha1.sh b/t/t7009-filter-branch-null-sha1.sh\nindex c27f90f28..a8d9ec498 100755\n--- a/t/t7009-filter-branch-null-sha1.sh\n+++ b/t/t7009-filter-branch-null-sha1.sh\n@@ -31,6 +31,12 @@ test_expect_success 'setup: bring HEAD and index in sync' '\n \tgit commit -a -m \"back to normal\"\n '\n \n+test_expect_success 'noop filter-branch complains' '\n+\ttest_must_fail git filter-branch \\\n+\t\t--force --prune-empty \\\n+\t\t--index-filter \"true\"\n+'\n+\n test_expect_success 'filter commands are still checked' '\n \ttest_must_fail git filter-branch \\\n \t\t--force --prune-empty \\\n-- \n2.13.0.rc0.364.g36b4d8031\n"},{"id":"317664","messageId":"CACsJy8AAtV5KJHBqWvnYb3Mw9CVzEdG3M-UJA+jd5MR5e-UMsA@mail.gmail.com","threadId":"45768","inReplyTo":"20170421184617.lc5bioa5px6ninrj@sigill.intra.peff.net","subject":"Re: [PATCH] cache-tree: reject entries with null sha1","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-24T10:39:33Z","receivedAt":"2017-04-24T10:42:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 22, 2017 at 1:46 AM, Jeff King wrote:\n> We generally disallow null sha1s from entering the index,\n> due to 4337b5856 (do not write null sha1s to on-disk index,\n> 2012-07-28). However, we loosened that in 83bd7437c\n> (write_index: optionally allow broken null sha1s,\n> 2013-08-27) so that tools like filter-branch could be used\n> to repair broken history.\n\nUh oh.. cache-tree.\n\n> However, we should make sure that these broken entries do\n> not get propagated into new trees. For most entries, we'd\n> catch them with the missing-object check (since presumably\n> the null sha1 does not exist in our object database). But\n> gitlink entries do not need reachability, so we may blindly\n> copy the entry into a bogus tree.\n\nPhew.. not another bug of mine :D\n\n> When merged to pu, this fixes the existing test breakage in t7009 when\n> GIT_TEST_SPLIT_INDEX is used (because the split index didn't rewrite the\n> whole index, \"git rm --cached\" didn't always barf).\n\nLatest 'pu' has your patch, but t7009 still fails on me (with \"invalid\nobject\" error), more on this later..\n\n> But I think it's worth doing on its own merits, as demonstrated by the new tests.\n\nAgreed. The patch looks correct.\n\nJust checking, since cache-tree helps speed up many operations,\ndropping cache-tree can have some performance implication. But this\nmust be an error case (null sha1) and we will not run into it often to\nworry about unnecessary dropping, correct?\n\n> The one thing I haven't figured out it is why the new test in t7009\n> fails with the split-index. And even more curiously, the new tests in\n> t1601 _don't_ fail with it, even if I instrument the fake index to have\n> more entries (making it more likely to split).\n\nback to t7009 failure. I'll see if I can look more into this this\nweekend. If split-index somehow produces these null sha1, then I\nprobably have a problem.\n\nThanks for looking at it anyway. One bug down. Thousands to go...\n\nBTW, I ran t7009 with valgrind and it reported this. Is it something\nwe should be worried about? I vaguely recall you're doing something\nwith prio-queue...\n\n==4246== Source and destination overlap in memcpy(0x5952990, 0x5952990, 16)\n==4246== at 0x4C2EACD: memcpy@@GLIBC_2.14 (in\n/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)\n==4246== by 0x545D05: swap (prio-queue.c:15)\n==4246== by 0x545D72: prio_queue_reverse (prio-queue.c:25)\n==4246== by 0x4CBC0C: sort_in_topological_order (commit.c:723)\n==4246== by 0x574C97: prepare_revision_walk (revision.c:2858)\n==4246== by 0x48A2BA: cmd_rev_list (rev-list.c:385)\n==4246== by 0x405A6F: run_builtin (git.c:371)\n==4246== by 0x405CDC: handle_builtin (git.c:572)\n==4246== by 0x405E51: run_argv (git.c:624)\n==4246== by 0x405FF3: cmd_main (git.c:701)\n==4246== by 0x4A48CE: main (common-main.c:43)\n-- \nDuy\n"},{"id":"317668","messageId":"20170424111330.rfkoafb74wxzszl2@sigill.intra.peff.net","threadId":"45768","inReplyTo":"CACsJy8AAtV5KJHBqWvnYb3Mw9CVzEdG3M-UJA+jd5MR5e-UMsA@mail.gmail.com","subject":"Re: [PATCH] cache-tree: reject entries with null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-24T11:13:31Z","receivedAt":"2017-04-24T11:17:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 24, 2017 at 05:39:33PM +0700, Duy Nguyen wrote:\n\n> > When merged to pu, this fixes the existing test breakage in t7009 when\n> > GIT_TEST_SPLIT_INDEX is used (because the split index didn't rewrite the\n> > whole index, \"git rm --cached\" didn't always barf).\n> \n> Latest 'pu' has your patch, but t7009 still fails on me (with \"invalid\n> object\" error), more on this later..\n\nRight, it fails for me, too. But only on the new test \"noop\nfilter-branch complaisn\". The old one \"filter commands are still\nchecked\" should pass after my patch.\n\n> Just checking, since cache-tree helps speed up many operations,\n> dropping cache-tree can have some performance implication. But this\n> must be an error case (null sha1) and we will not run into it often to\n> worry about unnecessary dropping, correct?\n\nCorrect. We usually die when we see a null sha1. So this only kicks in\nwhen GIT_ALLOW_NULL_SHA1 is set, which basically means when\nfilter-branch is running. And of course there it only kicks in when you\nactually have a null sha1, which is a rare error case (and you must be\nremoving it with your filter, or write-tree will barf anyway).\n\n> > The one thing I haven't figured out it is why the new test in t7009\n> > fails with the split-index. And even more curiously, the new tests in\n> > t1601 _don't_ fail with it, even if I instrument the fake index to have\n> > more entries (making it more likely to split).\n> \n> back to t7009 failure. I'll see if I can look more into this this\n> weekend. If split-index somehow produces these null sha1, then I\n> probably have a problem.\n\nI don't think it's producing them. It's just that the check in\nwrite-tree isn't triggering for some reasons. If I run t7009 with\nGIT_TRACE=1, the failing test shows:\n\n trace: built-in: git 'read-tree' '-i' '-m' 'e3ed0c3a94f05540151bd8cb9ac647b8777964a6'\n error: invalid object 160000 0000000000000000000000000000000000000000 for 'broken'\n warning: cache entry has null sha1: broken\n trace: built-in: git 'cat-file' 'commit' 'e3ed0c3a94f05540151bd8cb9ac647b8777964a6'\n trace: built-in: git 'write-tree'\n\nThat last write-tree _should_ barf, but it doesn't. I suspect the reason\nis that in the read-tree step, we do not properly strip the cache-tree\nwhen we see the null sha1, so it just gets reused later. But I couldn't\nreproduce it when running t1601, which does roughly the same steps.\n\nIf I instrument it like this:\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 15a4779f2..4d9482092 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2268,9 +2268,10 @@ static int do_write_index(struct index_state *istate, int newfd,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n+\twarning(\"drop_cache_tree = %d\", drop_cache_tree);\n \tif (!strip_extensions && !drop_cache_tree && istate->cache_tree) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n-\n+\t\twarning(\"reusing cache tree\");\n \t\tcache_tree_write(&sb, istate->cache_tree);\n \t\terr = write_index_ext_header(&c, newfd, CACHE_EXT_TREE, sb.len) < 0\n \t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n\nthen I see:\n\n trace: built-in: git 'read-tree' '-i' '-m' 'e3ed0c3a94f05540151bd8cb9ac647b8777964a6'\n error: invalid object 160000 0000000000000000000000000000000000000000 for 'broken'\n warning: cache entry has null sha1: broken\n warning: drop_cache_tree = 1\n warning: drop_cache_tree = 0\n warning: reusing cache tree\n trace: built-in: git 'cat-file' 'commit' 'e3ed0c3a94f05540151bd8cb9ac647b8777964a6'\n trace: built-in: git 'write-tree'\n\nSo we end up in do_write_index twice, and one time we copy the cache\ntree. I don't know how cache-tree works with the split index. Is it\npossible that we write the cache-tree covering the entry for \"broken\"\ninto a separate index than the \"broken\" entry itself? That would explain\nwhat I'm seeing.\n\n> BTW, I ran t7009 with valgrind and it reported this. Is it something\n> we should be worried about? I vaguely recall you're doing something\n> with prio-queue...\n> \n> ==4246== Source and destination overlap in memcpy(0x5952990, 0x5952990, 16)\n> ==4246== at 0x4C2EACD: memcpy@@GLIBC_2.14 (in\n> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)\n> ==4246== by 0x545D05: swap (prio-queue.c:15)\n> ==4246== by 0x545D72: prio_queue_reverse (prio-queue.c:25)\n> ==4246== by 0x4CBC0C: sort_in_topological_order (commit.c:723)\n> ==4246== by 0x574C97: prepare_revision_walk (revision.c:2858)\n> ==4246== by 0x48A2BA: cmd_rev_list (rev-list.c:385)\n> ==4246== by 0x405A6F: run_builtin (git.c:371)\n> ==4246== by 0x405CDC: handle_builtin (git.c:572)\n> ==4246== by 0x405E51: run_argv (git.c:624)\n> ==4246== by 0x405FF3: cmd_main (git.c:701)\n> ==4246== by 0x4A48CE: main (common-main.c:43)\n\nI couldn't reproduce here, but it looks like it comes from René's new\nSWAP macro. It doesn't detect a noop swap, and we may call memcpy with\nidentical src and dst parameters. That's _probably_ fine in practice,\nbut we may want to exit the function early.\n\nI'll ping separately in that thread.\n\n-Peff\n"},{"id":"318361","messageId":"12c3312c-30a1-2cb1-8f05-63bb663bd0a0@web.de","threadId":"45768","inReplyTo":"CACsJy8AAtV5KJHBqWvnYb3Mw9CVzEdG3M-UJA+jd5MR5e-UMsA@mail.gmail.com","subject":"Re: [PATCH] cache-tree: reject entries with null sha1","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-05-01T11:23:28Z","receivedAt":"2017-05-01T11:23:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.04.2017 um 12:39 schrieb Duy Nguyen:\n> BTW, I ran t7009 with valgrind and it reported this. Is it something\n> we should be worried about? I vaguely recall you're doing something\n> with prio-queue...\n> \n> ==4246== Source and destination overlap in memcpy(0x5952990, 0x5952990, 16)\n> ==4246== at 0x4C2EACD: memcpy@@GLIBC_2.14 (in\n> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)\n> ==4246== by 0x545D05: swap (prio-queue.c:15)\n> ==4246== by 0x545D72: prio_queue_reverse (prio-queue.c:25)\n> ==4246== by 0x4CBC0C: sort_in_topological_order (commit.c:723)\n> ==4246== by 0x574C97: prepare_revision_walk (revision.c:2858)\n> ==4246== by 0x48A2BA: cmd_rev_list (rev-list.c:385)\n> ==4246== by 0x405A6F: run_builtin (git.c:371)\n> ==4246== by 0x405CDC: handle_builtin (git.c:572)\n> ==4246== by 0x405E51: run_argv (git.c:624)\n> ==4246== by 0x405FF3: cmd_main (git.c:701)\n> ==4246== by 0x4A48CE: main (common-main.c:43)\n\nI can only get gcc and clang to call memcpy instead of inlining it by\nspecifying -fno-builtin. Do you use that option? If yes, why? (Just\ncurious.)\n\nBut I can't get Valgrind to report overlapping (nicely explained in\nhttp://valgrind.org/docs/manual/mc-manual.html#mc-manual.overlap, by\nthe way), not for t7009 and not for the short test program at the\nbottom. Do you set flags in GIT_VALGRIND_OPTIONS or use a special\nversion of Valgrind? I use valgrind-3.12.0.SVN from Debian testing.\n\nThanks,\nRené\n\n\n/* Compile with -fno-builtin. */\n#include \nint main(void) {int i = 1; memcpy(&i, &i, sizeof(i)); return i;}\n"},{"id":"318364","messageId":"8754b32d-9eec-973d-a225-175085fc7e82@web.de","threadId":"45768","inReplyTo":"12c3312c-30a1-2cb1-8f05-63bb663bd0a0@web.de","subject":"Re: [PATCH] cache-tree: reject entries with null sha1","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-05-01T11:55:03Z","receivedAt":"2017-05-01T11:58:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.05.2017 um 13:23 schrieb René Scharfe:\n> But I can't get Valgrind to report overlapping (nicely explained in\n> http://valgrind.org/docs/manual/mc-manual.html#mc-manual.overlap, by\n> the way), not for t7009 and not for the short test program at the\n> bottom. Do you set flags in GIT_VALGRIND_OPTIONS or use a special\n> version of Valgrind? I use valgrind-3.12.0.SVN from Debian testing.\n> \n> Thanks,\n> René\n> \n> \n> /* Compile with -fno-builtin. */\n> #include \n> int main(void) {int i = 1; memcpy(&i, &i, sizeof(i)); return i;}\n\nActually I can, on Debian stable x86 (Valgrind-3.10.0). Not sure\nwhether it's the older version of Valgrind or the 32-bitness, but\nit gives me a way to reproduce, so nevermind.\n\nRené\n"},{"id":"318401","messageId":"20170501192212.xy5o72tinb2hy2a3@sigill.intra.peff.net","threadId":"45768","inReplyTo":"12c3312c-30a1-2cb1-8f05-63bb663bd0a0@web.de","subject":"Re: [PATCH] cache-tree: reject entries with null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-01T19:22:12Z","receivedAt":"2017-05-01T19:25:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 01, 2017 at 01:23:28PM +0200, René Scharfe wrote:\n\n> Am 24.04.2017 um 12:39 schrieb Duy Nguyen:\n> > BTW, I ran t7009 with valgrind and it reported this. Is it something\n> > we should be worried about? I vaguely recall you're doing something\n> > with prio-queue...\n> > \n> > ==4246== Source and destination overlap in memcpy(0x5952990, 0x5952990, 16)\n> > ==4246== at 0x4C2EACD: memcpy@@GLIBC_2.14 (in\n> > /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)\n> > ==4246== by 0x545D05: swap (prio-queue.c:15)\n> > ==4246== by 0x545D72: prio_queue_reverse (prio-queue.c:25)\n> > ==4246== by 0x4CBC0C: sort_in_topological_order (commit.c:723)\n> > ==4246== by 0x574C97: prepare_revision_walk (revision.c:2858)\n> > ==4246== by 0x48A2BA: cmd_rev_list (rev-list.c:385)\n> > ==4246== by 0x405A6F: run_builtin (git.c:371)\n> > ==4246== by 0x405CDC: handle_builtin (git.c:572)\n> > ==4246== by 0x405E51: run_argv (git.c:624)\n> > ==4246== by 0x405FF3: cmd_main (git.c:701)\n> > ==4246== by 0x4A48CE: main (common-main.c:43)\n> \n> I can only get gcc and clang to call memcpy instead of inlining it by\n> specifying -fno-builtin. Do you use that option? If yes, why? (Just\n> curious.)\n\nI do my normal edit-compile cycles with -O0 because it's fast, and\nbecause it makes debugging much easier.\n\n> But I can't get Valgrind to report overlapping (nicely explained in\n> http://valgrind.org/docs/manual/mc-manual.html#mc-manual.overlap, by\n> the way), not for t7009 and not for the short test program at the\n> bottom. Do you set flags in GIT_VALGRIND_OPTIONS or use a special\n> version of Valgrind? I use valgrind-3.12.0.SVN from Debian testing.\n\nI saw it with 3.12.0-1.1 on Debian unstable.\n\n-Peff\n"},{"id":"318405","messageId":"456378ba-a117-8003-eea2-69272ed9bb58@web.de","threadId":"45768","inReplyTo":"20170501192212.xy5o72tinb2hy2a3@sigill.intra.peff.net","subject":"Re: [PATCH] cache-tree: reject entries with null sha1","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-05-01T21:00:58Z","receivedAt":"2017-05-01T21:01:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.05.2017 um 21:22 schrieb Jeff King:\n> On Mon, May 01, 2017 at 01:23:28PM +0200, René Scharfe wrote:\n>> I can only get gcc and clang to call memcpy instead of inlining it by\n>> specifying -fno-builtin. Do you use that option? If yes, why? (Just\n>> curious.)\n> \n> I do my normal edit-compile cycles with -O0 because it's fast, and\n> because it makes debugging much easier.\n\nGCC and Clang still inline memcpy with -O0 alone (at least the versions\nI tested).\n\nRené\n"},{"id":"318407","messageId":"20170501212656.h4vhkstjolh46whe@sigill.intra.peff.net","threadId":"45768","inReplyTo":"456378ba-a117-8003-eea2-69272ed9bb58@web.de","subject":"Re: [PATCH] cache-tree: reject entries with null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-01T21:26:56Z","receivedAt":"2017-05-01T21:27:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 01, 2017 at 11:00:58PM +0200, René Scharfe wrote:\n\n> Am 01.05.2017 um 21:22 schrieb Jeff King:\n> > On Mon, May 01, 2017 at 01:23:28PM +0200, René Scharfe wrote:\n> > > I can only get gcc and clang to call memcpy instead of inlining it by\n> > > specifying -fno-builtin. Do you use that option? If yes, why? (Just\n> > > curious.)\n> > \n> > I do my normal edit-compile cycles with -O0 because it's fast, and\n> > because it makes debugging much easier.\n> \n> GCC and Clang still inline memcpy with -O0 alone (at least the versions\n> I tested).\n\nI just checked the assembler output and confirmed that is the case for\nmy builds, even with -O0. Which probably explains why I wasn't able to\nreplicate Duy's valgrind error in the first place.\n\n-Peff\n"},{"id":"318614","messageId":"CACsJy8CdjLySD+UEKE2NkbYqEoVOQtvydFBui6UbbnFLo8U1Ug@mail.gmail.com","threadId":"45768","inReplyTo":"20170501192212.xy5o72tinb2hy2a3@sigill.intra.peff.net","subject":"Re: [PATCH] cache-tree: reject entries with null sha1","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T09:46:04Z","receivedAt":"2017-05-03T09:46:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 2, 2017 at 2:22 AM, Jeff King wrote:\n> On Mon, May 01, 2017 at 01:23:28PM +0200, René Scharfe wrote:\n>\n>> Am 24.04.2017 um 12:39 schrieb Duy Nguyen:\n>> > BTW, I ran t7009 with valgrind and it reported this. Is it something\n>> > we should be worried about? I vaguely recall you're doing something\n>> > with prio-queue...\n>> >\n>> > ==4246== Source and destination overlap in memcpy(0x5952990, 0x5952990, 16)\n>> > ==4246== at 0x4C2EACD: memcpy@@GLIBC_2.14 (in\n>> > /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)\n>> > ==4246== by 0x545D05: swap (prio-queue.c:15)\n>> > ==4246== by 0x545D72: prio_queue_reverse (prio-queue.c:25)\n>> > ==4246== by 0x4CBC0C: sort_in_topological_order (commit.c:723)\n>> > ==4246== by 0x574C97: prepare_revision_walk (revision.c:2858)\n>> > ==4246== by 0x48A2BA: cmd_rev_list (rev-list.c:385)\n>> > ==4246== by 0x405A6F: run_builtin (git.c:371)\n>> > ==4246== by 0x405CDC: handle_builtin (git.c:572)\n>> > ==4246== by 0x405E51: run_argv (git.c:624)\n>> > ==4246== by 0x405FF3: cmd_main (git.c:701)\n>> > ==4246== by 0x4A48CE: main (common-main.c:43)\n>>\n>> I can only get gcc and clang to call memcpy instead of inlining it by\n>> specifying -fno-builtin. Do you use that option? If yes, why? (Just\n>> curious.)\n>\n> I do my normal edit-compile cycles with -O0 because it's fast, and\n> because it makes debugging much easier.\n\nSame here. My CFLAGS (without lots of -Wstuff)\n\nCFLAGS = -g -O0 -fstack-protector\n\nMaybe it's -fstack-protector then? This is gcc 4.9.3. I think Gentoo\ndoes not add any distro-specific patches on this particular version.\n-- \nDuy\n"},{"id":"318692","messageId":"cddd09e5-8e8a-3dc9-001f-f307d3647619@web.de","threadId":"45768","inReplyTo":"CACsJy8CdjLySD+UEKE2NkbYqEoVOQtvydFBui6UbbnFLo8U1Ug@mail.gmail.com","subject":"Re: [PATCH] cache-tree: reject entries with null sha1","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-05-03T19:48:31Z","receivedAt":"2017-05-03T19:48:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.05.2017 um 11:46 schrieb Duy Nguyen:\n> On Tue, May 2, 2017 at 2:22 AM, Jeff King wrote:\n>> On Mon, May 01, 2017 at 01:23:28PM +0200, René Scharfe wrote:\n>>>\n>>> I can only get gcc and clang to call memcpy instead of inlining it by\n>>> specifying -fno-builtin. Do you use that option? If yes, why? (Just\n>>> curious.)\n>>\n>> I do my normal edit-compile cycles with -O0 because it's fast, and\n>> because it makes debugging much easier.\n> \n> Same here. My CFLAGS (without lots of -Wstuff)\n> \n> CFLAGS = -g -O0 -fstack-protector\n> \n> Maybe it's -fstack-protector then? This is gcc 4.9.3. I think Gentoo\n> does not add any distro-specific patches on this particular version.\n\ngcc 4.9.2 on Debian i386 still inlines memcpy for me with these options.\n\nhttps://packages.debian.org/jessie/gcc-4.9 links to a 5MB diff, and it\nadds these lines to NEWS.gcc (plus changing a whole lot of other files,\nof course):\n\n+ - Better inlining of memcpy and memset that is aware of value ranges\n+ and produces shorter alignment prologues.\n\nThat might be it.\n\nRené\n"}]}