{"thread":{"id":"45505","subject":"[PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN","startedAt":"2017-03-24T22:52:31Z","lastAt":"2017-03-27T17:06:39Z","messageCount":5,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315343","messageId":"856355e49e7a6016f5d1ef5f9785d94a455b5755.1490395862.git.johannes.schindelin@gmx.de","threadId":"45505","inReplyTo":null,"subject":"[PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-24T22:52:10Z","receivedAt":"2017-03-24T22:52:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In sha1dc/sha1.c, we #define BIGENDIAN under certain circumstances, and\nobviously leave the door open for scenarios where our conditions do not\ncatch and that constant is #defined elsewhere.\n\nHowever, we did not expect that anybody would possibly #define BIGENDIAN\nto 0, indicating that the current platform is *not* big endian.\n\nThis is not just a theoretical consideration: On Windows, the winsock2.h\nheader file (which is used to allow Git to communicate via network) does\nindeed do this.\n\nLet's test for that circumstance, too, and byte-swap as intended in that\ncase.\n\nThis fixes a massive breakage on Windows where current `pu` (having\nswitched on DC_SHA1 by default) breaks pretty much every single test\ncase.\n\nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/sha1dc-bigendian=0-v1\nFetch-It-Via: git fetch https://github.com/dscho/git sha1dc-bigendian=0-v1\n\n\tObviously, this patch is based on `next`.\n\n sha1dc/sha1.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex 6dd0da36084..d99db4f2e1b 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -35,7 +35,7 @@\n \n #define sha1_mix(W, t) (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 16], 1))\n \n-#if defined(BIGENDIAN)\n+#if defined(BIGENDIAN) && BIGENDIAN != 0\n \t#define sha1_load(m, t, temp) { temp = m[t]; }\n #else\n \t#define sha1_load(m, t, temp) { temp = m[t]; sha1_bswap32(temp); }\n\nbase-commit: c21884356fab0bc6bc5fa6abcadbda27a112a76c\n-- \n2.12.1.windows.1\n"},{"id":"315375","messageId":"xmqqr31l6ggf.fsf@gitster.mtv.corp.google.com","threadId":"45505","inReplyTo":"856355e49e7a6016f5d1ef5f9785d94a455b5755.1490395862.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-25T06:03:28Z","receivedAt":"2017-03-25T06:03:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n> index 6dd0da36084..d99db4f2e1b 100644\n> --- a/sha1dc/sha1.c\n> +++ b/sha1dc/sha1.c\n> @@ -35,7 +35,7 @@\n> \n> #define sha1_mix(W, t) (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 16], 1))\n> \n> -#if defined(BIGENDIAN)\n> +#if defined(BIGENDIAN) && BIGENDIAN != 0\n> \t#define sha1_load(m, t, temp) { temp = m[t]; }\n> #else\n> \t#define sha1_load(m, t, temp) { temp = m[t]; sha1_bswap32(temp); }\n>\n> base-commit: c21884356fab0bc6bc5fa6abcadbda27a112a76c\n\nIs this change _only_ for Windows where BIGENDIAN happens to be\ndefined to be 0?\n\nIf we only assume that BIGENDIAN can be either (0) Not set at all by\nplatform headers, (1) Not set or set to \"0\" on little-endian\nsystems, or (2) Set to any value other than \"0\" on big-endian\nsystems, then the patch looks OK.\n\nBut it feels a bit too brittle [*1*].\n\nLooking at sha1dc/sha1.c, I notice that it begins like so:\n\n /*\n Because Little-Endian architectures are most common,\n we only set BIGENDIAN if one of these conditions is met.\n Note that all MSFT platforms are little endian,\n so none of these will be defined under the MSC compiler.\n If you are compiling on a big endian platform and your\n compiler does not define one of these, you will have to add\n whatever macros your tool chain defines to indicate\n Big-Endianness.\n */\n #if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \\\n (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) || \\\n defined(__BIG_ENDIAN__) || defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \\\n defined(_MIPSEB) || defined(__MIPSEB) || defined(__MIPSEB__)\n\n #define BIGENDIAN\t(1)\n\n #endif /*ENDIANNESS SELECTION*/\n\nNotice:\n\n - It does not consider BIGENDIAN can be set from the outside (i.e.\n it does not say \"#ifndef BIGENDIAN\" around the above auto\n selection).\n\n - It considers the actual value of BIGENDIAN given from the outside\n is expendable (i.e. if somehow the caller sets it to 2, it is\n reset to 1 if __BYTE_ORDER and friends tells us we are on a\n big-endian box.\n\n - It really wants to tell little endian machines not to set\n BIGENDIAN (i.e. lack of \"#else\" that does \"#undef BIGENDIAN\").\n\nWhich leads me to wonder if a more robust solution that is in line\nwith the original design of sha1dc/sha1.c code may be to do an\nunconditional \"#undef BIGENDIAN\" before the above block, so that no\nmatter what the calling environment sets BIGENDIAN to (including\n\"0\"), it gets ignored and we always use the auto-selection.\n\nAnyway, thanks for noticing and raising the issue.\n\n\n[Footnote]\n\n*1* The \"brittle\" comment is because the assumption feels a bit too\n narrow, and if we want to solve this without assuming too much\n (i.e. stop thinking about \"I just want to fix Windows; I do not\n care about a more general and robust solution\"), I think we\n could solve it for something that completely violates your\n assumption. For example, a platform where something like this\n is used in the header to switch behaviour depending on the\n endianness:\n\n\t/* Symbolic constants */\n\t#define BIGENDIAN 0\n\t#define LITTLEENDIAN 1\n\t...\n\n\t/* Say which one MY build is */\n\t#define MYENDIAN BIGENDIAN\n\t...\n\n\t#if MYENDIAN == LITTLEENDIAN\n\t... code for little endian systems ...\n\t#else /* MYENDIAN == BIGENDIAN */\n\t... code for big endian systems ...\n\t#endif\n\n would break your patch. Such a big-endian box will end up using\n the wrong definition of sha1_load(). The original happens to\n choose the right definition by accident, as the header's design\n is that MYENDIAN describes the box you are on, though.\n\n But as my analysis of how BIGENDIAN is set in the sha1.c file\n itself shows, I think it would be a more robust solution if we\n made sure that we do not pay attention to BIGENDIAN coming from\n the outside world at all, i.e. \"#undef\" upfront.\n\n Another possibility would be to rename BIGENDIAN used in\n sha1dc/sha1.c file to something a lot less generic,\n e.g. SHA1DC_BIGENDIAN.\n\n\n"},{"id":"315399","messageId":"xmqqwpbd46gd.fsf@gitster.mtv.corp.google.com","threadId":"45505","inReplyTo":"xmqqr31l6ggf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-25T17:22:26Z","receivedAt":"2017-03-25T17:28:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Which leads me to wonder if a more robust solution that is in line\n> with the original design of sha1dc/sha1.c code may be to do an\n> unconditional \"#undef BIGENDIAN\" before the above block, so that no\n> matter what the calling environment sets BIGENDIAN to (including\n> \"0\"), it gets ignored and we always use the auto-selection.\n\nSo here is what I came up with as a replacement (this time as a\nproper patch not a comment on a patch).\n\nDscho, could you see if this fixes your build?\n\nAlso, could people on big-endian boxes see if this breaks your\nsetting (relative to the state before this patch)?\n\nThanks.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Sat, 25 Mar 2017 10:05:13 -0700\nSubject: [PATCH] sha1dc: avoid CPP macro collisions\n\nIn an early part of sha1dc/sha1.c, the code checks the endianness of\nthe target platform by inspecting common CPP macros defined on\nbig-endian boxes, and sets BIGENDIAN macro to 1. If these common\nCPP macros are not defined, the code declares that the target\nplatform is little endian and does nothing (most notably, it does\nnot #undef its BIGENDIAN macro).\n\nThe code that does so even has this comment\n\n Note that all MSFT platforms are little endian,\n so none of these will be defined under the MSC compiler.\n\nand later, the defined-ness of the BIGENDIAN macro is used to switch\nthe implementation of sha1_load() macro.\n\nOne thing the code did not anticipate is that somebody might define\nBIGENDIAN macro in some header it includes to 0 on a little-endian\ntarget platform. Because the auto-detection based on common macros\ndo not touch BIGENDIAN macro when it detects a little-endian target,\nsuch a definition is still valid and then defined-ness test will say\n\"Ah, BIGENDIAN is defined\" and takes the wrong sha1_load().\n\nAs this auto-detection logic pretends as if it owns the BIGENDIAN\nmacro by ignoring the setting that may come from the outside and by\nnot explicitly unsetting when it decides that it is working for a\nlittle-endian target, solve this problem without breaking that\nassumption. Namely, we can rename BIGENDIAN this code uses to\nsomething much less generic, i.e. SHA1DC_BIGENDIAN. For extra\nprotection, undef the macro on a little-endian target.\n\nIt is possible to work it around by instead #undef BIGENDIAN in\nthe auto-detection code, but a macro (or include) that happens later\nin the code can be implemented in terms of BIGENDIAN on Windows and\nit is possible that the implementation gets upset when it sees the\nCPP macro undef'ed (instead of set to 0). Renaming the private macro\nintended to be used only in this file to a less generic name relieves\nus from having to worry about that kind of breakage.\n\nNoticed-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n sha1dc/sha1.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex 6dd0da3608..35e9dd5bf4 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -12,7 +12,7 @@\n \n /*\n Because Little-Endian architectures are most common,\n- we only set BIGENDIAN if one of these conditions is met.\n+ we only set SHA1DC_BIGENDIAN if one of these conditions is met.\n Note that all MSFT platforms are little endian,\n so none of these will be defined under the MSC compiler.\n If you are compiling on a big endian platform and your compiler does not define one of these,\n@@ -23,8 +23,9 @@\n defined(__BIG_ENDIAN__) || defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \\\n defined(_MIPSEB) || defined(__MIPSEB) || defined(__MIPSEB__)\n \n-#define BIGENDIAN\t(1)\n-\n+#define SHA1DC_BIGENDIAN\t1\n+#else\n+#undef SHA1DC_BIGENDIAN\n #endif /*ENDIANNESS SELECTION*/\n \n #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n))))\n@@ -35,11 +36,11 @@\n \n #define sha1_mix(W, t) (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 16], 1))\n \n-#if defined(BIGENDIAN)\n+#if defined(SHA1DC_BIGENDIAN)\n \t#define sha1_load(m, t, temp) { temp = m[t]; }\n #else\n \t#define sha1_load(m, t, temp) { temp = m[t]; sha1_bswap32(temp); }\n-#endif /*define(BIGENDIAN)*/\n+#endif /* !defined(SHA1DC_BIGENDIAN) */\n \n #define sha1_store(W, t, x)\t*(volatile uint32_t *)&W[t] = x\n \n-- \n2.12.2-399-g034667a458\n\n"},{"id":"315513","messageId":"alpine.DEB.2.20.1703271739210.14958@virtualbox","threadId":"45505","inReplyTo":"xmqqwpbd46gd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-27T15:39:43Z","receivedAt":"2017-03-27T15:40:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sat, 25 Mar 2017, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Which leads me to wonder if a more robust solution that is in line\n> > with the original design of sha1dc/sha1.c code may be to do an\n> > unconditional \"#undef BIGENDIAN\" before the above block, so that no\n> > matter what the calling environment sets BIGENDIAN to (including\n> > \"0\"), it gets ignored and we always use the auto-selection.\n> \n> So here is what I came up with as a replacement (this time as a\n> proper patch not a comment on a patch).\n> \n> Dscho, could you see if this fixes your build?\n\nThe Continuous Testing is back to normal, thanks.\n\nCiao,\nJohannes\n"},{"id":"315519","messageId":"xmqq37dy3b0y.fsf@gitster.mtv.corp.google.com","threadId":"45505","inReplyTo":"alpine.DEB.2.20.1703271739210.14958@virtualbox","subject":"Re: [PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-27T17:05:49Z","receivedAt":"2017-03-27T17:06:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> So here is what I came up with as a replacement (this time as a\n>> proper patch not a comment on a patch).\n>> \n>> Dscho, could you see if this fixes your build?\n>\n> The Continuous Testing is back to normal, thanks.\n\nThanks for a quick response.\n\nI wished if we can reuse the endianness detection we already have in\n\"compat/bswap.h\", which is included by git-compat-util.h hence also\nhere. It would have been very nice if we could say\n\n #if GIT_BYTE_ORDER == GIT_BIG_ENDIAN\n ... do big endian thing ...\n #else\n ... do little endian thing ...\n #endif\n\nwhen necessary, without having to reinvent the cheks on __BYTE_ORDER\nand friends. It however needs a bit of rework in \"compat/bswap.h\",\nwhich is way beyond the scope of making a quick fix-up to unblock us.\n\n\n\n\n\n"}]}