gcc 3 warnings & errors

[solardiz]

Not sure if we care, but gcc 3.4.6 on x86_64 gives:

yescrypt/yescrypt-opt.c: In function `yescrypt_kdf_body':
yescrypt/yescrypt-opt.c:1192: warning: comparison is always false due to limited range of data type
sha2.c:706:7: warning: no newline at end of file
stribog_fmt_plug.c:472:2: warning: #warning Stribog-256 and Stribog-512 formats require SSE 4.1, formats disabled
memory.c: In function `alloc_region_t':
memory.c:625: warning: dereferencing type-punned pointer will break strict-aliasing rules
loader.c: In function `ldr_fill_user_words':
loader.c:1975: warning: comparison is always true due to limited range of data type
loader.c:1975: warning: comparison is always true due to limited range of data type
loader.c:2002: warning: comparison is always true due to limited range of data type
racf2john.c: In function `main':
racf2john.c:100: warning: 'h1_len' might be used uninitialized in this function
racf2john.c:102: warning: 'h2_len' might be used uninitialized in this function
ztex.c: In function `ztex_scan_new_devices':
ztex.c:444: warning: 'result_open' might be used uninitialized in this function
In file included from dmg2john.c:41:
filevault.h:5: warning: malformed '#pragma pack(push[, id], <n>)' - ignored
filevault.h:73: warning: #pragma pack (pop) encountered without matching #pragma pack (push, <n>)
In file included from wpapcap2john.c:37:
jumbo.h:38:1: warning: "__STDC_FORMAT_MACROS" redefined
wpapcap2john.c:29:1: warning: this is the location of the previous definition
ar: creating aes.a
memory.c: In function `alloc_region_t':
memory.c:625: warning: dereferencing type-punned pointer will break strict-aliasing rules
memory.c: In function `alloc_region_t':
memory.c:625: warning: dereferencing type-punned pointer will break strict-aliasing rules
memory.c: In function `alloc_region_t':
memory.c:625: warning: dereferencing type-punned pointer will break strict-aliasing rules

Yet the build succeeds.

gcc 3.3.6:

In file included from lzma/LzmaDec.h:7,
                 from 7z_common.h:13,
                 from 7z_common_plug.c:31:
lzma/7zTypes.h:66: warning: redefinition of `Byte'
/usr/include/zconf.h:261: warning: `Byte' previously declared here
In file included from lzma/LzmaDec.h:7,
                 from 7z_common.h:13,
                 from 7z_fmt_plug.c:73:
lzma/7zTypes.h:66: warning: redefinition of `Byte'
/usr/include/zconf.h:261: warning: `Byte' previously declared here
yescrypt/yescrypt-opt.c: In function `yescrypt_kdf_body':
yescrypt/yescrypt-opt.c:1192: warning: comparison is always false due to limited range of data type
In file included from argon2_core_plug.c:32:
blake2.h:13:9: warning: #pragma once is obsolete
In file included from argon2_core_plug.c:33:
blake2-impl.h:13:9: warning: #pragma once is obsolete
In file included from argon2_opt_plug.c:22:
blake2.h:13:9: warning: #pragma once is obsolete
In file included from blamka-round-opt.h:4,
                 from argon2_opt_plug.c:23:
blake2-impl.h:13:9: warning: #pragma once is obsolete
In file included from blake2b_plug.c:20:
blake2.h:13:9: warning: #pragma once is obsolete
In file included from chacha_plug.c:7:
chacha.h:16: warning: redefinition of `u_char'
/usr/include/sys/types.h:34: warning: `u_char' previously declared here
chacha.h:18: warning: redefinition of `u_int'
/usr/include/sys/types.h:36: warning: `u_int' previously declared here
In file included from blake2b_plug.c:21:
blake2-impl.h:13:9: warning: #pragma once is obsolete
In file included from blake2b_plug.c:37:
blake2b-round.h:13:9: warning: #pragma once is obsolete
In file included from blake2b-round.h:146,
                 from blake2b_plug.c:37:
blake2b-load-sse2.h:13:9: warning: #pragma once is obsolete
blake2b_plug.c: In function `blake2b_compress':
blake2b_plug.c:194: warning: dereferencing type-punned pointer will break strict-aliasing rules
sha2.c:706:7: warning: no newline at end of file
In file included from jh_plug.c:16:
jh.h:15:9: warning: #pragma once is obsolete
In file included from keepass_fmt_plug.c:44:
chacha.h:16: warning: redefinition of `u_char'
/usr/include/sys/types.h:34: warning: `u_char' previously declared here
chacha.h:18: warning: redefinition of `u_int'
/usr/include/sys/types.h:36: warning: `u_int' previously declared here
In file included from monero_fmt_plug.c:37:
chacha.h:16: warning: redefinition of `u_char'
/usr/include/sys/types.h:34: warning: `u_char' previously declared here
chacha.h:18: warning: redefinition of `u_int'
/usr/include/sys/types.h:36: warning: `u_int' previously declared here
In file included from rawBLAKE2_512_fmt_plug.c:22:
blake2.h:13:9: warning: #pragma once is obsolete
In file included from slow_hash_plug.c:11:
int-util.h:5:9: warning: #pragma once is obsolete
In file included from slow_hash_plug.c:15:
jh.h:15:9: warning: #pragma once is obsolete
stribog_fmt_plug.c:472:2: warning: #warning Stribog-256 and Stribog-512 formats require SSE 4.1, formats disabled
In file included from tezos_fmt_plug.c:36:
blake2.h:13:9: warning: #pragma once is obsolete
dynamic_fmt.c: In function `LoadOneFormat':
dynamic_fmt.c:7976: warning: unused variable `options'
In file included from loader.c:51:
mgetl.h: In function `mgetl':
mgetl.h:46: warning: implicit declaration of function `__builtin_ctzl'
In file included from wordlist.c:73:
mgetl.h: In function `mgetl':
mgetl.h:46: warning: implicit declaration of function `__builtin_ctzl'
memory.c: In function `alloc_region_t':
memory.c:625: warning: dereferencing type-punned pointer will break strict-aliasing rules
loader.c: In function `ldr_fill_user_words':
loader.c:1975: warning: comparison is always true due to limited range of data type
loader.c:1975: warning: comparison is always true due to limited range of data type
loader.c:2002: warning: comparison is always true due to limited range of data type
racf2john.c: In function `process_user_rec':
racf2john.c:100: warning: `h1_len' might be used uninitialized in this function
racf2john.c:102: warning: `h2_len' might be used uninitialized in this function
In file included from dmg2john.c:41:
filevault.h:5: warning: malformed '#pragma pack(push[, id], <n>)' - ignored
filevault.h:73: warning: #pragma pack (pop) encountered without matching #pragma pack (push, <n>)
In file included from wpapcap2john.c:37:
jumbo.h:38:1: warning: "__STDC_FORMAT_MACROS" redefined
wpapcap2john.c:29:1: warning: this is the location of the previous definition
ztex.c: In function `ztex_scan_new_devices':
ztex.c:444: warning: `result_open' might be used uninitialized in this function
ar: creating aes.a
memory.c: In function `alloc_region_t':
memory.c:625: warning: dereferencing type-punned pointer will break strict-aliasing rules
memory.c: In function `alloc_region_t':
memory.c:625: warning: dereferencing type-punned pointer will break strict-aliasing rules
memory.c: In function `alloc_region_t':
memory.c:625: warning: dereferencing type-punned pointer will break strict-aliasing rules
ar: creating secp256k1.a
ar: creating ed25519-donna.a
loader.o: In function `mgetl':
/home/solar/j/bleeding-jumbo-20200607/src/mgetl.h:46: undefined reference to `__builtin_ctzl'
wordlist.o: In function `mgetl':
/home/solar/j/bleeding-jumbo-20200607/src/mgetl.h:46: undefined reference to `__builtin_ctzl'

Build succeeds after this fix:

+++ b/src/mgetl.h
@@ -42,7 +42,7 @@ static MAYBE_INLINE char *mgetl(char *res)
 
                vstoreu((vtype*)pos, x);
                if (v) {
-#ifdef __GNUC__
+#if __GNUC__ >= 4 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4)
                        unsigned int r = __builtin_ctzl(v);
 #else
                        unsigned int r = ffs(v) - 1;

This is the exact same check that we already have in gost.h for the same reason, so I think we should commit it.

With both builds, all 408 CPU format tests pass.

[solardiz]

memory.c: In function `alloc_region_t':
memory.c:625: warning: dereferencing type-punned pointer will break strict-aliasing rules

This points out several things:

1. We have an older revision of code from yescrypt-platform.c duplicated in memory.c. We'll need to de-duplicate this eventually.

2. memory.c forgot to include sys/mman.h, which results in it using the mmap-unaware code there. This might have performance impact for Argon2.

3. Some compilers treat the pointer cast in the HAVE_POSIX_MEMALIGN code as violating C aliasing rules. I think it actually does not, because the cast is to an object of same size (also a pointer). However, that some compilers think that it does is yet another reason for me to drop this special case altogether, which I was already considering. (I could also introduce a temporary pointer to make those compilers happy, but as I was already considering dropping the code I probably should drop it now.)

4. We have autoconf logic enabling HAVE_POSIX_MEMALIGN. We should probably drop that as well. We have the fallback code anyway, which performance-wise is just as good, so there's little reason to inflate our source code size and reduce code coverage of our testing by having too many special cases.

[magnumripper]

This is definitely not the first time I've seen an ancient, and sane, gcc build give us much more reasonable and actually worhtful warnings than any ridiculous version since 2010 or so (which also emphasises my feeling of just being an old git). I'm pretty sure they have some Guiness Book of Records winning member (as in "human with most OCD in the entire world") trying to tie him/herself into knots with completely useless warnings but in fear of a useless war I have yet to try to express this to them.

BTW what does ffs() mean? Surely not what first comes to my mind 😆🤣

[solardiz]

Except for gcc 8's, I actually find recent gcc warnings helpful. The gcc 10 errors and warnings helped us fix some bugs in ZTEX support code and helped detect the meaningless misuse of strncpy, although I think someone else could have fixed that by making it even more complicated. The gcc 8 warnings were problematic in that they prompted ridiculous non-fixes for the uses of strncpy, but what gcc 10 added on top of that is reasonable and can actually detect some of those ridiculous non-fixes.

BTW what does ffs() mean?

Find First Set bit.

[solardiz]

What do we mean by the checks for < 128 on these lines in loader.c? -

                                if ((seeds_sorted == (MAYBE | SORTED | ASCII)) && *line < 128 && *reset_pos < 128 &&
                                    strncmp(line, reset_pos, strlen(line)) < 0) {
                                        fprintf(stderr, "User-seed file isn't sorted, slow load.\n");

#if HAVE_MMAP
                                        map_pos = (*user < 128) ? reset_pos : mem_map;
#else
                                        fseek(file, (*user < 128) ? reset_pos : 0, SEEK_SET);

These comparisons are currently always true on systems with signed char, which is probably not what we wanted? Do we add casts to (unsigned char) in these comparisons?

[solardiz]

Turns out there are more uses of HAVE_POSIX_MEMALIGN in jumbo, so I fixed and kept it for now.

I also made gcc 3.3.6 happy of BLAKE2, but we'll need to revisit (and maybe reconsider) this when merging new BLAKE2 upstream code.

Since we don't consistently rely on #pragma once anyway, I dropped/replaced the few uses of it that we inadvertently had.

For the struct packing in filevault.h, I used the same approach that we already had in diskcryptor_common.h. It is not ideal, but at least it's the same - so either both work or both fail.

For the checks against 128, I added casts to (unsigned char).

There are many other assorted changes, which I hope are self-explanatory.

[magnumripper]

What do we mean by the checks for < 128 on these lines in loader.c? -

                                if ((seeds_sorted == (MAYBE | SORTED | ASCII)) && *line < 128 && *reset_pos < 128 &&
                                    strncmp(line, reset_pos, strlen(line)) < 0) {
                                        fprintf(stderr, "User-seed file isn't sorted, slow load.\n");

#if HAVE_MMAP
                                        map_pos = (*user < 128) ? reset_pos : mem_map;
#else
                                        fseek(file, (*user < 128) ? reset_pos : 0, SEEK_SET);

These comparisons are currently always true on systems with signed char, which is probably not what we wanted? Do we add casts to (unsigned char) in these comparisons?

Ouch. I never saw ill behaviour caused by this, but there should of been some. Thanks for fixing.

[solardiz]

@magnumripper Why did this code try to handle 8-bit chars specially? Is that somehow needed for utf-8 or for legacy 8-bit charsets, or both?

[magnumripper]

If a user "sorted" the file (which speeds things up significantly), it might be sorted with C or with en_US.UTF-8 or any other strange encoding or codepage, and this code needs its view of that to be the same, or unreliable. So this is just a really trivial sanity check which falls back to being slow but (I think) correct.

[magnumripper]

Anyway it's all post-release IIRC

[solardiz]

@magnumripper I think this needs a source code comment.

Also, I think sort order can vary between locales even for character codes below 128. This isn't a reason against the sanity check, but we should realize (and document in that comment?) that the check is non-perfect.

[magnumripper]

True, while fairly unlikely these days. I'll review the whole shebang and try to figure out what I was thinking.