{"thread":{"id":"46087","subject":"Unaligned accesses in sha1dc","startedAt":"2017-06-01T08:29:03Z","lastAt":"2017-06-03T00:15:19Z","messageCount":28,"participants":["Andreas Schwab","Junio C Hamano","brian m. carlson","Lars Schneider","Martin Ågren","Ævar Arnfjörð Bjarmason","Liam R. Howlett","demerphq","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"321261","messageId":"mvm4lw0un5n.fsf@suse.de","threadId":"46087","inReplyTo":null,"subject":"Unaligned accesses in sha1dc","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2017-06-01T08:28:52Z","receivedAt":"2017-06-01T08:29:03Z","isPatch":false,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"The sh1dc implementation is making unaligned accesses, which will crash\non some architectures, others have to emulate them in software.\n\nBreakpoint 4, sha1_compression_states (ihv=0x600ffffffffe7010, \n m=, W=0x600ffffffffe70a8, states=0x600ffffffffe7328)\n at sha1dc/sha1.c:398\n398 SHA1COMPRESS_FULL_ROUND2_STEP(e, a, b, c, d, W, 21, temp);\n(gdb) n\n403 SHA1COMPRESS_FULL_ROUND2_STEP(d, e, a, b, c, W, 22, temp);\n(gdb) \n408 SHA1COMPRESS_FULL_ROUND2_STEP(c, d, e, a, b, W, 23, temp);\n(gdb) \n413 SHA1COMPRESS_FULL_ROUND2_STEP(b, c, d, e, a, W, 24, temp);\n(gdb) \n418 SHA1COMPRESS_FULL_ROUND2_STEP(a, b, c, d, e, W, 25, temp);\n(gdb) \n291 SHA1COMPRESS_FULL_ROUND1_STEP_LOAD(a, b, c, d, e, m, W, 0, temp);\n(gdb) \ngit(21728): unaligned access to 0x600000000009f8d5, ip=0x40000000003336d0\n423 SHA1COMPRESS_FULL_ROUND2_STEP(e, a, b, c, d, W, 26, temp);\n\nAndreas.\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"321262","messageId":"mvmzidst6vt.fsf@suse.de","threadId":"46087","inReplyTo":"CDB32E2C-48AF-4636-B921-4C45B614FD35@marc-stevens.nl","subject":"Re: Unaligned accesses in sha1dc","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2017-06-01T09:05:42Z","receivedAt":"2017-06-01T09:05:50Z","isPatch":false,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"SHA1DCUpdate calls sha1_process with buf being unaligned.\n\nAndreas.\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"321263","messageId":"xmqqk24w5asf.fsf@gitster.mtv.corp.google.com","threadId":"46087","inReplyTo":"mvm4lw0un5n.fsf@suse.de","subject":"Re: Unaligned accesses in sha1dc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-01T09:15:12Z","receivedAt":"2017-06-01T09:15:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Is this with or without a0103914 (\"sha1dc: update from upstream\",\n2017-05-20)?\n"},{"id":"321264","messageId":"mvmvaogt6eo.fsf@suse.de","threadId":"46087","inReplyTo":"xmqqk24w5asf.fsf@gitster.mtv.corp.google.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2017-06-01T09:15:59Z","receivedAt":"2017-06-01T09:16:16Z","isPatch":false,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"This is git 2.13.0.\n\nAndreas.\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"321265","messageId":"20170601091856.r3oaddwzsndniyfa@genre.crustytoothpaste.net","threadId":"46087","inReplyTo":"mvm4lw0un5n.fsf@suse.de","subject":"Re: Unaligned accesses in sha1dc","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-06-01T09:18:57Z","receivedAt":"2017-06-01T09:19:07Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Jun 01, 2017 at 10:28:52AM +0200, Andreas Schwab wrote:\n> The sh1dc implementation is making unaligned accesses, which will crash\n> on some architectures, others have to emulate them in software.\n> \n> Breakpoint 4, sha1_compression_states (ihv=0x600ffffffffe7010, \n> m=, W=0x600ffffffffe70a8, states=0x600ffffffffe7328)\n> at sha1dc/sha1.c:398\n> 398 SHA1COMPRESS_FULL_ROUND2_STEP(e, a, b, c, d, W, 21, temp);\n> (gdb) n\n> 403 SHA1COMPRESS_FULL_ROUND2_STEP(d, e, a, b, c, W, 22, temp);\n> (gdb) \n> 408 SHA1COMPRESS_FULL_ROUND2_STEP(c, d, e, a, b, W, 23, temp);\n> (gdb) \n> 413 SHA1COMPRESS_FULL_ROUND2_STEP(b, c, d, e, a, W, 24, temp);\n> (gdb) \n> 418 SHA1COMPRESS_FULL_ROUND2_STEP(a, b, c, d, e, W, 25, temp);\n> (gdb) \n> 291 SHA1COMPRESS_FULL_ROUND1_STEP_LOAD(a, b, c, d, e, m, W, 0, temp);\n> (gdb) \n> git(21728): unaligned access to 0x600000000009f8d5, ip=0x40000000003336d0\n> 423 SHA1COMPRESS_FULL_ROUND2_STEP(e, a, b, c, d, W, 26, temp);\n\nWhat architecture are you seeing this on?\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"321266","messageId":"5100A096-EBAC-4B01-A94D-69D31093148D@gmail.com","threadId":"46087","inReplyTo":"mvm4lw0un5n.fsf@suse.de","subject":"Re: Unaligned accesses in sha1dc","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-01T09:21:31Z","receivedAt":"2017-06-01T09:21:43Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 01 Jun 2017, at 10:28, Andreas Schwab wrote:\n> \n> The sh1dc implementation is making unaligned accesses, which will crash\n> on some architectures, others have to emulate them in software.\n\nIs SPARC an architecture that would run into this problem? I think\nthere was a thread a few days ago about this...\n\nWhat architectures are affected by this? I think I read somewhere\nthat ARM requires aligned accesses, too, right?\n\nI wonder if it makes sense to emulate SPARC/ARM/... with QEMU on TravisCI [1].\nIs this what you had in mind with \"emulate\" or do you see a better way?\nIf we compile and test in this environment - we should be able to catch\nthose bugs, right?\n\n- Lars\n\n\n[1] https://www.tomaz.me/2013/12/02/running-travis-ci-tests-on-arm.html"},{"id":"321267","messageId":"mvmr2z4t5ns.fsf@suse.de","threadId":"46087","inReplyTo":"20170601091856.r3oaddwzsndniyfa@genre.crustytoothpaste.net","subject":"Re: Unaligned accesses in sha1dc","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2017-06-01T09:32:07Z","receivedAt":"2017-06-01T09:32:15Z","isPatch":false,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Jun 01 2017, \"brian m. carlson\" wrote:\n\n> On Thu, Jun 01, 2017 at 10:28:52AM +0200, Andreas Schwab wrote:\n>> The sh1dc implementation is making unaligned accesses, which will crash\n>> on some architectures, others have to emulate them in software.\n>> \n>> Breakpoint 4, sha1_compression_states (ihv=0x600ffffffffe7010, \n>> m=, W=0x600ffffffffe70a8, states=0x600ffffffffe7328)\n>> at sha1dc/sha1.c:398\n>> 398 SHA1COMPRESS_FULL_ROUND2_STEP(e, a, b, c, d, W, 21, temp);\n>> (gdb) n\n>> 403 SHA1COMPRESS_FULL_ROUND2_STEP(d, e, a, b, c, W, 22, temp);\n>> (gdb) \n>> 408 SHA1COMPRESS_FULL_ROUND2_STEP(c, d, e, a, b, W, 23, temp);\n>> (gdb) \n>> 413 SHA1COMPRESS_FULL_ROUND2_STEP(b, c, d, e, a, W, 24, temp);\n>> (gdb) \n>> 418 SHA1COMPRESS_FULL_ROUND2_STEP(a, b, c, d, e, W, 25, temp);\n>> (gdb) \n>> 291 SHA1COMPRESS_FULL_ROUND1_STEP_LOAD(a, b, c, d, e, m, W, 0, temp);\n>> (gdb) \n>> git(21728): unaligned access to 0x600000000009f8d5, ip=0x40000000003336d0\n>> 423 SHA1COMPRESS_FULL_ROUND2_STEP(e, a, b, c, d, W, 26, temp);\n>\n> What architecture are you seeing this on?\n\nIt doesn't matter.\n\nAndreas.\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"321268","messageId":"xmqqfufk59wc.fsf@gitster.mtv.corp.google.com","threadId":"46087","inReplyTo":"mvmvaogt6eo.fsf@suse.de","subject":"Re: Unaligned accesses in sha1dc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-01T09:34:27Z","receivedAt":"2017-06-01T09:35:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab writes:\n\n> This is git 2.13.0.\n\nThanks. It is a known issue with a known fix cooking in 'next' to\nbe merged down to 'master' and 'maint' not in a too distant future.\nAn extra testing to ensure that the \"fix\" actually works before it\nis merged down to a maintenance release is very much appreciated.\n\n $ git fetch git://git.kernel.org/pub/scm/git/git.git/ next\n $ git checkout v2.13.0^0\n $ git merge a0103914\n\nshould show what the proposed v2.13.x would look like wrt to this\nissue.\n\nThanks.\n"},{"id":"321271","messageId":"xmqqwp8w3uff.fsf@gitster.mtv.corp.google.com","threadId":"46087","inReplyTo":"5100A096-EBAC-4B01-A94D-69D31093148D@gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-01T09:53:56Z","receivedAt":"2017-06-01T09:54:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 01 Jun 2017, at 10:28, Andreas Schwab wrote:\n>> \n>> The sh1dc implementation is making unaligned accesses, which will crash\n>> on some architectures, others have to emulate them in software.\n>\n> Is SPARC an architecture that would run into this problem? I think\n> there was a thread a few days ago about this...\n>\n> What architectures are affected by this? I think I read somewhere\n> that ARM requires aligned accesses, too, right?\n>\n> I wonder if it makes sense to emulate SPARC/ARM/... with QEMU on TravisCI [1].\n> Is this what you had in mind with \"emulate\" or do you see a better way?\n\nI think Andreas's \"emulate\" is that on these architectures often the\nkernel catches the hardware trap and makes the unaligned access\nappear to \"just work\" to the userland software, just like in very\nold days, i386 and i486 without 387/487 used software floating point\n\"emulation\" to give illusion to the userland softare that the co\nprocessor was available.\n\nHaving to trap and emulate of course costs cycles, and if the\nuserland is written in such a way not to do an unaligned access in\nthe first place.\n\nDepending on the model of \"ARM\" (or \"SPARC\") emulated with QEMU, and\ndepending on the OS that runs on such an \"ARM\" or \"SPARC\", we may\nnot see this---if the emulated OS has the \"software unaligned-access\nemulation\" our userland may not see a SIGBUS.\n"},{"id":"321273","messageId":"mvmmv9st3yv.fsf@suse.de","threadId":"46087","inReplyTo":"xmqqwp8w3uff.fsf@gitster.mtv.corp.google.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2017-06-01T10:08:40Z","receivedAt":"2017-06-01T10:08:45Z","isPatch":false,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Jun 01 2017, Junio C Hamano wrote:\n\n> Depending on the model of \"ARM\" (or \"SPARC\") emulated with QEMU, and\n> depending on the OS that runs on such an \"ARM\" or \"SPARC\", we may\n> not see this---if the emulated OS has the \"software unaligned-access\n> emulation\" our userland may not see a SIGBUS.\n\nEven if the architecture implements unaligned accesses in hardware, it\nis still undefined behaviour, and the compiler will (eventually) take\nadvantage of it.\n\nAndreas.\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"321274","messageId":"CAN0heSrzpwhS3Zf83vTzHAAmi8YVD4CoCh_px5SBXBZhSKPqPQ@mail.gmail.com","threadId":"46087","inReplyTo":"mvmmv9st3yv.fsf@suse.de","subject":"Re: Unaligned accesses in sha1dc","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-06-01T10:26:08Z","receivedAt":"2017-06-01T10:26:14Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 1 June 2017 at 12:08, Andreas Schwab wrote:\n> On Jun 01 2017, Junio C Hamano wrote:\n>\n>> Depending on the model of \"ARM\" (or \"SPARC\") emulated with QEMU, and\n>> depending on the OS that runs on such an \"ARM\" or \"SPARC\", we may\n>> not see this---if the emulated OS has the \"software unaligned-access\n>> emulation\" our userland may not see a SIGBUS.\n>\n> Even if the architecture implements unaligned accesses in hardware, it\n> is still undefined behaviour, and the compiler will (eventually) take\n> advantage of it.\n\nI tried to optically follow the macros and ended up on line 87/89 in\nlib/sha1.c of the sha1dc-library, where there is undefined behavior if\nthe address is unaligned, which it seems it could be. Maybe Git uses\nsome particular combination of macro-definitions and I went down the\nwrong path... There might also be other spots; I haven't thrown UBSan\nat the code.\n\nUsing memcpy on those lines should not be a performance problem on\nplatforms where unaligned access is ok, of course assuming the\ncompiler sees the opportunity.\n\nMartin\n"},{"id":"321275","messageId":"CACBZZX6H9EZTLVnunoH2641fw6QmQL=hO9isinK07-dHnuxyFQ@mail.gmail.com","threadId":"46087","inReplyTo":"CAN0heSrzpwhS3Zf83vTzHAAmi8YVD4CoCh_px5SBXBZhSKPqPQ@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-01T10:33:10Z","receivedAt":"2017-06-01T10:33:36Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Jun 1, 2017 at 12:26 PM, Martin Ågren wrote:\n> On 1 June 2017 at 12:08, Andreas Schwab wrote:\n>> On Jun 01 2017, Junio C Hamano wrote:\n>>\n>>> Depending on the model of \"ARM\" (or \"SPARC\") emulated with QEMU, and\n>>> depending on the OS that runs on such an \"ARM\" or \"SPARC\", we may\n>>> not see this---if the emulated OS has the \"software unaligned-access\n>>> emulation\" our userland may not see a SIGBUS.\n>>\n>> Even if the architecture implements unaligned accesses in hardware, it\n>> is still undefined behaviour, and the compiler will (eventually) take\n>> advantage of it.\n>\n> I tried to optically follow the macros and ended up on line 87/89 in\n> lib/sha1.c of the sha1dc-library, where there is undefined behavior if\n> the address is unaligned, which it seems it could be. Maybe Git uses\n> some particular combination of macro-definitions and I went down the\n> wrong path... There might also be other spots; I haven't thrown UBSan\n> at the code.\n>\n> Using memcpy on those lines should not be a performance problem on\n> platforms where unaligned access is ok, of course assuming the\n> compiler sees the opportunity.\n\nThis is what the upstream version of sha1dc now in the next branch\ndoes, i.e. just does a memcpy() on platforms which aren't on a\nwhitelist of CPUs that allow unaligned access.\n"},{"id":"321281","messageId":"CAN0heSrZcW3b6Osa8XNs0ghg2RE0ZS6FdPq8oPpwLcJjXAtLHg@mail.gmail.com","threadId":"46087","inReplyTo":"CACBZZX6H9EZTLVnunoH2641fw6QmQL=hO9isinK07-dHnuxyFQ@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-06-01T11:53:00Z","receivedAt":"2017-06-01T11:53:12Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 1 June 2017 at 12:33, Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Jun 1, 2017 at 12:26 PM, Martin Ågren wrote:\n>> On 1 June 2017 at 12:08, Andreas Schwab wrote:\n>>> Even if the architecture implements unaligned accesses in hardware, it\n>>> is still undefined behaviour, and the compiler will (eventually) take\n>>> advantage of it.\n>>\n>> I tried to optically follow the macros and ended up on line 87/89 in\n>> lib/sha1.c of the sha1dc-library, where there is undefined behavior if\n>> the address is unaligned, which it seems it could be. Maybe Git uses\n>> some particular combination of macro-definitions and I went down the\n>> wrong path... There might also be other spots; I haven't thrown UBSan\n>> at the code.\n>>\n>> Using memcpy on those lines should not be a performance problem on\n>> platforms where unaligned access is ok, of course assuming the\n>> compiler sees the opportunity.\n>\n> This is what the upstream version of sha1dc now in the next branch\n> does, i.e. just does a memcpy() on platforms which aren't on a\n> whitelist of CPUs that allow unaligned access.\n\nOk, now I get it. Undefined behavior can occur on line 1772 in\nsha1dc/sha1.c on \"next\", but only if SHA1DC_ALLOW_UNALIGNED_ACCESS is\ndefined. I don't think the macro does what its name suggests, though.\nTo me, it behaves more like\nSHA1DC_RELY_ON_UNDEFINED_BEHAVIOR_TO_DO_THE_RIGHT_THING_BY_CHANCE...\n\nSo it seems the call chain of commands and macros could be redesigned\nto work with \"char*\" instead of \"uint32_t*\"... Then the lines I\nmentioned earlier could be converted to memcpy and \"should\" be\ncompiled to efficient loads where possible. Yes, I know, patches speak\nfor themselves, and no, this mail is not a patch.\n"},{"id":"321296","messageId":"CAN0heSp9DpW4_0QL57_oAHGu+os8k6yd=Z5+0MJnaL6iXTa-qQ@mail.gmail.com","threadId":"46087","inReplyTo":"CAN0heSrZcW3b6Osa8XNs0ghg2RE0ZS6FdPq8oPpwLcJjXAtLHg@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-06-01T15:57:48Z","receivedAt":"2017-06-01T15:58:05Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 1 June 2017 at 13:53, Martin Ågren wrote:\n> On 1 June 2017 at 12:33, Ævar Arnfjörð Bjarmason wrote:\n>> On Thu, Jun 1, 2017 at 12:26 PM, Martin Ågren wrote:\n>>> On 1 June 2017 at 12:08, Andreas Schwab wrote:\n>>>> Even if the architecture implements unaligned accesses in hardware, it\n>>>> is still undefined behaviour, and the compiler will (eventually) take\n>>>> advantage of it.\n>>>\n>>> I tried to optically follow the macros and ended up on line 87/89 in\n>>> lib/sha1.c of the sha1dc-library, where there is undefined behavior if\n>>> the address is unaligned, which it seems it could be. Maybe Git uses\n>>> some particular combination of macro-definitions and I went down the\n>>> wrong path... There might also be other spots; I haven't thrown UBSan\n>>> at the code.\n>>>\n>>> Using memcpy on those lines should not be a performance problem on\n>>> platforms where unaligned access is ok, of course assuming the\n>>> compiler sees the opportunity.\n>>\n>> This is what the upstream version of sha1dc now in the next branch\n>> does, i.e. just does a memcpy() on platforms which aren't on a\n>> whitelist of CPUs that allow unaligned access.\n>\n> Ok, now I get it. Undefined behavior can occur on line 1772 in\n> sha1dc/sha1.c on \"next\", but only if SHA1DC_ALLOW_UNALIGNED_ACCESS is\n> defined. I don't think the macro does what its name suggests, though.\n> To me, it behaves more like\n> SHA1DC_RELY_ON_UNDEFINED_BEHAVIOR_TO_DO_THE_RIGHT_THING_BY_CHANCE...\n>\n> So it seems the call chain of commands and macros could be redesigned\n> to work with \"char*\" instead of \"uint32_t*\"... Then the lines I\n> mentioned earlier could be converted to memcpy and \"should\" be\n> compiled to efficient loads where possible. Yes, I know, patches speak\n> for themselves, and no, this mail is not a patch.\n\nI looked into this some more. It turns out it is possible to trigger\nundefined behavior on \"next\". Here's what I did:\n\ndiff --git a/Makefile b/Makefile\nindex 7c621f7..e3fdad0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -402,7 +402,7 @@ GIT-VERSION-FILE: FORCE\n\n # CFLAGS and LDFLAGS are for the users to override from the command line.\n\n-CFLAGS = -g -O2 -Wall\n+CFLAGS = -g -O2 -Wall -fsanitize=undefined -fsanitize-recover=undefined\n DEVELOPER_CFLAGS = -Werror \\\n -Wdeclaration-after-statement \\\n -Wno-format-zero-length \\\n@@ -412,7 +412,7 @@ DEVELOPER_CFLAGS = -Werror \\\n -Wstrict-prototypes \\\n -Wunused \\\n -Wvla\n-LDFLAGS =\n+LDFLAGS = -fsanitize=undefined -fsanitize-recover=undefined\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n STRIP ?= strip\ndiff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c\nindex a1c13f5..d4e1463 100644\n--- a/t/helper/test-sha1.c\n+++ b/t/helper/test-sha1.c\n@@ -24,6 +24,8 @@ int cmd_main(int ac, const char **av)\n if (bufsz < 1024)\n die(\"OOPS\");\n }\n+ buffer++;\n+ bufsz--;\n\n git_SHA1_Init(&ctx);\n\n\n\nThen I ran\n\n$ ./test-sha1 < ../t0013/shattered-1.pdf\n\nin t/helper. No doubt an experienced Git developer would not patch the\nMakefile and would not run the test like that, but that's what I did.\nUBSan reports an unaligned load (the UB happened already as the\npointer was cast). Of course, tweaking the alignment of \"buffer\" is\ncheating, but similar alignments can supposedly occur in the wild, or\nthe bug reports wouldn't be coming in.\n\nThis \"fixes\" the problem:\n\ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex 3dff80a..d6f4c44 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -66,9 +66,9 @@\n #define sha1_mix(W, t) (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14]\n^ W[t - 16], 1))\n\n #ifdef SHA1DC_BIGENDIAN\n- #define sha1_load(m, t, temp) { temp = m[t]; }\n+ #define sha1_load(m, t, temp) { memcpy(&temp, m+t*4, 4); }\n #else\n- #define sha1_load(m, t, temp) { temp = m[t]; sha1_bswap32(temp); }\n+ #define sha1_load(m, t, temp) { memcpy(&temp, m+t*4, 4);\nsha1_bswap32(temp); }\n #endif\n\n #define sha1_store(W, t, x) *(volatile uint32_t *)&W[t] = x\n@@ -309,7 +309,7 @@ static void sha1_compression_W(uint32_t ihv[5],\nconst uint32_t W[80])\n\n\n\n-void sha1_compression_states(uint32_t ihv[5], const uint32_t m[16],\nuint32_t W[80], uint32_t states[80][5])\n+void sha1_compression_states(uint32_t ihv[5], const uint8_t m[64],\nuint32_t W[80], uint32_t states[80][5])\n {\n uint32_t a = ihv[0], b = ihv[1], c = ihv[2], d = ihv[3], e = ihv[4];\n uint32_t temp;\n@@ -1639,7 +1639,7 @@ static void sha1_recompression_step(uint32_t\nstep, uint32_t ihvin[5], uint32_t i\n\n\n\n-static void sha1_process(SHA1_CTX* ctx, const uint32_t block[16])\n+static void sha1_process(SHA1_CTX* ctx, const uint8_t block[64])\n {\n unsigned i, j;\n uint32_t ubc_dv_mask[DVMASKSIZE] = { 0xFFFFFFFF };\n@@ -1759,7 +1759,7 @@ void SHA1DCUpdate(SHA1_CTX* ctx, const char*\nbuf, size_t len)\n {\n ctx->total += fill;\n memcpy(ctx->buffer + left, buf, fill);\n- sha1_process(ctx, (uint32_t*)(ctx->buffer));\n+ sha1_process(ctx, (uint8_t*)(ctx->buffer));\n buf += fill;\n len -= fill;\n left = 0;\n@@ -1769,10 +1769,10 @@ void SHA1DCUpdate(SHA1_CTX* ctx, const char*\nbuf, size_t len)\n ctx->total += 64;\n\n #if defined(SHA1DC_ALLOW_UNALIGNED_ACCESS)\n- sha1_process(ctx, (uint32_t*)(buf));\n+ sha1_process(ctx, (uint8_t*)(buf));\n #else\n memcpy(ctx->buffer, buf, 64);\n- sha1_process(ctx, (uint32_t*)(ctx->buffer));\n+ sha1_process(ctx, (uint8_t*)(ctx->buffer));\n #endif /* defined(SHA1DC_ALLOW_UNALIGNED_ACCESS) */\n buf += 64;\n len -= 64;\n@@ -1809,7 +1809,7 @@ int SHA1DCFinal(unsigned char output[20], SHA1_CTX *ctx)\n ctx->buffer[61] = (unsigned char)(total >> 16);\n ctx->buffer[62] = (unsigned char)(total >> 8);\n ctx->buffer[63] = (unsigned char)(total);\n- sha1_process(ctx, (uint32_t*)(ctx->buffer));\n+ sha1_process(ctx, (uint8_t*)(ctx->buffer));\n output[0] = (unsigned char)(ctx->ihv[0] >> 24);\n output[1] = (unsigned char)(ctx->ihv[0] >> 16);\n output[2] = (unsigned char)(ctx->ihv[0] >> 8);\ndiff --git a/sha1dc/sha1.h b/sha1dc/sha1.h\nindex a0ff5d1..1d181a1 100644\n--- a/sha1dc/sha1.h\n+++ b/sha1dc/sha1.h\n@@ -18,7 +18,7 @@ extern \"C\" {\n\n /* sha-1 compression function that takes an already expanded message,\nand additionally store intermediate states */\n /* only stores states ii (the state between step ii-1 and step ii)\nwhen DOSTORESTATEii is defined in ubc_check.h */\n-void sha1_compression_states(uint32_t[5], const uint32_t[16],\nuint32_t[80], uint32_t[80][5]);\n+void sha1_compression_states(uint32_t[5], const uint8_t[64],\nuint32_t[80], uint32_t[80][5]);\n\n /*\n // Function type for sha1_recompression_step_T (uint32_t ihvin[5],\nuint32_t ihvout[5], const uint32_t me2[80], const uint32_t state[5]).\n\n\n\nWith this diff, various tests which seem relevant for SHA-1 pass,\nincluding t0013, and the UBSan-error is gone. The second diff is just\na monkey-patch. I have no reason to believe I will be able to come up\nwith a proper and complete patch for sha1dc. And I guess such a thing\nwould not really be Git's patch to carry, either. But at least Git\ncould consider whether to keep relying on undefined behavior or not.\n\nThere's a fair chance I've mangled the whitespace. I'm using gmail's\nweb interface... Sorry about that.\n"},{"id":"321362","messageId":"xmqq37bj454a.fsf@gitster.mtv.corp.google.com","threadId":"46087","inReplyTo":"CAN0heSp9DpW4_0QL57_oAHGu+os8k6yd=Z5+0MJnaL6iXTa-qQ@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-02T00:15:17Z","receivedAt":"2017-06-02T00:15:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> I looked into this some more. It turns out it is possible to trigger\n> undefined behavior on \"next\". Here's what I did:\n> ...\n>\n> This \"fixes\" the problem:\n> ...\n> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n> index 3dff80a..d6f4c44 100644\n> --- a/sha1dc/sha1.c\n> +++ b/sha1dc/sha1.c\n> @@ -66,9 +66,9 @@\n> ...\n> With this diff, various tests which seem relevant for SHA-1 pass,\n> including t0013, and the UBSan-error is gone. The second diff is just\n> a monkey-patch. I have no reason to believe I will be able to come up\n> with a proper and complete patch for sha1dc. And I guess such a thing\n> would not really be Git's patch to carry, either. But at least Git\n> could consider whether to keep relying on undefined behavior or not.\n>\n> There's a fair chance I've mangled the whitespace. I'm using gmail's\n> web interface... Sorry about that.\n\nThanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\nthat the final \"fix\" would come from his sha1collisiondetection\nrepository via Ævar.\n\nIn the meantime, I am wondering if it makes sense to merge the\nearlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\nSHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\nwould at least unblock those on platforms v2.13.0 did not work\ncorrectly at all.\n\nÆvar, thoughts?\n"},{"id":"321396","messageId":"CACBZZX7EvUqH28uni+r=RUBXb9=WTp732B4=rq+ViD_kecxZaw@mail.gmail.com","threadId":"46087","inReplyTo":"xmqq37bj454a.fsf@gitster.mtv.corp.google.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-02T08:51:40Z","receivedAt":"2017-06-02T08:53:16Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> I looked into this some more. It turns out it is possible to trigger\n>> undefined behavior on \"next\". Here's what I did:\n>> ...\n>>\n>> This \"fixes\" the problem:\n>> ...\n>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>> index 3dff80a..d6f4c44 100644\n>> --- a/sha1dc/sha1.c\n>> +++ b/sha1dc/sha1.c\n>> @@ -66,9 +66,9 @@\n>> ...\n>> With this diff, various tests which seem relevant for SHA-1 pass,\n>> including t0013, and the UBSan-error is gone. The second diff is just\n>> a monkey-patch. I have no reason to believe I will be able to come up\n>> with a proper and complete patch for sha1dc. And I guess such a thing\n>> would not really be Git's patch to carry, either. But at least Git\n>> could consider whether to keep relying on undefined behavior or not.\n>>\n>> There's a fair chance I've mangled the whitespace. I'm using gmail's\n>> web interface... Sorry about that.\n>\n> Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n> that the final \"fix\" would come from his sha1collisiondetection\n> repository via Ævar.\n>\n> In the meantime, I am wondering if it makes sense to merge the\n> earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n> SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n> would at least unblock those on platforms v2.13.0 did not work\n> correctly at all.\n>\n> Ævar, thoughts?\n\nI think we're mixing up several things here, which need to be untangled:\n\n1) The sha1dc works just fine on most platforms even with undefined\nbehavior, as evidenced by 2.13.0 working.\n\n2) There was a bug in practice with unaligned access on SPARC. It's\nnot clear to me whether anyone (Andreas, Liam?) still has any issues\nin practice on any platform without specifying compile flags like what\nMartin Ågren suggested above.\n\nAndreas: Is your initial report of unaligned access here fixed in the\nnext branch with my \"sha1dc: update from upstream\" commit? You didn't\nsay what platform you were on.\n\nLiam: How about your issue on SPARC?\n\n3) Now we have another issue reported by Martin Ågren here, which is\nthat while the code works in practice on most platforms it's using\nundefined behavior. On my GCC 7.1.1 it's sufficient to:\n\n make -j8 CFLAGS=\"-fsanitize=undefined\n-fsanitize-recover=undefined\" LDFLAGS=\"-fsanitize=undefined\n-fsanitize-recover=undefined\" all\n\nAnd then run e.g.:\n\n ./t0020-crlf.sh -v\n\nTo get spiel like:\n\n sha1dc/sha1.c:346:2: runtime error: load of misaligned address\n0x5610bf16d005 for type 'const uint32_t', which requires 4 byte\nalignment\n 0x5610bf16d005: note: pointer points here\n 65 6e 74 20 66 30 34 66 61 39 37 36 36 64 62 62 38 65 34 63 37\n33 38 34 37 30 61 31 36 63 61 62\n\nI think that this is definitely something worth looking into /\ncoordinating with upstream, but I haven't seen anything to suggest\nthat we need to be rushing to get a patch in to fix this given 1) and\nnobody saying yet that 2) doesn't solve their issue as long as they're\nnot supplying some -fsanitize=* flags.\n\nNow, stepping a bit back from this whole thing: I didn't read the\nentire discussion back in February when sha1dc was integrated, but I\nreally don't see given all this churn / bug reporting we're getting\nnow why another acceptable solution wouldn't be to just revert\ne6b07da278 (\"Makefile: make DC_SHA1 the default\", 2017-03-17) &\nrelease 2.13.1 with that.\n\nClearly there are outstanding issues with it, and needing to do a\nmemcpy() as my `next` patch does will harm performance on some\nplatforms, and something like Martin's patch on top will slow it down\neven more.\n\nIt seems to me that we should give it more time to cook, and better\nunderstand the various trade-offs involved. The shattered attack is\nvery unlikely to impact anything in practice, and users who are\nparanoid about it can opt-in to this extra protection.\n"},{"id":"321397","messageId":"CAN0heSq3CSe=Hgygtzd+ZM4rW-qM1_chrNd7Pq0KnYnKEVXcpw@mail.gmail.com","threadId":"46087","inReplyTo":"CACBZZX7EvUqH28uni+r=RUBXb9=WTp732B4=rq+ViD_kecxZaw@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-06-02T09:49:54Z","receivedAt":"2017-06-02T09:50:05Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 2 June 2017 at 10:51, Ævar Arnfjörð Bjarmason wrote:\n> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n>> Martin Ågren writes:\n>>\n>>> I looked into this some more. It turns out it is possible to trigger\n>>> undefined behavior on \"next\". Here's what I did:\n>>> ...\n>>>\n>>> This \"fixes\" the problem:\n>>> ...\n>>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>>> index 3dff80a..d6f4c44 100644\n>>> --- a/sha1dc/sha1.c\n>>> +++ b/sha1dc/sha1.c\n>>> @@ -66,9 +66,9 @@\n>>> ...\n>>> With this diff, various tests which seem relevant for SHA-1 pass,\n>>> including t0013, and the UBSan-error is gone. The second diff is just\n>>> a monkey-patch. I have no reason to believe I will be able to come up\n>>> with a proper and complete patch for sha1dc. And I guess such a thing\n>>> would not really be Git's patch to carry, either. But at least Git\n>>> could consider whether to keep relying on undefined behavior or not.\n>>>\n>>> There's a fair chance I've mangled the whitespace. I'm using gmail's\n>>> web interface... Sorry about that.\n>>\n>> Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n>> that the final \"fix\" would come from his sha1collisiondetection\n>> repository via Ævar.\n>>\n>> In the meantime, I am wondering if it makes sense to merge the\n>> earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n>> SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n>> would at least unblock those on platforms v2.13.0 did not work\n>> correctly at all.\n>>\n>> Ævar, thoughts?\n>\n> I think we're mixing up several things here, which need to be untangled:\n>\n> 1) The sha1dc works just fine on most platforms even with undefined\n> behavior, as evidenced by 2.13.0 working.\n\nRight, with \"platform\" meaning \"combination of hardware-architecture\nand compiler\". Nothing can be said about how the current code behaves\non \"x86\". Such statements can only be made with regard to \"x86 and\nthis or that compiler\". Even then, short of studying the compiler\nimplementation/documentation in detail, one cannot be certain that\nseemingly unrelated changes in Git don't make the code do something\nelse entirely.\n\n> 2) There was a bug in practice with unaligned access on SPARC. It's\n> not clear to me whether anyone (Andreas, Liam?) still has any issues\n> in practice on any platform without specifying compile flags like what\n> Martin Ågren suggested above.\n\nTrue.\n\n> 3) Now we have another issue reported by Martin Ågren here, which is\n> that while the code works in practice on most platforms it's using\n> undefined behavior.\n...\n> I think that this is definitely something worth looking into /\n> coordinating with upstream, but I haven't seen anything to suggest\n> that we need to be rushing to get a patch in to fix this given 1) and\n> nobody saying yet that 2) doesn't solve their issue as long as they're\n> not supplying some -fsanitize=* flags.\n>\n> Now, stepping a bit back from this whole thing: I didn't read the\n> entire discussion back in February when sha1dc was integrated, but I\n> really don't see given all this churn / bug reporting we're getting\n> now why another acceptable solution wouldn't be to just revert\n> e6b07da278 (\"Makefile: make DC_SHA1 the default\", 2017-03-17) &\n> release 2.13.1 with that.\n>\n> Clearly there are outstanding issues with it, and needing to do a\n> memcpy() as my `next` patch does will harm performance on some\n> platforms, and something like Martin's patch on top will slow it down\n> even more.\n\nThe only thing in my second \"patch\" which could possibly affect\nperformance as I see it would be the call to memcpy(.. ,.. ,4),\nincluding pointer-calculation. Focusing on x86, I would not say that\nit \"will\" slow it down until I'd measured performance. I wouldn't even\nrule out that the compiled assembler could be identical. I would just\nsay the patch \"would most likely slow it down even more on some\narchitectures, with some compilers and/or with some\ncompiler-settings\".\n\nIf undefined behavior is avoided with memcpy(.., .., 4) then there\nshould be no formal need for your \"big\" memcpy where things are copied\ninto a known-to-be-aligned buffer. The behavior will be defined on all\narchitectures, anyway. Then your memcpy would simply be part of an\noptimization to prefer one big memcpy and many loads instead of many\nsmall memcpy-calls. On some architectures, that might be a very good\noptimization. But on others, if the small memcpy is compiled to a\nsimple load, then I believe such an optimization would most likely be\na slow-down (modulo crazy-clever compiler optimizations).\n\n> It seems to me that we should give it more time to cook, and better\n> understand the various trade-offs involved. The shattered attack is\n> very unlikely to impact anything in practice, and users who are\n> paranoid about it can opt-in to this extra protection.\n\nRegarding reverting and cooking, I don't feel like I'm in a position\nto express an opinion. Thanks for thinking about this undefined\nbehavior, though, and I hope I'm contributing in some way, although\nI'm aware I'm just standing at the side-line, waving my hands, and not\ncontributing any actual code.\n"},{"id":"321406","messageId":"20170602144622.xottin6efikpkdel@oracle.com","threadId":"46087","inReplyTo":"CACBZZX7EvUqH28uni+r=RUBXb9=WTp732B4=rq+ViD_kecxZaw@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Liam R. Howlett","fromEmail":"liam.howlett@oracle.com","sentAt":"2017-06-02T14:46:23Z","receivedAt":"2017-06-02T14:46:55Z","isPatch":false,"sender":{"key":"liam.howlett@oracle.com","avatar":null},"body":"* ?var Arnfj?r? Bjarmason [170602 04:53]:\n> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n> > Martin Ågren writes:\n> >\n> >> I looked into this some more. It turns out it is possible to trigger\n> >> undefined behavior on \"next\". Here's what I did:\n> >> ...\n> >>\n> >> This \"fixes\" the problem:\n> >> ...\n> >> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n> >> index 3dff80a..d6f4c44 100644\n> >> --- a/sha1dc/sha1.c\n> >> +++ b/sha1dc/sha1.c\n> >> @@ -66,9 +66,9 @@\n> >> ...\n> >> With this diff, various tests which seem relevant for SHA-1 pass,\n> >> including t0013, and the UBSan-error is gone. The second diff is just\n> >> a monkey-patch. I have no reason to believe I will be able to come up\n> >> with a proper and complete patch for sha1dc. And I guess such a thing\n> >> would not really be Git's patch to carry, either. But at least Git\n> >> could consider whether to keep relying on undefined behavior or not.\n> >>\n> >> There's a fair chance I've mangled the whitespace. I'm using gmail's\n> >> web interface... Sorry about that.\n> >\n> > Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n> > that the final \"fix\" would come from his sha1collisiondetection\n> > repository via Ævar.\n> >\n> > In the meantime, I am wondering if it makes sense to merge the\n> > earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n> > SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n> > would at least unblock those on platforms v2.13.0 did not work\n> > correctly at all.\n> >\n> > Ævar, thoughts?\n> \n> I think we're mixing up several things here, which need to be untangled:\n> \n> 1) The sha1dc works just fine on most platforms even with undefined\n> behavior, as evidenced by 2.13.0 working.\n> \n> 2) There was a bug in practice with unaligned access on SPARC. It's\n> not clear to me whether anyone (Andreas, Liam?) still has any issues\n> in practice on any platform without specifying compile flags like what\n> Martin Ågren suggested above.\n> \n> Andreas: Is your initial report of unaligned access here fixed in the\n> next branch with my \"sha1dc: update from upstream\" commit? You didn't\n> say what platform you were on.\n> \n> Liam: How about your issue on SPARC?\n\n2.13.0 is very much broken for me on SPARC.\n{maint//git} $ make -j120\n[...]\n{maint//git} $ ./git log\n[1] 1004506 bus error (core dumped) ./git log\n\nThis is with b06d36431 (maint).\n\nThe same thing happens on v2.13.0-384-g826c06412 (master).\n\nv2.13.0-539-g4b9c06c7d (next) works for me, as did following the\ninstructions on upgrading the sha1dc code myself.\n\n> \n> 3) Now we have another issue reported by Martin Ågren here, which is\n> that while the code works in practice on most platforms it's using\n> undefined behavior. On my GCC 7.1.1 it's sufficient to:\n\nMy platforms gcc is older than 7.1.1.\n\n> \n> make -j8 CFLAGS=\"-fsanitize=undefined\n> -fsanitize-recover=undefined\" LDFLAGS=\"-fsanitize=undefined\n> -fsanitize-recover=undefined\" all\n> \n> And then run e.g.:\n> \n> ./t0020-crlf.sh -v\n\nThese tests pass With my older gcc - which those flags are not\nrecognized.\n\n# passed all 35 test(s)\n\n\n> \n> To get spiel like:\n> \n> sha1dc/sha1.c:346:2: runtime error: load of misaligned address\n> 0x5610bf16d005 for type 'const uint32_t', which requires 4 byte\n> alignment\n> 0x5610bf16d005: note: pointer points here\n> 65 6e 74 20 66 30 34 66 61 39 37 36 36 64 62 62 38 65 34 63 37\n> 33 38 34 37 30 61 31 36 63 61 62\n> \n> I think that this is definitely something worth looking into /\n> coordinating with upstream, but I haven't seen anything to suggest\n> that we need to be rushing to get a patch in to fix this given 1) and\n> nobody saying yet that 2) doesn't solve their issue as long as they're\n> not supplying some -fsanitize=* flags.\n> \n> Now, stepping a bit back from this whole thing: I didn't read the\n> entire discussion back in February when sha1dc was integrated, but I\n> really don't see given all this churn / bug reporting we're getting\n> now why another acceptable solution wouldn't be to just revert\n> e6b07da278 (\"Makefile: make DC_SHA1 the default\", 2017-03-17) &\n> release 2.13.1 with that.\n> \n> Clearly there are outstanding issues with it, and needing to do a\n> memcpy() as my `next` patch does will harm performance on some\n> platforms, and something like Martin's patch on top will slow it down\n> even more.\n> \n> It seems to me that we should give it more time to cook, and better\n> understand the various trade-offs involved. The shattered attack is\n> very unlikely to impact anything in practice, and users who are\n> paranoid about it can opt-in to this extra protection.\n\nI have not seen issues with DC_SAH1 with the newer code base on SPARC.\n\nThanks,\nLiam\n"},{"id":"321410","messageId":"CACBZZX5iSxKz9p1V5h=t0+QtrY75g6haqRqMu7GEfrJHpWkefA@mail.gmail.com","threadId":"46087","inReplyTo":"20170602144622.xottin6efikpkdel@oracle.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-02T16:53:11Z","receivedAt":"2017-06-02T16:53:37Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jun 2, 2017 at 4:46 PM, Liam R. Howlett wrote:\n> * ?var Arnfj?r? Bjarmason [170602 04:53]:\n>> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n>> > Martin Ågren writes:\n>> >\n>> >> I looked into this some more. It turns out it is possible to trigger\n>> >> undefined behavior on \"next\". Here's what I did:\n>> >> ...\n>> >>\n>> >> This \"fixes\" the problem:\n>> >> ...\n>> >> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>> >> index 3dff80a..d6f4c44 100644\n>> >> --- a/sha1dc/sha1.c\n>> >> +++ b/sha1dc/sha1.c\n>> >> @@ -66,9 +66,9 @@\n>> >> ...\n>> >> With this diff, various tests which seem relevant for SHA-1 pass,\n>> >> including t0013, and the UBSan-error is gone. The second diff is just\n>> >> a monkey-patch. I have no reason to believe I will be able to come up\n>> >> with a proper and complete patch for sha1dc. And I guess such a thing\n>> >> would not really be Git's patch to carry, either. But at least Git\n>> >> could consider whether to keep relying on undefined behavior or not.\n>> >>\n>> >> There's a fair chance I've mangled the whitespace. I'm using gmail's\n>> >> web interface... Sorry about that.\n>> >\n>> > Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n>> > that the final \"fix\" would come from his sha1collisiondetection\n>> > repository via Ævar.\n>> >\n>> > In the meantime, I am wondering if it makes sense to merge the\n>> > earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n>> > SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n>> > would at least unblock those on platforms v2.13.0 did not work\n>> > correctly at all.\n>> >\n>> > Ævar, thoughts?\n>>\n>> I think we're mixing up several things here, which need to be untangled:\n>>\n>> 1) The sha1dc works just fine on most platforms even with undefined\n>> behavior, as evidenced by 2.13.0 working.\n>>\n>> 2) There was a bug in practice with unaligned access on SPARC. It's\n>> not clear to me whether anyone (Andreas, Liam?) still has any issues\n>> in practice on any platform without specifying compile flags like what\n>> Martin Ågren suggested above.\n>>\n>> Andreas: Is your initial report of unaligned access here fixed in the\n>> next branch with my \"sha1dc: update from upstream\" commit? You didn't\n>> say what platform you were on.\n>>\n>> Liam: How about your issue on SPARC?\n>\n> 2.13.0 is very much broken for me on SPARC.\n> {maint//git} $ make -j120\n> [...]\n> {maint//git} $ ./git log\n> [1] 1004506 bus error (core dumped) ./git log\n>\n> This is with b06d36431 (maint).\n>\n> The same thing happens on v2.13.0-384-g826c06412 (master).\n>\n> v2.13.0-539-g4b9c06c7d (next) works for me, as did following the\n> instructions on upgrading the sha1dc code myself.\n\nThanks a lot. So that works as I suspect on SPARC, hopefully it'll be\nin master (and 2.13.1) soon.\n\n>>\n>> 3) Now we have another issue reported by Martin Ågren here, which is\n>> that while the code works in practice on most platforms it's using\n>> undefined behavior. On my GCC 7.1.1 it's sufficient to:\n>\n> My platforms gcc is older than 7.1.1.\n\nRight, shouldn't matter. Just thought I'd note the version for context\nto note what version was producing that warning.\n\n>>\n>> make -j8 CFLAGS=\"-fsanitize=undefined\n>> -fsanitize-recover=undefined\" LDFLAGS=\"-fsanitize=undefined\n>> -fsanitize-recover=undefined\" all\n>>\n>> And then run e.g.:\n>>\n>> ./t0020-crlf.sh -v\n>\n> These tests pass With my older gcc - which those flags are not\n> recognized.\n>\n> # passed all 35 test(s)\n>\n>\n>>\n>> To get spiel like:\n>>\n>> sha1dc/sha1.c:346:2: runtime error: load of misaligned address\n>> 0x5610bf16d005 for type 'const uint32_t', which requires 4 byte\n>> alignment\n>> 0x5610bf16d005: note: pointer points here\n>> 65 6e 74 20 66 30 34 66 61 39 37 36 36 64 62 62 38 65 34 63 37\n>> 33 38 34 37 30 61 31 36 63 61 62\n>>\n>> I think that this is definitely something worth looking into /\n>> coordinating with upstream, but I haven't seen anything to suggest\n>> that we need to be rushing to get a patch in to fix this given 1) and\n>> nobody saying yet that 2) doesn't solve their issue as long as they're\n>> not supplying some -fsanitize=* flags.\n>>\n>> Now, stepping a bit back from this whole thing: I didn't read the\n>> entire discussion back in February when sha1dc was integrated, but I\n>> really don't see given all this churn / bug reporting we're getting\n>> now why another acceptable solution wouldn't be to just revert\n>> e6b07da278 (\"Makefile: make DC_SHA1 the default\", 2017-03-17) &\n>> release 2.13.1 with that.\n>>\n>> Clearly there are outstanding issues with it, and needing to do a\n>> memcpy() as my `next` patch does will harm performance on some\n>> platforms, and something like Martin's patch on top will slow it down\n>> even more.\n>>\n>> It seems to me that we should give it more time to cook, and better\n>> understand the various trade-offs involved. The shattered attack is\n>> very unlikely to impact anything in practice, and users who are\n>> paranoid about it can opt-in to this extra protection.\n>\n> I have not seen issues with DC_SAH1 with the newer code base on SPARC.\n\nGreat, thanks!\n"},{"id":"321424","messageId":"CACBZZX5re5Ge1OzxYOE42nwBhhusya6=M9An08X-KzaqNH9Cog@mail.gmail.com","threadId":"46087","inReplyTo":"CAN0heSq3CSe=Hgygtzd+ZM4rW-qM1_chrNd7Pq0KnYnKEVXcpw@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-02T19:32:12Z","receivedAt":"2017-06-02T19:32:49Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jun 2, 2017 at 11:49 AM, Martin Ågren wrote:\n> On 2 June 2017 at 10:51, Ævar Arnfjörð Bjarmason wrote:\n>> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n>>> Martin Ågren writes:\n>>>\n>>>> I looked into this some more. It turns out it is possible to trigger\n>>>> undefined behavior on \"next\". Here's what I did:\n>>>> ...\n>>>>\n>>>> This \"fixes\" the problem:\n>>>> ...\n>>>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>>>> index 3dff80a..d6f4c44 100644\n>>>> --- a/sha1dc/sha1.c\n>>>> +++ b/sha1dc/sha1.c\n>>>> @@ -66,9 +66,9 @@\n>>>> ...\n>>>> With this diff, various tests which seem relevant for SHA-1 pass,\n>>>> including t0013, and the UBSan-error is gone. The second diff is just\n>>>> a monkey-patch. I have no reason to believe I will be able to come up\n>>>> with a proper and complete patch for sha1dc. And I guess such a thing\n>>>> would not really be Git's patch to carry, either. But at least Git\n>>>> could consider whether to keep relying on undefined behavior or not.\n>>>>\n>>>> There's a fair chance I've mangled the whitespace. I'm using gmail's\n>>>> web interface... Sorry about that.\n>>>\n>>> Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n>>> that the final \"fix\" would come from his sha1collisiondetection\n>>> repository via Ævar.\n>>>\n>>> In the meantime, I am wondering if it makes sense to merge the\n>>> earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n>>> SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n>>> would at least unblock those on platforms v2.13.0 did not work\n>>> correctly at all.\n>>>\n>>> Ævar, thoughts?\n>>\n>> I think we're mixing up several things here, which need to be untangled:\n>>\n>> 1) The sha1dc works just fine on most platforms even with undefined\n>> behavior, as evidenced by 2.13.0 working.\n>\n> Right, with \"platform\" meaning \"combination of hardware-architecture\n> and compiler\". Nothing can be said about how the current code behaves\n> on \"x86\". Such statements can only be made with regard to \"x86 and\n> this or that compiler\". Even then, short of studying the compiler\n> implementation/documentation in detail, one cannot be certain that\n> seemingly unrelated changes in Git don't make the code do something\n> else entirely.\n\nI think you're veering into a theoretical discussion here that has\nlittle to no bearing on the practicalities involved here.\n\nYes if something is undefined behavior in C the compiler &\narchitecture is free to do anything they want with it. In practice\nlots of undefined behavior is de-facto standardized across various\nplatforms.\n\nAs far as I can tell unaligned access is one of those things. I don't\nthink there's ever been an x86 chip / compiler that would run this\ncode with any semantic differences when it comes to unaligned access,\nand such a chip / compiler is unlikely to ever exist.\n\nI'm not advocating that we rely on undefined behavior willy-nilly,\njust that we should consider the real situation is (i.e. what actual\narchitectures / compilers are doing or are likely to do) as opposed to\nthe purely theoretical (if you gave a bunch of aliens who'd never\nheard of our technology the ANSI C standard to implement from\nscratch).\n\nHere's a performance test of your patch above against p3400-rebase.sh.\nI don't know how much these error bars from t/perf can be trusted.\nThis is over 30 runs with -O3:\n\n- 3400.2: rebase on top of a lot of unrelated changes\n v2.12.0 : 1.25(1.10+0.06)\n v2.13.0 : 1.21(1.06+0.06) -3.2%\n origin/next : 1.22(1.04+0.07) -2.4%\n martin : 1.23(1.06+0.07) -1.6%\n- 3400.4: rebase a lot of unrelated changes without split-index\n v2.12.0 : 6.49(3.60+0.52)\n v2.13.0 : 8.21(4.18+0.55) +26.5%\n origin/next : 8.27(4.34+0.64) +27.4%\n martin : 8.80(4.36+0.62) +35.6%\n- 3400.6: rebase a lot of unrelated changes with split-index\n v2.12.0 : 6.77(3.56+0.51)\n v2.13.0 : 4.09(2.67+0.38) -39.6%\n origin/next : 4.13(2.70+0.36) -39.0%\n martin : 4.30(2.80+0.32) -36.5%\n\nAnd just your patch v.s. next:\n\n- 3400.2: rebase on top of a lot of unrelated changes\n origin/next : 1.22(1.06+0.06)\n martin : 1.22(1.06+0.05) +0.0%\n- 3400.4: rebase a lot of unrelated changes without split-index\n origin/next : 7.54(4.13+0.60)\n martin : 7.75(4.34+0.67) +2.8%\n- 3400.6: rebase a lot of unrelated changes with split-index\n origin/next : 4.19(2.92+0.31)\n martin : 4.14(2.84+0.39) -1.2%\n\nIt seems to be a bit slower, is that speedup worth the use of\nunaligned access? I genuinely don't know. I'm just interested to find\nwhat if anything we need to urgently fix in a release version of git.\n\nOne data point there is that the fallback blk-sha1 implementation\nwe've shipped since 2009 has the same errors about unaligned access as\nbefore your patch with -fsanitize[..], and looking at the commit\nmessage this was something Linus knew about at the time, see\nd7c208a92e (\"Add new optimized C 'block-sha1' routines\", 2009-08-05).\n\nThat's strong empirical data suggesting that whatever we want to do\nabout unaligned access in the future it's not something which in\npractice would cause wrong sha1 results for the implementation\nshipping with v2.13.0.\n\nAs an aside, when I was trying to apply your patch I made a mistake,\nand found that git's test suite could run 100% OK with a bad sha1\nimplementation, I didn't apply this part (word diff):\n\ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex 04b104fe58..d6f4c442b5 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -312 +312 @@ static void sha1_compression_W(uint32_t ihv[5], const\nuint32_t W[80])\nvoid sha1_compression_states(uint32_t ihv[5], const\n[-uint32_t-]{+uint8_t+} m[64], uint32_t W[80], uint32_t states[80][5])\n@@ -1642 +1642 @@ static void sha1_recompression_step(uint32_t step,\nuint32_t ihvin[5], uint32_t i\nstatic void sha1_process(SHA1_CTX* ctx, const [-uint32_t-]{+uint8_t+} block[64])\ndiff --git a/sha1dc/sha1.h b/sha1dc/sha1.h\nindex 1d1d2b8d7c..1d181a1403 100644\n--- a/sha1dc/sha1.h\n+++ b/sha1dc/sha1.h\n@@ -21 +21 @@ extern \"C\" {\nvoid sha1_compression_states(uint32_t[5], const\n[-uint32_t-]{+uint8_t+}[64], uint32_t[80], uint32_t[80][5]);\n\nThe p3400-rebase.sh test will fail with your patch without that\nchange, but not any of the tests, gulp!\n\n>> 2) There was a bug in practice with unaligned access on SPARC. It's\n>> not clear to me whether anyone (Andreas, Liam?) still has any issues\n>> in practice on any platform without specifying compile flags like what\n>> Martin Ågren suggested above.\n>\n> True.\n>\n>> 3) Now we have another issue reported by Martin Ågren here, which is\n>> that while the code works in practice on most platforms it's using\n>> undefined behavior.\n> ...\n>> I think that this is definitely something worth looking into /\n>> coordinating with upstream, but I haven't seen anything to suggest\n>> that we need to be rushing to get a patch in to fix this given 1) and\n>> nobody saying yet that 2) doesn't solve their issue as long as they're\n>> not supplying some -fsanitize=* flags.\n>>\n>> Now, stepping a bit back from this whole thing: I didn't read the\n>> entire discussion back in February when sha1dc was integrated, but I\n>> really don't see given all this churn / bug reporting we're getting\n>> now why another acceptable solution wouldn't be to just revert\n>> e6b07da278 (\"Makefile: make DC_SHA1 the default\", 2017-03-17) &\n>> release 2.13.1 with that.\n>>\n>> Clearly there are outstanding issues with it, and needing to do a\n>> memcpy() as my `next` patch does will harm performance on some\n>> platforms, and something like Martin's patch on top will slow it down\n>> even more.\n>\n> The only thing in my second \"patch\" which could possibly affect\n> performance as I see it would be the call to memcpy(.. ,.. ,4),\n> including pointer-calculation. Focusing on x86, I would not say that\n> it \"will\" slow it down until I'd measured performance. I wouldn't even\n> rule out that the compiled assembler could be identical. I would just\n> say the patch \"would most likely slow it down even more on some\n> architectures, with some compilers and/or with some\n> compiler-settings\".\n>\n> If undefined behavior is avoided with memcpy(.., .., 4) then there\n> should be no formal need for your \"big\" memcpy where things are copied\n> into a known-to-be-aligned buffer. The behavior will be defined on all\n> architectures, anyway. Then your memcpy would simply be part of an\n> optimization to prefer one big memcpy and many loads instead of many\n> small memcpy-calls. On some architectures, that might be a very good\n> optimization. But on others, if the small memcpy is compiled to a\n> simple load, then I believe such an optimization would most likely be\n> a slow-down (modulo crazy-clever compiler optimizations).\n>\n>> It seems to me that we should give it more time to cook, and better\n>> understand the various trade-offs involved. The shattered attack is\n>> very unlikely to impact anything in practice, and users who are\n>> paranoid about it can opt-in to this extra protection.\n>\n> Regarding reverting and cooking, I don't feel like I'm in a position\n> to express an opinion. Thanks for thinking about this undefined\n> behavior, though, and I hope I'm contributing in some way, although\n> I'm aware I'm just standing at the side-line, waving my hands, and not\n> contributing any actual code.\n"},{"id":"321431","messageId":"CAN0heSoV3nPO1v=CWze4DfpnmBn7+wVLm3_4f4ouv+PSGMAd+w@mail.gmail.com","threadId":"46087","inReplyTo":"CACBZZX5re5Ge1OzxYOE42nwBhhusya6=M9An08X-KzaqNH9Cog@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-06-02T20:11:25Z","receivedAt":"2017-06-02T20:11:31Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 2 June 2017 at 21:32, Ævar Arnfjörð Bjarmason wrote:\n> On Fri, Jun 2, 2017 at 11:49 AM, Martin Ågren wrote:\n>> On 2 June 2017 at 10:51, Ævar Arnfjörð Bjarmason wrote:\n>>> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n>>>> Martin Ågren writes:\n>>>>\n>>>>> I looked into this some more. It turns out it is possible to trigger\n>>>>> undefined behavior on \"next\". Here's what I did:\n>>>>> ...\n>>>>>\n>>>>> This \"fixes\" the problem:\n>>>>> ...\n>>>>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>>>>> index 3dff80a..d6f4c44 100644\n>>>>> --- a/sha1dc/sha1.c\n>>>>> +++ b/sha1dc/sha1.c\n>>>>> @@ -66,9 +66,9 @@\n>>>>> ...\n>>>>> With this diff, various tests which seem relevant for SHA-1 pass,\n>>>>> including t0013, and the UBSan-error is gone. The second diff is just\n>>>>> a monkey-patch. I have no reason to believe I will be able to come up\n>>>>> with a proper and complete patch for sha1dc. And I guess such a thing\n>>>>> would not really be Git's patch to carry, either. But at least Git\n>>>>> could consider whether to keep relying on undefined behavior or not.\n>>>>>\n>>>>> There's a fair chance I've mangled the whitespace. I'm using gmail's\n>>>>> web interface... Sorry about that.\n>>>>\n>>>> Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n>>>> that the final \"fix\" would come from his sha1collisiondetection\n>>>> repository via Ævar.\n>>>>\n>>>> In the meantime, I am wondering if it makes sense to merge the\n>>>> earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n>>>> SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n>>>> would at least unblock those on platforms v2.13.0 did not work\n>>>> correctly at all.\n>>>>\n>>>> Ævar, thoughts?\n>>>\n>>> I think we're mixing up several things here, which need to be untangled:\n>>>\n>>> 1) The sha1dc works just fine on most platforms even with undefined\n>>> behavior, as evidenced by 2.13.0 working.\n>>\n>> Right, with \"platform\" meaning \"combination of hardware-architecture\n>> and compiler\". Nothing can be said about how the current code behaves\n>> on \"x86\". Such statements can only be made with regard to \"x86 and\n>> this or that compiler\". Even then, short of studying the compiler\n>> implementation/documentation in detail, one cannot be certain that\n>> seemingly unrelated changes in Git don't make the code do something\n>> else entirely.\n>\n> I think you're veering into a theoretical discussion here that has\n> little to no bearing on the practicalities involved here.\n>\n> Yes if something is undefined behavior in C the compiler &\n> architecture is free to do anything they want with it. In practice\n> lots of undefined behavior is de-facto standardized across various\n> platforms.\n>\n> As far as I can tell unaligned access is one of those things. I don't\n> think there's ever been an x86 chip / compiler that would run this\n> code with any semantic differences when it comes to unaligned access,\n> and such a chip / compiler is unlikely to ever exist.\n>\n> I'm not advocating that we rely on undefined behavior willy-nilly,\n> just that we should consider the real situation is (i.e. what actual\n> architectures / compilers are doing or are likely to do) as opposed to\n> the purely theoretical (if you gave a bunch of aliens who'd never\n> heard of our technology the ANSI C standard to implement from\n> scratch).\n\nYeah, that's an argument. I just thought I'd provide whatever input I\ncould, albeit in text form. The only thing that matters in the end is\nthat you (the Git project) feel that you make the correct decision,\npossibly going beyond \"theoretical\" reasoning into engineering-land.\n\nTake care,\nMartin\n"},{"id":"321434","messageId":"CACBZZX7BReh=ssqp2HezWZsy8359SVXbBtWiJJNtHYXRVu_hXQ@mail.gmail.com","threadId":"46087","inReplyTo":"CAN0heSoV3nPO1v=CWze4DfpnmBn7+wVLm3_4f4ouv+PSGMAd+w@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-02T20:14:51Z","receivedAt":"2017-06-02T20:15:23Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jun 2, 2017 at 10:11 PM, Martin Ågren wrote:\n> On 2 June 2017 at 21:32, Ævar Arnfjörð Bjarmason wrote:\n>> On Fri, Jun 2, 2017 at 11:49 AM, Martin Ågren wrote:\n>>> On 2 June 2017 at 10:51, Ævar Arnfjörð Bjarmason wrote:\n>>>> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n>>>>> Martin Ågren writes:\n>>>>>\n>>>>>> I looked into this some more. It turns out it is possible to trigger\n>>>>>> undefined behavior on \"next\". Here's what I did:\n>>>>>> ...\n>>>>>>\n>>>>>> This \"fixes\" the problem:\n>>>>>> ...\n>>>>>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>>>>>> index 3dff80a..d6f4c44 100644\n>>>>>> --- a/sha1dc/sha1.c\n>>>>>> +++ b/sha1dc/sha1.c\n>>>>>> @@ -66,9 +66,9 @@\n>>>>>> ...\n>>>>>> With this diff, various tests which seem relevant for SHA-1 pass,\n>>>>>> including t0013, and the UBSan-error is gone. The second diff is just\n>>>>>> a monkey-patch. I have no reason to believe I will be able to come up\n>>>>>> with a proper and complete patch for sha1dc. And I guess such a thing\n>>>>>> would not really be Git's patch to carry, either. But at least Git\n>>>>>> could consider whether to keep relying on undefined behavior or not.\n>>>>>>\n>>>>>> There's a fair chance I've mangled the whitespace. I'm using gmail's\n>>>>>> web interface... Sorry about that.\n>>>>>\n>>>>> Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n>>>>> that the final \"fix\" would come from his sha1collisiondetection\n>>>>> repository via Ævar.\n>>>>>\n>>>>> In the meantime, I am wondering if it makes sense to merge the\n>>>>> earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n>>>>> SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n>>>>> would at least unblock those on platforms v2.13.0 did not work\n>>>>> correctly at all.\n>>>>>\n>>>>> Ævar, thoughts?\n>>>>\n>>>> I think we're mixing up several things here, which need to be untangled:\n>>>>\n>>>> 1) The sha1dc works just fine on most platforms even with undefined\n>>>> behavior, as evidenced by 2.13.0 working.\n>>>\n>>> Right, with \"platform\" meaning \"combination of hardware-architecture\n>>> and compiler\". Nothing can be said about how the current code behaves\n>>> on \"x86\". Such statements can only be made with regard to \"x86 and\n>>> this or that compiler\". Even then, short of studying the compiler\n>>> implementation/documentation in detail, one cannot be certain that\n>>> seemingly unrelated changes in Git don't make the code do something\n>>> else entirely.\n>>\n>> I think you're veering into a theoretical discussion here that has\n>> little to no bearing on the practicalities involved here.\n>>\n>> Yes if something is undefined behavior in C the compiler &\n>> architecture is free to do anything they want with it. In practice\n>> lots of undefined behavior is de-facto standardized across various\n>> platforms.\n>>\n>> As far as I can tell unaligned access is one of those things. I don't\n>> think there's ever been an x86 chip / compiler that would run this\n>> code with any semantic differences when it comes to unaligned access,\n>> and such a chip / compiler is unlikely to ever exist.\n>>\n>> I'm not advocating that we rely on undefined behavior willy-nilly,\n>> just that we should consider the real situation is (i.e. what actual\n>> architectures / compilers are doing or are likely to do) as opposed to\n>> the purely theoretical (if you gave a bunch of aliens who'd never\n>> heard of our technology the ANSI C standard to implement from\n>> scratch).\n>\n> Yeah, that's an argument. I just thought I'd provide whatever input I\n> could, albeit in text form. The only thing that matters in the end is\n> that you (the Git project) feel that you make the correct decision,\n> possibly going beyond \"theoretical\" reasoning into engineering-land.\n\nI forgot to note, I think it would be very useful if you could submit\nthat patch of yours in cleaned up form to the upstream sha1dc project:\nhttps://github.com/cr-marcstevens/sha1collisiondetection\n\nThey might be interested in taking it, even if it's guarded by some\nmacro \"don't do unaligned access even on archs that seem OK with it\".\n\nMy comments are just focusing on this in the context of whether we\nshould be hotfixing our copy due to an issue in the wild, like e.g.\nthe SPARC issue.\n"},{"id":"321437","messageId":"CANgJU+UzoaN3Urj=L4unMMtNwFm6G8LGxx19g49AR5R+76F2OA@mail.gmail.com","threadId":"46087","inReplyTo":"CACBZZX5re5Ge1OzxYOE42nwBhhusya6=M9An08X-KzaqNH9Cog@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2017-06-02T20:17:40Z","receivedAt":"2017-06-02T20:17:48Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"On 2 June 2017 at 21:32, Ævar Arnfjörð Bjarmason wrote:\n> On Fri, Jun 2, 2017 at 11:49 AM, Martin Ågren wrote:\n>> On 2 June 2017 at 10:51, Ævar Arnfjörð Bjarmason wrote:\n>>> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n>>>> Martin Ågren writes:\n>>>>\n>>>>> I looked into this some more. It turns out it is possible to trigger\n>>>>> undefined behavior on \"next\". Here's what I did:\n>>>>> ...\n>>>>>\n>>>>> This \"fixes\" the problem:\n>>>>> ...\n>>>>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>>>>> index 3dff80a..d6f4c44 100644\n>>>>> --- a/sha1dc/sha1.c\n>>>>> +++ b/sha1dc/sha1.c\n>>>>> @@ -66,9 +66,9 @@\n>>>>> ...\n>>>>> With this diff, various tests which seem relevant for SHA-1 pass,\n>>>>> including t0013, and the UBSan-error is gone. The second diff is just\n>>>>> a monkey-patch. I have no reason to believe I will be able to come up\n>>>>> with a proper and complete patch for sha1dc. And I guess such a thing\n>>>>> would not really be Git's patch to carry, either. But at least Git\n>>>>> could consider whether to keep relying on undefined behavior or not.\n>>>>>\n>>>>> There's a fair chance I've mangled the whitespace. I'm using gmail's\n>>>>> web interface... Sorry about that.\n>>>>\n>>>> Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n>>>> that the final \"fix\" would come from his sha1collisiondetection\n>>>> repository via Ævar.\n>>>>\n>>>> In the meantime, I am wondering if it makes sense to merge the\n>>>> earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n>>>> SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n>>>> would at least unblock those on platforms v2.13.0 did not work\n>>>> correctly at all.\n>>>>\n>>>> Ævar, thoughts?\n>>>\n>>> I think we're mixing up several things here, which need to be untangled:\n>>>\n>>> 1) The sha1dc works just fine on most platforms even with undefined\n>>> behavior, as evidenced by 2.13.0 working.\n>>\n>> Right, with \"platform\" meaning \"combination of hardware-architecture\n>> and compiler\". Nothing can be said about how the current code behaves\n>> on \"x86\". Such statements can only be made with regard to \"x86 and\n>> this or that compiler\". Even then, short of studying the compiler\n>> implementation/documentation in detail, one cannot be certain that\n>> seemingly unrelated changes in Git don't make the code do something\n>> else entirely.\n>\n> I think you're veering into a theoretical discussion here that has\n> little to no bearing on the practicalities involved here.\n>\n> Yes if something is undefined behavior in C the compiler &\n> architecture is free to do anything they want with it. In practice\n> lots of undefined behavior is de-facto standardized across various\n> platforms.\n>\n> As far as I can tell unaligned access is one of those things. I don't\n> think there's ever been an x86 chip / compiler that would run this\n> code with any semantic differences when it comes to unaligned access,\n> and such a chip / compiler is unlikely to ever exist.\n>\n> I'm not advocating that we rely on undefined behavior willy-nilly,\n> just that we should consider the real situation is (i.e. what actual\n> architectures / compilers are doing or are likely to do) as opposed to\n> the purely theoretical (if you gave a bunch of aliens who'd never\n> heard of our technology the ANSI C standard to implement from\n> scratch).\n>\n> Here's a performance test of your patch above against p3400-rebase.sh.\n> I don't know how much these error bars from t/perf can be trusted.\n> This is over 30 runs with -O3:\n>\n> - 3400.2: rebase on top of a lot of unrelated changes\n> v2.12.0 : 1.25(1.10+0.06)\n> v2.13.0 : 1.21(1.06+0.06) -3.2%\n> origin/next : 1.22(1.04+0.07) -2.4%\n> martin : 1.23(1.06+0.07) -1.6%\n> - 3400.4: rebase a lot of unrelated changes without split-index\n> v2.12.0 : 6.49(3.60+0.52)\n> v2.13.0 : 8.21(4.18+0.55) +26.5%\n> origin/next : 8.27(4.34+0.64) +27.4%\n> martin : 8.80(4.36+0.62) +35.6%\n> - 3400.6: rebase a lot of unrelated changes with split-index\n> v2.12.0 : 6.77(3.56+0.51)\n> v2.13.0 : 4.09(2.67+0.38) -39.6%\n> origin/next : 4.13(2.70+0.36) -39.0%\n> martin : 4.30(2.80+0.32) -36.5%\n>\n> And just your patch v.s. next:\n>\n> - 3400.2: rebase on top of a lot of unrelated changes\n> origin/next : 1.22(1.06+0.06)\n> martin : 1.22(1.06+0.05) +0.0%\n> - 3400.4: rebase a lot of unrelated changes without split-index\n> origin/next : 7.54(4.13+0.60)\n> martin : 7.75(4.34+0.67) +2.8%\n> - 3400.6: rebase a lot of unrelated changes with split-index\n> origin/next : 4.19(2.92+0.31)\n> martin : 4.14(2.84+0.39) -1.2%\n>\n> It seems to be a bit slower, is that speedup worth the use of\n> unaligned access? I genuinely don't know. I'm just interested to find\n> what if anything we need to urgently fix in a release version of git.\n>\n> One data point there is that the fallback blk-sha1 implementation\n> we've shipped since 2009 has the same errors about unaligned access as\n> before your patch with -fsanitize[..], and looking at the commit\n> message this was something Linus knew about at the time, see\n> d7c208a92e (\"Add new optimized C 'block-sha1' routines\", 2009-08-05).\n>\n> That's strong empirical data suggesting that whatever we want to do\n> about unaligned access in the future it's not something which in\n> practice would cause wrong sha1 results for the implementation\n> shipping with v2.13.0.\n>\n> As an aside, when I was trying to apply your patch I made a mistake,\n> and found that git's test suite could run 100% OK with a bad sha1\n> implementation, I didn't apply this part (word diff):\n>\n> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n> index 04b104fe58..d6f4c442b5 100644\n> --- a/sha1dc/sha1.c\n> +++ b/sha1dc/sha1.c\n> @@ -312 +312 @@ static void sha1_compression_W(uint32_t ihv[5], const\n> uint32_t W[80])\n> void sha1_compression_states(uint32_t ihv[5], const\n> [-uint32_t-]{+uint8_t+} m[64], uint32_t W[80], uint32_t states[80][5])\n> @@ -1642 +1642 @@ static void sha1_recompression_step(uint32_t step,\n> uint32_t ihvin[5], uint32_t i\n> static void sha1_process(SHA1_CTX* ctx, const [-uint32_t-]{+uint8_t+} block[64])\n> diff --git a/sha1dc/sha1.h b/sha1dc/sha1.h\n> index 1d1d2b8d7c..1d181a1403 100644\n> --- a/sha1dc/sha1.h\n> +++ b/sha1dc/sha1.h\n> @@ -21 +21 @@ extern \"C\" {\n> void sha1_compression_states(uint32_t[5], const\n> [-uint32_t-]{+uint8_t+}[64], uint32_t[80], uint32_t[80][5]);\n\n\nThat change doesnt look right anyway.\n\nThe original code asserts that it will receive a pointer to 64\nuint32_t's as the second argument.\n\nThe changed code asserts that it will receive a pointer to 64\nuint8_t's as the second argument.\n\nIf you change the type you will need to change the *number* correspondingly.\n\nI would expect to see the uint32_t[64] to turn into uint8_t[256]\n\nI have noticed that gcc does not necessarily enforce these types of\ndeclarations and I believe that the change might not have any affect\nat all depending on how the pointers are being used. (C passes\npointers on the stack, so afaik these things are more hints than\nanything else.)\n\nLooking at the code it looks like it conflates endianness with\nalignedness when they arent the same thing, except that the majority\nof little-endian boxes are x86 which do not require aligned access,\nand many big-endian boxes do require aligned access. IOW, even they\nare often related they are distinct properties.\n\nMost hash function implementations have code like the following\n(extracted and reduced from hv_macro.h in perl.git [which only\nsupports little-endian hash functions]):\n\n#ifndef U32_ALIGNMENT_REQUIRED\n #if (BYTEORDER == 0x1234 || BYTEORDER == 0x12345678)\n #define U8TO32_LE(ptr) (*((const U32*)(ptr)))\n#elif (BYTEORDER == 0x4321 || BYTEORDER == 0x87654321)\n #if defined(__GNUC__) && (__GNUC__>4 || (__GNUC__==4 && __GNUC_MINOR__>=3))\n #define U8TO32_LE(ptr) (__builtin_bswap32(*((U32*)(ptr))))\n #endif\n #endif\n#endif\n\n#ifndef U8TO16_LE\n /* Without a known fast bswap32 we're just as well off doing this */\n #define U8TO32_LE(ptr)\n((U32)(ptr)[0]|(U32)(ptr)[1]<<8|(U32)(ptr)[2]<<16|(U32)(ptr)[3]<<24)\n#endif\n\nOf course, in the case of SHA1 it is defined as being big-endian\n(making it a relatively poor choice for use on x86), so you would need\nto reverse a bit of this logic.\n\nNote the form shown in that last block will work regardless of\nendianness or alignedness requirements and should be optimised\nproperly by most compilers into the most efficient operation.\n\nIn other words, if you guys just switch to that kind of pattern this\nproblem will go away everywhere, and the only issue you will have to\nconsider is if it makes some oddball platforms too slow.\n\nYves\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"321439","messageId":"CANgJU+XjVpc2vJ3bdY1MReRq0hZL3PPOQhNwAGvqrh7ZThLV-Q@mail.gmail.com","threadId":"46087","inReplyTo":"CACBZZX7BReh=ssqp2HezWZsy8359SVXbBtWiJJNtHYXRVu_hXQ@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2017-06-02T20:25:07Z","receivedAt":"2017-06-02T20:25:14Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"On 2 June 2017 at 22:14, Ævar Arnfjörð Bjarmason wrote:\n> On Fri, Jun 2, 2017 at 10:11 PM, Martin Ågren wrote:\n>> On 2 June 2017 at 21:32, Ævar Arnfjörð Bjarmason wrote:\n>>> On Fri, Jun 2, 2017 at 11:49 AM, Martin Ågren wrote:\n>>>> On 2 June 2017 at 10:51, Ævar Arnfjörð Bjarmason wrote:\n>>>>> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n>>>>>> Martin Ågren writes:\n>>>>>>\n>>>>>>> I looked into this some more. It turns out it is possible to trigger\n>>>>>>> undefined behavior on \"next\". Here's what I did:\n>>>>>>> ...\n>>>>>>>\n>>>>>>> This \"fixes\" the problem:\n>>>>>>> ...\n>>>>>>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>>>>>>> index 3dff80a..d6f4c44 100644\n>>>>>>> --- a/sha1dc/sha1.c\n>>>>>>> +++ b/sha1dc/sha1.c\n>>>>>>> @@ -66,9 +66,9 @@\n>>>>>>> ...\n>>>>>>> With this diff, various tests which seem relevant for SHA-1 pass,\n>>>>>>> including t0013, and the UBSan-error is gone. The second diff is just\n>>>>>>> a monkey-patch. I have no reason to believe I will be able to come up\n>>>>>>> with a proper and complete patch for sha1dc. And I guess such a thing\n>>>>>>> would not really be Git's patch to carry, either. But at least Git\n>>>>>>> could consider whether to keep relying on undefined behavior or not.\n>>>>>>>\n>>>>>>> There's a fair chance I've mangled the whitespace. I'm using gmail's\n>>>>>>> web interface... Sorry about that.\n>>>>>>\n>>>>>> Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n>>>>>> that the final \"fix\" would come from his sha1collisiondetection\n>>>>>> repository via Ævar.\n>>>>>>\n>>>>>> In the meantime, I am wondering if it makes sense to merge the\n>>>>>> earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n>>>>>> SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n>>>>>> would at least unblock those on platforms v2.13.0 did not work\n>>>>>> correctly at all.\n>>>>>>\n>>>>>> Ævar, thoughts?\n>>>>>\n>>>>> I think we're mixing up several things here, which need to be untangled:\n>>>>>\n>>>>> 1) The sha1dc works just fine on most platforms even with undefined\n>>>>> behavior, as evidenced by 2.13.0 working.\n>>>>\n>>>> Right, with \"platform\" meaning \"combination of hardware-architecture\n>>>> and compiler\". Nothing can be said about how the current code behaves\n>>>> on \"x86\". Such statements can only be made with regard to \"x86 and\n>>>> this or that compiler\". Even then, short of studying the compiler\n>>>> implementation/documentation in detail, one cannot be certain that\n>>>> seemingly unrelated changes in Git don't make the code do something\n>>>> else entirely.\n>>>\n>>> I think you're veering into a theoretical discussion here that has\n>>> little to no bearing on the practicalities involved here.\n>>>\n>>> Yes if something is undefined behavior in C the compiler &\n>>> architecture is free to do anything they want with it. In practice\n>>> lots of undefined behavior is de-facto standardized across various\n>>> platforms.\n>>>\n>>> As far as I can tell unaligned access is one of those things. I don't\n>>> think there's ever been an x86 chip / compiler that would run this\n>>> code with any semantic differences when it comes to unaligned access,\n>>> and such a chip / compiler is unlikely to ever exist.\n>>>\n>>> I'm not advocating that we rely on undefined behavior willy-nilly,\n>>> just that we should consider the real situation is (i.e. what actual\n>>> architectures / compilers are doing or are likely to do) as opposed to\n>>> the purely theoretical (if you gave a bunch of aliens who'd never\n>>> heard of our technology the ANSI C standard to implement from\n>>> scratch).\n>>\n>> Yeah, that's an argument. I just thought I'd provide whatever input I\n>> could, albeit in text form. The only thing that matters in the end is\n>> that you (the Git project) feel that you make the correct decision,\n>> possibly going beyond \"theoretical\" reasoning into engineering-land.\n>\n> I forgot to note, I think it would be very useful if you could submit\n> that patch of yours in cleaned up form to the upstream sha1dc project:\n> https://github.com/cr-marcstevens/sha1collisiondetection\n>\n> They might be interested in taking it, even if it's guarded by some\n> macro \"don't do unaligned access even on archs that seem OK with it\".\n>\n> My comments are just focusing on this in the context of whether we\n> should be hotfixing our copy due to an issue in the wild, like e.g.\n> the SPARC issue.\n\nA good way to get the sha1dc project properly tests on all platforms\nwould be to wrap it in a cpan distribution and let cpants (cpan\ntesters) test it for you on all the platforms under the sun.\n\nIn the Sereal project we found and fixed many portability issues with\nthe csnappy code simply because there are people testing modules in\nthe cpan world on every platform you can think of, and a few you might\nbe surprised to find out people still use.\n\nYves\n\nYves\n\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"321441","messageId":"CACBZZX5ZLGpZFnPcGiohMTAk9ShBZe0Mrx2s42xNhmK_zPApDg@mail.gmail.com","threadId":"46087","inReplyTo":"CANgJU+UzoaN3Urj=L4unMMtNwFm6G8LGxx19g49AR5R+76F2OA@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-02T20:38:39Z","receivedAt":"2017-06-02T20:39:06Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jun 2, 2017 at 10:17 PM, demerphq wrote:\n> On 2 June 2017 at 21:32, Ævar Arnfjörð Bjarmason wrote:\n>> On Fri, Jun 2, 2017 at 11:49 AM, Martin Ågren wrote:\n>>> On 2 June 2017 at 10:51, Ævar Arnfjörð Bjarmason wrote:\n>>>> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano wrote:\n>>>>> Martin Ågren writes:\n>>>>>\n>>>>>> I looked into this some more. It turns out it is possible to trigger\n>>>>>> undefined behavior on \"next\". Here's what I did:\n>>>>>> ...\n>>>>>>\n>>>>>> This \"fixes\" the problem:\n>>>>>> ...\n>>>>>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>>>>>> index 3dff80a..d6f4c44 100644\n>>>>>> --- a/sha1dc/sha1.c\n>>>>>> +++ b/sha1dc/sha1.c\n>>>>>> @@ -66,9 +66,9 @@\n>>>>>> ...\n>>>>>> With this diff, various tests which seem relevant for SHA-1 pass,\n>>>>>> including t0013, and the UBSan-error is gone. The second diff is just\n>>>>>> a monkey-patch. I have no reason to believe I will be able to come up\n>>>>>> with a proper and complete patch for sha1dc. And I guess such a thing\n>>>>>> would not really be Git's patch to carry, either. But at least Git\n>>>>>> could consider whether to keep relying on undefined behavior or not.\n>>>>>>\n>>>>>> There's a fair chance I've mangled the whitespace. I'm using gmail's\n>>>>>> web interface... Sorry about that.\n>>>>>\n>>>>> Thanks. I see Marc Stevens is CC'ed in the thread, so I'd expect\n>>>>> that the final \"fix\" would come from his sha1collisiondetection\n>>>>> repository via Ævar.\n>>>>>\n>>>>> In the meantime, I am wondering if it makes sense to merge the\n>>>>> earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef\n>>>>> SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which\n>>>>> would at least unblock those on platforms v2.13.0 did not work\n>>>>> correctly at all.\n>>>>>\n>>>>> Ævar, thoughts?\n>>>>\n>>>> I think we're mixing up several things here, which need to be untangled:\n>>>>\n>>>> 1) The sha1dc works just fine on most platforms even with undefined\n>>>> behavior, as evidenced by 2.13.0 working.\n>>>\n>>> Right, with \"platform\" meaning \"combination of hardware-architecture\n>>> and compiler\". Nothing can be said about how the current code behaves\n>>> on \"x86\". Such statements can only be made with regard to \"x86 and\n>>> this or that compiler\". Even then, short of studying the compiler\n>>> implementation/documentation in detail, one cannot be certain that\n>>> seemingly unrelated changes in Git don't make the code do something\n>>> else entirely.\n>>\n>> I think you're veering into a theoretical discussion here that has\n>> little to no bearing on the practicalities involved here.\n>>\n>> Yes if something is undefined behavior in C the compiler &\n>> architecture is free to do anything they want with it. In practice\n>> lots of undefined behavior is de-facto standardized across various\n>> platforms.\n>>\n>> As far as I can tell unaligned access is one of those things. I don't\n>> think there's ever been an x86 chip / compiler that would run this\n>> code with any semantic differences when it comes to unaligned access,\n>> and such a chip / compiler is unlikely to ever exist.\n>>\n>> I'm not advocating that we rely on undefined behavior willy-nilly,\n>> just that we should consider the real situation is (i.e. what actual\n>> architectures / compilers are doing or are likely to do) as opposed to\n>> the purely theoretical (if you gave a bunch of aliens who'd never\n>> heard of our technology the ANSI C standard to implement from\n>> scratch).\n>>\n>> Here's a performance test of your patch above against p3400-rebase.sh.\n>> I don't know how much these error bars from t/perf can be trusted.\n>> This is over 30 runs with -O3:\n>>\n>> - 3400.2: rebase on top of a lot of unrelated changes\n>> v2.12.0 : 1.25(1.10+0.06)\n>> v2.13.0 : 1.21(1.06+0.06) -3.2%\n>> origin/next : 1.22(1.04+0.07) -2.4%\n>> martin : 1.23(1.06+0.07) -1.6%\n>> - 3400.4: rebase a lot of unrelated changes without split-index\n>> v2.12.0 : 6.49(3.60+0.52)\n>> v2.13.0 : 8.21(4.18+0.55) +26.5%\n>> origin/next : 8.27(4.34+0.64) +27.4%\n>> martin : 8.80(4.36+0.62) +35.6%\n>> - 3400.6: rebase a lot of unrelated changes with split-index\n>> v2.12.0 : 6.77(3.56+0.51)\n>> v2.13.0 : 4.09(2.67+0.38) -39.6%\n>> origin/next : 4.13(2.70+0.36) -39.0%\n>> martin : 4.30(2.80+0.32) -36.5%\n>>\n>> And just your patch v.s. next:\n>>\n>> - 3400.2: rebase on top of a lot of unrelated changes\n>> origin/next : 1.22(1.06+0.06)\n>> martin : 1.22(1.06+0.05) +0.0%\n>> - 3400.4: rebase a lot of unrelated changes without split-index\n>> origin/next : 7.54(4.13+0.60)\n>> martin : 7.75(4.34+0.67) +2.8%\n>> - 3400.6: rebase a lot of unrelated changes with split-index\n>> origin/next : 4.19(2.92+0.31)\n>> martin : 4.14(2.84+0.39) -1.2%\n>>\n>> It seems to be a bit slower, is that speedup worth the use of\n>> unaligned access? I genuinely don't know. I'm just interested to find\n>> what if anything we need to urgently fix in a release version of git.\n>>\n>> One data point there is that the fallback blk-sha1 implementation\n>> we've shipped since 2009 has the same errors about unaligned access as\n>> before your patch with -fsanitize[..], and looking at the commit\n>> message this was something Linus knew about at the time, see\n>> d7c208a92e (\"Add new optimized C 'block-sha1' routines\", 2009-08-05).\n>>\n>> That's strong empirical data suggesting that whatever we want to do\n>> about unaligned access in the future it's not something which in\n>> practice would cause wrong sha1 results for the implementation\n>> shipping with v2.13.0.\n>>\n>> As an aside, when I was trying to apply your patch I made a mistake,\n>> and found that git's test suite could run 100% OK with a bad sha1\n>> implementation, I didn't apply this part (word diff):\n>>\n>> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n>> index 04b104fe58..d6f4c442b5 100644\n>> --- a/sha1dc/sha1.c\n>> +++ b/sha1dc/sha1.c\n>> @@ -312 +312 @@ static void sha1_compression_W(uint32_t ihv[5], const\n>> uint32_t W[80])\n>> void sha1_compression_states(uint32_t ihv[5], const\n>> [-uint32_t-]{+uint8_t+} m[64], uint32_t W[80], uint32_t states[80][5])\n>> @@ -1642 +1642 @@ static void sha1_recompression_step(uint32_t step,\n>> uint32_t ihvin[5], uint32_t i\n>> static void sha1_process(SHA1_CTX* ctx, const [-uint32_t-]{+uint8_t+} block[64])\n>> diff --git a/sha1dc/sha1.h b/sha1dc/sha1.h\n>> index 1d1d2b8d7c..1d181a1403 100644\n>> --- a/sha1dc/sha1.h\n>> +++ b/sha1dc/sha1.h\n>> @@ -21 +21 @@ extern \"C\" {\n>> void sha1_compression_states(uint32_t[5], const\n>> [-uint32_t-]{+uint8_t+}[64], uint32_t[80], uint32_t[80][5]);\n>\n>\n> That change doesnt look right anyway.\n>\n> The original code asserts that it will receive a pointer to 64\n> uint32_t's as the second argument.\n>\n> The changed code asserts that it will receive a pointer to 64\n> uint8_t's as the second argument.\n>\n> If you change the type you will need to change the *number* correspondingly.\n>\n> I would expect to see the uint32_t[64] to turn into uint8_t[256]\n\nIndeed, the full change is one where \"uint32_t m[16]\" is changed to\n\"uint8_t m[64]\". See Martin's patch upthread.\n\nThe word-diff I posted is in the context of my misapplying that patch\n(since GMail destroyed it I manually typed in the change). That\nproduced \"uint32_t m[16]\" -> \"uint32_t m[64]\" instead of Martin's\nversion.\n\nThat's obviously incorrect and results in a broken SHA-1\nimplementation, but git's test suite isn't exhaustive enough that\nnothing in t/ caught it, only the t/perf rebase performance test.\n\n> I have noticed that gcc does not necessarily enforce these types of\n> declarations and I believe that the change might not have any affect\n> at all depending on how the pointers are being used. (C passes\n> pointers on the stack, so afaik these things are more hints than\n> anything else.)\n>\n> Looking at the code it looks like it conflates endianness with\n> alignedness when they arent the same thing, except that the majority\n> of little-endian boxes are x86 which do not require aligned access,\n> and many big-endian boxes do require aligned access. IOW, even they\n> are often related they are distinct properties.\n>\n> Most hash function implementations have code like the following\n> (extracted and reduced from hv_macro.h in perl.git [which only\n> supports little-endian hash functions]):\n>\n> #ifndef U32_ALIGNMENT_REQUIRED\n> #if (BYTEORDER == 0x1234 || BYTEORDER == 0x12345678)\n> #define U8TO32_LE(ptr) (*((const U32*)(ptr)))\n> #elif (BYTEORDER == 0x4321 || BYTEORDER == 0x87654321)\n> #if defined(__GNUC__) && (__GNUC__>4 || (__GNUC__==4 && __GNUC_MINOR__>=3))\n> #define U8TO32_LE(ptr) (__builtin_bswap32(*((U32*)(ptr))))\n> #endif\n> #endif\n> #endif\n>\n> #ifndef U8TO16_LE\n> /* Without a known fast bswap32 we're just as well off doing this */\n> #define U8TO32_LE(ptr)\n> ((U32)(ptr)[0]|(U32)(ptr)[1]<<8|(U32)(ptr)[2]<<16|(U32)(ptr)[3]<<24)\n> #endif\n>\n> Of course, in the case of SHA1 it is defined as being big-endian\n> (making it a relatively poor choice for use on x86), so you would need\n> to reverse a bit of this logic.\n>\n> Note the form shown in that last block will work regardless of\n> endianness or alignedness requirements and should be optimised\n> properly by most compilers into the most efficient operation.\n>\n> In other words, if you guys just switch to that kind of pattern this\n> problem will go away everywhere, and the only issue you will have to\n> consider is if it makes some oddball platforms too slow.\n\nI think this is something Marc Stevens & co would be very interested\nto hear about at\nhttps://github.com/cr-marcstevens/sha1collisiondetection\n\nHe's CC'd here but I suspect he's not keeping up with this deluge of\nE-Mails from the git project very closely, we're just using his\nsoftware as-is.\n"},{"id":"321445","messageId":"CA+55aFwL9LQfx8t7tixYgV+2w_=_dAABxk54_GLJoGod-w=mbw@mail.gmail.com","threadId":"46087","inReplyTo":"CANgJU+UzoaN3Urj=L4unMMtNwFm6G8LGxx19g49AR5R+76F2OA@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-06-02T21:53:59Z","receivedAt":"2017-06-02T21:54:05Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Jun 2, 2017 at 1:17 PM, demerphq wrote:\n> Most hash function implementations have code like the following\n> (extracted and reduced from hv_macro.h in perl.git [which only\n> supports little-endian hash functions]):\n\nYes.\n\nPlease do *not* try to make things overly portable by adding random\nmemcpy() functions.\n\nYes, many compilers will do ok with it, and generate the right code\n(single load from an unaligned address). But many won't. Gcc\ncompletely screws it up in older versions on ARM, for example.\n\nDereferencing an unaligned pointer may be \"undefined\" in some\ntechnical meaning, but it sure as hell isn't undefined in reality, and\ncompilers that willfully do stupid things should not be catered to\noverly. Reality is a lot more important.\n\nAnd I think gcc can be tweaked to use \"movbe\" on x86 with the right\nmagic incantation (ie not just __builtin_bswap32(), but also the\nswitch to enable the new instructions). So having code to detect gcc\nand using __builtin_bswap32() would indeed be a good thing.\n\n Linus\n"},{"id":"321464","messageId":"xmqq4lvyx71c.fsf@gitster.mtv.corp.google.com","threadId":"46087","inReplyTo":"CA+55aFwL9LQfx8t7tixYgV+2w_=_dAABxk54_GLJoGod-w=mbw@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-03T00:13:19Z","receivedAt":"2017-06-03T00:13:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Dereferencing an unaligned pointer may be \"undefined\" in some\n> technical meaning, but it sure as hell isn't undefined in reality, and\n> compilers that willfully do stupid things should not be catered to\n> overly. Reality is a lot more important.\n\nThanks for succinctly putting it this way. I think you said the\nabove number of times, and I was looking for one of them to include\na reference to in the response I was preparing, but this message\nmade it unnecessary ;-)\n\n> And I think gcc can be tweaked to use \"movbe\" on x86 with the right\n> magic incantation (ie not just __builtin_bswap32(), but also the\n> switch to enable the new instructions). So having code to detect gcc\n> and using __builtin_bswap32() would indeed be a good thing.\n"},{"id":"321465","messageId":"xmqqzidqvsdr.fsf@gitster.mtv.corp.google.com","threadId":"46087","inReplyTo":"CACBZZX5iSxKz9p1V5h=t0+QtrY75g6haqRqMu7GEfrJHpWkefA@mail.gmail.com","subject":"Re: Unaligned accesses in sha1dc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-03T00:15:12Z","receivedAt":"2017-06-03T00:15:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Fri, Jun 2, 2017 at 4:46 PM, Liam R. Howlett wrote:\n>\n>> 2.13.0 is very much broken for me on SPARC.\n>> {maint//git} $ make -j120\n>> [...]\n>> {maint//git} $ ./git log\n>> [1] 1004506 bus error (core dumped) ./git log\n>>\n>> This is with b06d36431 (maint).\n>>\n>> The same thing happens on v2.13.0-384-g826c06412 (master).\n>>\n>> v2.13.0-539-g4b9c06c7d (next) works for me, as did following the\n>> instructions on upgrading the sha1dc code myself.\n>\n> Thanks a lot. So that works as I suspect on SPARC, hopefully it'll be\n> in master (and 2.13.1) soon.\n\nThanks. Hopefully your ab/sha1dc-maint a0103914 (\"sha1dc: update\nfrom upstream\", 2017-05-20) should be sufficient for helping the\nusers in the real-world, then.\n\n\n"}]}