From: Junio C Hamano <gitster@pobox•com>
To: Johannes Schindelin <johannes.schindelin@gmx•de>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN
Date: Sat, 25 Mar 2017 10:22:26 -0700 [thread overview]
Message-ID: <xmqqwpbd46gd.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <xmqqr31l6ggf.fsf@gitster.mtv.corp.google.com> (Junio C. Hamano's message of "Fri, 24 Mar 2017 23:03:28 -0700")
Junio C Hamano <gitster@pobox•com> writes:
> Which leads me to wonder if a more robust solution that is in line
> with the original design of sha1dc/sha1.c code may be to do an
> unconditional "#undef BIGENDIAN" before the above block, so that no
> matter what the calling environment sets BIGENDIAN to (including
> "0"), it gets ignored and we always use the auto-selection.
So here is what I came up with as a replacement (this time as a
proper patch not a comment on a patch).
Dscho, could you see if this fixes your build?
Also, could people on big-endian boxes see if this breaks your
setting (relative to the state before this patch)?
Thanks.
-- >8 --
From: Junio C Hamano <gitster@pobox•com>
Date: Sat, 25 Mar 2017 10:05:13 -0700
Subject: [PATCH] sha1dc: avoid CPP macro collisions
In an early part of sha1dc/sha1.c, the code checks the endianness of
the target platform by inspecting common CPP macros defined on
big-endian boxes, and sets BIGENDIAN macro to 1. If these common
CPP macros are not defined, the code declares that the target
platform is little endian and does nothing (most notably, it does
not #undef its BIGENDIAN macro).
The code that does so even has this comment
Note that all MSFT platforms are little endian,
so none of these will be defined under the MSC compiler.
and later, the defined-ness of the BIGENDIAN macro is used to switch
the implementation of sha1_load() macro.
One thing the code did not anticipate is that somebody might define
BIGENDIAN macro in some header it includes to 0 on a little-endian
target platform. Because the auto-detection based on common macros
do not touch BIGENDIAN macro when it detects a little-endian target,
such a definition is still valid and then defined-ness test will say
"Ah, BIGENDIAN is defined" and takes the wrong sha1_load().
As this auto-detection logic pretends as if it owns the BIGENDIAN
macro by ignoring the setting that may come from the outside and by
not explicitly unsetting when it decides that it is working for a
little-endian target, solve this problem without breaking that
assumption. Namely, we can rename BIGENDIAN this code uses to
something much less generic, i.e. SHA1DC_BIGENDIAN. For extra
protection, undef the macro on a little-endian target.
It is possible to work it around by instead #undef BIGENDIAN in
the auto-detection code, but a macro (or include) that happens later
in the code can be implemented in terms of BIGENDIAN on Windows and
it is possible that the implementation gets upset when it sees the
CPP macro undef'ed (instead of set to 0). Renaming the private macro
intended to be used only in this file to a less generic name relieves
us from having to worry about that kind of breakage.
Noticed-by: Johannes Schindelin <johannes.schindelin@gmx•de>
Signed-off-by: Junio C Hamano <gitster@pobox•com>
---
sha1dc/sha1.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 6dd0da3608..35e9dd5bf4 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -12,7 +12,7 @@
/*
Because Little-Endian architectures are most common,
- we only set BIGENDIAN if one of these conditions is met.
+ we only set SHA1DC_BIGENDIAN if one of these conditions is met.
Note that all MSFT platforms are little endian,
so none of these will be defined under the MSC compiler.
If you are compiling on a big endian platform and your compiler does not define one of these,
@@ -23,8 +23,9 @@
defined(__BIG_ENDIAN__) || defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
defined(_MIPSEB) || defined(__MIPSEB) || defined(__MIPSEB__)
-#define BIGENDIAN (1)
-
+#define SHA1DC_BIGENDIAN 1
+#else
+#undef SHA1DC_BIGENDIAN
#endif /*ENDIANNESS SELECTION*/
#define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n))))
@@ -35,11 +36,11 @@
#define sha1_mix(W, t) (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 16], 1))
-#if defined(BIGENDIAN)
+#if defined(SHA1DC_BIGENDIAN)
#define sha1_load(m, t, temp) { temp = m[t]; }
#else
#define sha1_load(m, t, temp) { temp = m[t]; sha1_bswap32(temp); }
-#endif /*define(BIGENDIAN)*/
+#endif /* !defined(SHA1DC_BIGENDIAN) */
#define sha1_store(W, t, x) *(volatile uint32_t *)&W[t] = x
--
2.12.2-399-g034667a458
next prev parent reply other threads:[~2017-03-25 17:28 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-24 22:52 [PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN Johannes Schindelin
2017-03-25 6:03 ` Junio C Hamano
2017-03-25 17:22 ` Junio C Hamano [this message]
2017-03-27 15:39 ` Johannes Schindelin
2017-03-27 17:05 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqwpbd46gd.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=johannes.schindelin@gmx$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox