Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tolerate when endian.h is symlink to sys/endian.h #4226

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

catap
Copy link
Contributor

@catap catap commented Nov 7, 2024

Since 5a60b36 varnish drop support of sys/endian.h and they had a bit different API one or two leading _.

Some system may have symlink from endian.h to sys/endian.h, for example OpenBSD. On such system build fails due to wrong SHA256 hash.

Since 5a60b36 varnish drop support of
sys/endian.h and they had a bit different API one or two leading `_`.

Some system may have symlink from endian.h to sys/endian.h, for example
OpenBSD. On such system build fails due to wrong SHA256 hash.
@@ -33,8 +33,16 @@

#ifndef __DARWIN_BYTE_ORDER
# include <endian.h>
# define VBYTE_ORDER __BYTE_ORDER
# define VBIG_ENDIAN __BIG_ENDIAN
# ifdef _BYTE_ORDER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a particular reason we should check for _BYTE_ORDER rather than __BYTE_ORDER to be define?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because on some systems migth be no __BYTE_ORDER and only _BYTE_ORDER.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's probably not important, but what I meant is: wouldn't we achieve the same with

# ifdef __BYTE_ORDER
#  define VBYTE_ORDER	__BYTE_ORDER
# else
#  define VBYTE_ORDER	_BYTE_ORDER
# endif

I am asking because we preferred __BYTE_ORDER before, so if you suggest to prefer _BYTE_ORDER now, I wanted to know if you had a particular reason?

@nigoroll
Copy link
Member

nigoroll commented Nov 7, 2024

I do not understand why the build uses the wrong byte order rather than failing at compile time because of undefined symbols?

@catap
Copy link
Contributor Author

catap commented Nov 7, 2024

@nigoroll this isn't symbols. This is macroses which aren't defined => no compilling time errors.

When I run grep against /usr/include on OpenBSD 7.6:

~ $ grep -irn __BYTE_ORDER /usr/include 
/usr/include/c++/v1/__config:294:#  ifdef __BYTE_ORDER__
/usr/include/c++/v1/__config:295:#    if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
/usr/include/c++/v1/__config:297:#    elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
/usr/include/c++/v1/__config:299:#    endif // __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
/usr/include/c++/v1/__config:300:#  endif   // __BYTE_ORDER__
/usr/include/c++/v1/__config:407:#    if __BYTE_ORDER == __LITTLE_ENDIAN
/usr/include/c++/v1/__config:409:#    elif __BYTE_ORDER == __BIG_ENDIAN
/usr/include/c++/v1/__config:411:#    else // __BYTE_ORDER == __BIG_ENDIAN
/usr/include/llvm/Demangle/ItaniumDemangle.h:2345:#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
~ $ 

I can't find __BYTE_ORDER macros. And as part of 5a60b36 you had removed support of _BYTE_ORDER and expects __BYTE_ORDER always.

So, on OpenBSD the condition

#if defined(VBYTE_ORDER) && VBYTE_ORDER == VBIG_ENDIAN

holds on x86_64. Here a demo:

tmp $ cat test.c  
#include <stdio.h>

#ifndef __DARWIN_BYTE_ORDER
#  include <endian.h>
#  define VBYTE_ORDER   __BYTE_ORDER
#  define VBIG_ENDIAN   __BIG_ENDIAN
#else
#  define VBYTE_ORDER   __DARWIN_BYTE_ORDER
#  define VBIG_ENDIAN   __DARWIN_BIG_ENDIAN
#endif

int main(int argc, char* argv[]) {
#if defined(VBYTE_ORDER) && VBYTE_ORDER == VBIG_ENDIAN
    printf("Hold\n");
#else
    printf("Doesn't hold\n");
#endif
    return 0;
}
tmp $ cc test.c   
tmp $ ./a.out     
Hold
tmp $ 

As result it computes SHA with assumption that x86_64 is big-endian machine, which leads to incorrect hash and build fails on AZ at VSHA256_Test:

	for (p = sha256test; p->input != NULL; p++) {
		VSHA256_Init(&c);
		VSHA256_Update(&c, p->input, strlen(p->input));
		VSHA256_Final(o, &c);
		AZ(memcmp(o, p->output, 32));
	}

Before I figured it out I've spent a few hours in attempt to understand why it producing wrong result.

@nigoroll
Copy link
Member

nigoroll commented Nov 7, 2024

OK, so this went unnoticed because we even accept VBYTE_ORDER to be undefined.

Had we used

#if VBYTE_ORDER == VBIG_ENDIAN

we would have hit the undefined macro (thank you, not symbol).

@catap
Copy link
Contributor Author

catap commented Nov 7, 2024

are you sure?

tmp $ rm a.out                                                                                                                            
tmp $ cat test.c  
#include <stdio.h>

#ifndef __DARWIN_BYTE_ORDER
#  include <endian.h>
#  define VBYTE_ORDER   __BYTE_ORDER
#  define VBIG_ENDIAN   __BIG_ENDIAN
#else
#  define VBYTE_ORDER   __DARWIN_BYTE_ORDER
#  define VBIG_ENDIAN   __DARWIN_BIG_ENDIAN
#endif

int main(int argc, char* argv[]) {
#if VBYTE_ORDER == VBIG_ENDIAN
    printf("Hold\n");
#else
    printf("Doesn't hold\n");
#endif
    return 0;
}
tmp $ cc test.c   
tmp $ ./a.out     
Hold
tmp $ 

@catap
Copy link
Contributor Author

catap commented Nov 7, 2024

See:

tmp $ cat test.c  
#include <stdio.h>

int main(int argc, char* argv[]) {
#if ABCDEFG == GFEDCBA
    printf("Hold\n");
#else
    printf("Doesn't hold\n");
#endif
    return 0;
}
tmp $ cc test.c   
tmp $ ./a.out     
Hold
tmp $ cc --version
OpenBSD clang version 16.0.6
Target: amd64-unknown-openbsd7.6
Thread model: posix
InstalledDir: /usr/bin
tmp $ 

@nigoroll
Copy link
Member

nigoroll commented Nov 7, 2024

I need to have another look on a new day, something is confusing me.

@catap catap mentioned this pull request Nov 7, 2024
15 tasks
nigoroll added a commit to nigoroll/varnish-cache that referenced this pull request Nov 18, 2024
to avoid errors and confusion as documented in varnishcache#4226
@nigoroll
Copy link
Member

Sorry for my confusion and see #4229 for an additional change which, hopefully, should avoid this in the future.

@nigoroll nigoroll merged commit d05d20e into varnishcache:master Nov 18, 2024
10 of 11 checks passed
@catap catap deleted the openbsd branch November 18, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants