Re: [PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2017, 06:03 UTC
Message-ID: <xmqqr31l6ggf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <856355e49e7a6016f5d1ef5f9785d94a455b5755.1490395862.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 15 quoted lines

> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> index 6dd0da36084..d99db4f2e1b 100644
> --- a/sha1dc/sha1.c
> +++ b/sha1dc/sha1.c
> @@ -35,7 +35,7 @@
>  
>  #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(BIGENDIAN) && BIGENDIAN != 0
>  	#define sha1_load(m, t, temp)  { temp = m[t]; }
>  #else
>  	#define sha1_load(m, t, temp)  { temp = m[t]; sha1_bswap32(temp); }
>
> base-commit: c21884356fab0bc6bc5fa6abcadbda27a112a76c

Is this change _only_ for Windows where BIGENDIAN happens to be defined to be 0?

If we only assume that BIGENDIAN can be either (0) Not set at all by platform headers, (1) Not set or set to "0" on little-endian systems, or (2) Set to any value other than "0" on big-endian systems, then the patch looks OK.

But it feels a bit too brittle [*1*].

Looking at sha1dc/sha1.c, I notice that it begins like so:

    /*
       Because Little-Endian architectures are most common,
       we only set 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, you will have to add
       whatever macros your tool chain defines to indicate
       Big-Endianness.
     */
    #if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
        (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) || \
        defined(__BIG_ENDIAN__) || defined(__ARMEB__) || defined(__THUMBEB__) ||  defined(__AARCH64EB__) || \
        defined(_MIPSEB) || defined(__MIPSEB) || defined(__MIPSEB__)

    #define BIGENDIAN	(1)

    #endif /*ENDIANNESS SELECTION*/

Notice:

 - It does not consider BIGENDIAN can be set from the outside (i.e.
   it does not say "#ifndef BIGENDIAN" around the above auto
   selection).

 - It considers the actual value of BIGENDIAN given from the outside
   is expendable (i.e. if somehow the caller sets it to 2, it is
   reset to 1 if __BYTE_ORDER and friends tells us we are on a
   big-endian box.

 - It really wants to tell little endian machines not to set
   BIGENDIAN (i.e. lack of "#else" that does "#undef BIGENDIAN").

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.

Anyway, thanks for noticing and raising the issue.

[Footnote]

*1* The "brittle" comment is because the assumption feels a bit too
    narrow, and if we want to solve this without assuming too much
    (i.e. stop thinking about "I just want to fix Windows; I do not
    care about a more general and robust solution"), I think we
    could solve it for something that completely violates your
    assumption.  For example, a platform where something like this
    is used in the header to switch behaviour depending on the
    endianness:

	/* Symbolic constants */
	#define BIGENDIAN 0
	#define LITTLEENDIAN 1
	...

	/* Say which one MY build is */
	#define MYENDIAN BIGENDIAN
	...

	#if MYENDIAN == LITTLEENDIAN
	... code for little endian systems ...
	#else /* MYENDIAN == BIGENDIAN */
	... code for big endian systems ...
	#endif

    would break your patch.  Such a big-endian box will end up using
    the wrong definition of sha1_load().  The original happens to
    choose the right definition by accident, as the header's design
    is that MYENDIAN describes the box you are on, though.

    But as my analysis of how BIGENDIAN is set in the sha1.c file
    itself shows, I think it would be a more robust solution if we
    made sure that we do not pay attention to BIGENDIAN coming from
    the outside world at all, i.e. "#undef" upfront.

    Another possibility would be to rename BIGENDIAN used in
    sha1dc/sha1.c file to something a lot less generic,
    e.g. SHA1DC_BIGENDIAN.