This isn't particularly a libcperciva issue, but I'm opening it here because this is where all the build infrastructure originates. In kivaloo we now have a nontrivial number of binaries, and right now each of them rebuilds the objects they need from the library code. At some point we should do something about this.

The parsenum test is failing with some compilers on Solaris on the four cases with hex, which boil down to

strtod("0x7F", &eptr);

This is because Solaris' strtod() supports C90 by default [1], because checking for hex was a change in C99 and they didn't want to break existing code. (The effect of this change is discussed in "Application Usage" in POSIX: https://pubs.opengroup.org/onlinepubs/9699919799/functions/strtod.html, so I guess it's not as ridiculous as it sounds at first glance?)

[1]

$ man 3 strtod
In default mode for strtod(), only decimal, INF/INFINITY, and
NAN/NAN(n-char-sequence) forms are recognized. In C99/SUSv3 mode,
hexadecimal strings are also recognized.
https://docs.oracle.com/cd/E88353_01/html/E37843/strtod-3c.html

cc and c99 (custom compilers) are fine. #319 enables gcc to work. Unfortunately, the same solution doesn't work for clang:

td@solaris:~/src/libcperciva/POSIX$ clang -std=c99 posix-strtod.c 
td@solaris:~/src/libcperciva/POSIX$ ./a.out ; echo $?
1

Inspired by an old gcc bug report about Solaris and -std=c99 [2], I can get clang to work by manually using:

td@solaris:~/src/libcperciva/POSIX$ clang posix-strtod.c /usr/lib/amd64/values-xpg6.o
td@solaris:~/src/libcperciva/POSIX$ ./a.out ; echo $?
0

However, this isn't a great solution. (-std=c99 seems to have no effect on this.)

[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40411

STYLE says that _free functions should do nothing when passed NULL.

Same bug appears in crypto_aes_key_free_aesni.

This was noticed when updating the spiped Docker image to Alpine 3.17 which ships with OpenSSL 3.0:

https://github.com/TimWolla/docker-spiped/actions/runs/3594642501/jobs/6053158056

  gcc  -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\"  -I.. -I../libcperciva/alg -I../libcperciva/cpusupport -I../libcperciva/crypto -I../libcperciva/datastruct -I../libcperciva/events -I../libcperciva/netbuf -I../libcperciva/network -I../libcperciva/util -I../lib/dnsthread -I../lib/proto -I../lib/util  -O1 -c ../libcperciva/crypto/crypto_aes.c -o crypto_aes.o
  ../libcperciva/crypto/crypto_aes.c: In function 'openssl_oneshot':
  ../libcperciva/crypto/crypto_aes.c:150:9: warning: 'AES_set_encrypt_key' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
    150 |         AES_set_encrypt_key(key, (int)(len * 8), &kexp);
        |         ^~~~~~~~~~~~~~~~~~~
  In file included from ../libcperciva/crypto/crypto_aes.c:6:
  /usr/include/openssl/aes.h:51:5: note: declared here
     51 | int AES_set_encrypt_key(const unsigned char *userKey, const int bits,
        |     ^~~~~~~~~~~~~~~~~~~
  ../libcperciva/crypto/crypto_aes.c:151:9: warning: 'AES_encrypt' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
    151 |         AES_encrypt(ptext, ctext, &kexp);
        |         ^~~~~~~~~~~
  /usr/include/openssl/aes.h:57:6: note: declared here
     57 | void AES_encrypt(const unsigned char *in, unsigned char *out,
        |      ^~~~~~~~~~~
  ../libcperciva/crypto/crypto_aes.c: In function 'crypto_aes_key_expand':
  ../libcperciva/crypto/crypto_aes.c:249:9: warning: 'AES_set_encrypt_key' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
    249 |         AES_set_encrypt_key(key, (int)(len * 8), kexp);
        |         ^~~~~~~~~~~~~~~~~~~
  /usr/include/openssl/aes.h:51:5: note: declared here
     51 | int AES_set_encrypt_key(const unsigned char *userKey, const int bits,
        |     ^~~~~~~~~~~~~~~~~~~
  ../libcperciva/crypto/crypto_aes.c: In function 'crypto_aes_encrypt_block':
  ../libcperciva/crypto/crypto_aes.c:285:9: warning: 'AES_encrypt' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
    285 |         AES_encrypt(in, out, (const void *)key);
        |         ^~~~~~~~~~~
  /usr/include/openssl/aes.h:57:6: note: declared here
     57 | void AES_encrypt(const unsigned char *in, unsigned char *out,
        |      ^~~~~~~~~~~

The resulting binary appears to be fully functional.

Put the T[] tables into BSS and initialize them at run-time without the malloc 
calls and atexit cleanup.  Don't want to ship 4kB of tables in the object file, 
but BSS is free (and won't even consume extra RAM at run-time, since crc32c.c 
should only be linked in if it's being used, and it will normally be used 
throughout the process lifetime).

Then CRC32C_Init can return void.

Original issue reported on code.google.com by [email protected] on 9 Sep 2011 at 2:54

It's not always a problem, but it very often is. It would be nice if there was some way to avoid this, by not merging changes to */Makefile at all and instead regenerating them after each merge to master.

I have no idea if anything like this is possible with git, but I thought I'd throw the idea out in case it turns out that there's an easy way to do this.

should close() be retried on EINTR in util/daemonize.c and util/entropy.c?

freebsd close(2) man page says: "In case of any error except EBADF, the supplied file descriptor is de-allocated and therefore is no longer valid."

linux close(2) man page also suggests not retrying close() in case of any error (except EINTR on HP-UX).

in a multi-threaded program, retrying close() may end up closing another thread's file descriptor (this is mentioned in linux close(2) man page too).

Looks like you forgot to edit this after copying code from the CRC test?

(where "perfection" means "a synonym of 'diagnostics' or 'warnings' that begins with a p")

I'm investigating this code:

#include "parsenum.h"
int main()
{
        unsigned int val;
        // ok
        if(PARSENUM(&val, "4", 2, SIZE_MAX/2))
                return 1;
        // gcc fail
        if(PARSENUM(&val, "4", 2, SIZE_MAX/2+1))
                return 1;

        return 0;
}

It's totally fine with

clang -Weverything -Wno-reserved-id-macro -I ../../util problem.c

However, the "fail" line gives a warning with

gcc9 -Wall -Wextra -I../../util problem.c
../../util/parsenum.h:124:14: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
  124 |     ((((max) < 0) && (errno == 0)) ? \
      |  

(this is on FreeBSD 12.1, BTW, but I don't think that influences anything here)

Linux 3.17+ has getrandom(2). Linux also has getentropy(3), which theoretically behaves the same way as the getentropy(2) in OpenBSD 5.6+. FreeBSD has the kern.arandom sysctl. iOS has SecRandom. Windows has CryptGenRandom.

clang-scan 13.0.0 complaints about

y = _mm_loadu_si64(a);

in cpusupport/Build/cpusupport-X86-AESNI.c.

Clang's <emmintrin.h> header looks ok to me:

1649 | /// Loads a 64-bit integer value to the low element of a 128-bit integer
1650 | ///    vector and clears the upper element.
1651 | ///
1652 | /// \headerfile <x86intrin.h>
1653 | ///
1654 | /// This intrinsic corresponds to the <c> VMOVQ / MOVQ </c> instruction.
1655 | ///
1656 | /// \param __a
1657 | ///    A pointer to a 64-bit memory location. The address of the memory
1658 | ///    location does not have to be aligned.
1659 | /// \returns A 128-bit vector of [2 x i64] containing the loaded value.
1660 | static __inline__ __m128i __DEFAULT_FN_ATTRS
1661 | _mm_loadu_si64(void const *__a)
1662 | {
1663 |   struct __loadu_si64 {
1664 |     long long __v;
1665 |   } __attribute__((__packed__, __may_alias__));
1666 |   long long __u = ((const struct __loadu_si64*)__a)->__v;
1667 |   return __extension__ (__m128i)(__v2di){__u, 0LL};
1668 | }

clang-scan complains about 1666 in particular.

Future work: take a look at their bug tracker to see if this is a known bug in clang-scan.

I would like to point out that identifiers like "_CRYPTO_ENTROPY_H_" and "_DAEMONIZE_H_" do not fit to the expected naming convention of the C++ language standard.
Would you like to adjust your selection for unique names?

select has a hard limit on the highest socket number it supports; poll has the same (less than ideal) performance with large numbers of sockets, but doesn't have any hard limit. See Tarsnap/spiped#24.

We should detect the Intel SHA extensions and use them to speed up alg/sha256 when available.

../netbuf/netbuf_write.c:262:2: warning: Dereference of null pointer (loaded from field 'stqh_last') [core.NullDereference]
        STAILQ_INSERT_TAIL(&W->buffers, WB, entries);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../external/queue/queue.h:603:21: note: expanded from macro 'STAILQ_INSERT_TAIL'
        *(head)->stqh_last = (elm);                                     \
                 ~~~~~~~~~ ^
1 warning generated.

Once STAILQ_INIT has been called, ->stqh_last should never be NULL.

As with other false warnings from the clang analyzer, this has been addressed (in a very ugly way) in:
https://github.com/Tarsnap/libcperciva/tree/fix-clang-scan-ridiculous

According to my notes, LibreSSL 2.9.0+ has SSL_set1_host, and LibreSSL 3.3.2+ has SSL_set_hostflags. We should adjust network_ssl_compat.c to reflect this.

There's a desire to support numeric values for usernames and groupnames in setuid. I'm a bit stuck on implementing this.

Current problem: how can I validate that a digit-only argument is a potentially valid username (or group name)?

• there's no UID_T_MAX, unfortunately. :(

• At first I thought that I could use strtoumax(), and ensure that the resulting value was not larger than (uid_t)-1. However, POSIX specifies that uid_t and gid_t are integer types. No mention of signed or unsigned.
  http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html
  HP-UX for example uses uid -2 for the "nobody" user.

• for reference, FreeBSD assumes that uid_t is unsigned long... which I suppose is fair enough since they control the whole stack.
  https://github.com/freebsd/freebsd/blob/18951eee499f30623500f670bc0396817ba3b067/usr.sbin/chown/chown.c#L252

I suppose that I could just parse the values with strtoimax(), force-cast them as uid_t, and hope for the best? I mean, if a user has the ability to su and wants to be malicious, there's far easier ways than trying to setuid to numeric user 4294967297? That feels like a cop-out, though.

I don't like getopt_opt, partly because there's a long list of incompatible implementations, and partly because they all require a list of long option names separately. I think we can do better.

I'm imagining something like this:

intmax_t ch;

while ((ch = getopt_magiclong(argc, argv, "ao:v")) != -1) {
        switch (ch) {
        case 'a':
        case GETOPTSTR("awesome"):
                printf("This is awesome\n");
                break;
        case 'o':
        case GETOPTSTR("output:"):
                printf("Output file: %s\n", optarg);
                break;
        case 'v':
        case GETOPTSTR("verbose"):
                printf("Verbosity enabled\n");
                break;
        default:
                usage();
        }
}

The idea here is that rather than providing a list of options ahead of time, getopt_magiclong would recognize long options and return a hash, which would match a compile-time computed value in GETOPTSTR(). This would result in us losing support for abbreviated options (e.g., foo --verb being parsed as foo --verbose because there are no other options starting with --verb), but that's a horrible misfeature anyway. There's a chance of hash collisions, but using intmax_t that's extremely unlikely.

There would be quite a lot of preprocessor magic needed here, but I think it's doable...

I tend to refer to funcname() in comments, because that's a huge visual reminder that it's a function rather than a random noun. Many older comments just refer to funcname, without parentheses.

I should probably consistencyify this. Which do you prefer?

We're using _mm_crc32_u64(), which of course can't be used on a 32-bit platform.

I see two solutions:

1. Document it as-is. We only use the CRC32C instructions in kivaloo, which I'm pretty certain we're only using on 64-bit platforms. So we could just document that our implementation of CRC32C is only aimed at 64-bit systems, and possibly even change

   Checking if compiler supports X86 CRC32 feature...
   

   into

   Checking if compiler supports X86 CRC32 64-bit feature...
   

   to avoid confusion.

2. change the code (and cpusupport/) to support _mm_crc32_u32().

I'm leaning towards the first option, but it's your call.

The

cflags-foo.o:
	@echo '$${CFLAGS_FOO_BAR}'

notations are starting to get a bit unwieldy, and I worry that it may also end up becoming error-prone once there's a large number of them.

I'm wondering if we should have something along the lines of

/**
 * CPUSUPPORT CFLAGS: FOO_BAR FOO_BAZ
 */

in the relevant C source, and have metabuild.sh do something like CF=$(grep 'CPUSUPPORT CFLAGS:' $S | cut -f 2- -d : | while read X; do printf "$${CFLAGS_%s}\n" "$X"; done) instead of the current $(${MAKEBSD} cflags-${F}).

The API for strtol is idiotic. I need to write a wrapper around it which looks at endptr in order to distinguish between "0" and "nothing parsable as an integer here". Something like OpenBSD's strtonum probably...

libcperciva/crypto/crypto_aesctr.c
line 82

void
crypto_aesctr_free(struct crypto_aesctr * stream)
{
        int i;

        /* Be compatible with free(NULL). */
        if (stream == NULL)
                return;

        /* Zero potentially sensitive information. */
        for (i = 0; i < 16; i++)
                stream->buf[i] = 0;
        stream->bytectr = stream->nonce = 0;

Should that "zero potentially sensitive information" be replaced with insecure_memzero? It also happens inside crypto_aesctr_buf (same file).

There are a few instances of e.g., sed 's/^/\t/'. This is not portable -- sed does not handle escaped characters aside from \n.

But while I'm looking at that code: Rather than

               ( cd ${D} && grep "test:" Makefile.BSD ) >> $@;         \
               ( cd ${D} && make test );                               \
               ( cd ${D} && ${MAKEBSD} -n "test" ) | sed 's/^/\t/' >> $@;      \

why not just use awk to extract the Makefile target?

               ( cd ${D} && awk '/test:/, /^$$/' Makefile.BSD ) >> $@; \

Code like

PARSENUM(intvar, "1", 10, 20);
warnp("arg error");

produces

scrypt: arg error 1: Result too large

when I'd expect it to be "Result too small".

That's because we use ERANGE in util/parsenum.h, which is interpreted by the system as "Result too large" [1]. Actually, POSIX even gives the same string[2].
[1] https://github.com/freebsd/freebsd/blob/master/sys/sys/errno.h#L91
[2] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

At first glance, it doesn't look like the C standard has this mistake; as far as I can tell, ERANGE is just the value of errno that functions like strtol can set.

At some point I may want to switch mpool to using a stack of free allocations instead of a linked list -- reducing the pointer chasing might improve performance. I'm not sure though; I need to try it and run benchmarks (probably in kivaloo since that's a major mpool user).

I'm opening this issue as a note to myself that I should investigate this at some point.

We're using the linux getauxval API but not the FreeBSD elf_aux_info equivalent. We should do that...

We should detect the SSE4.2 CRC32 instruction and use it when available to speed up alg/crc32c.

We should detect RDRAND on x86 systems and use it as an additional source of entropy in crypto_entropy's reseed function if available.

We have tests/sha256 but not tests/md5 or tests/sha1. These aren't critical but it's probably worth having them -- I noticed the absence of md5 when I wanted to check the performance of a system.

I created a branch with the MD5 code, but it needs to be hooked into the build/test framework:

https://github.com/Tarsnap/libcperciva/tree/tests-md5

The SHA256 test doesn't initialize warnp, resulting in ugly warnings (if any are printed).

I haven't checked if other tests are similarly broken.

It seems to me that test_libcperciva.sh runs the same tests as make test in the individual test directories. Is there a reason for having two different ways to run the tests?

Notes mainly to myself.

Clang's thread safety analysis warns that it cannot handle "conditionally held locks" [1]. They give the example of:

bool b = needsToLock();
if (b) mu.Lock();
...  // Warning!  Mutex 'mu' is not held on every path through here.
if (b) mu.Unlock();

This is rather unfortunate, because phread_mutex_lock() is documented as being able to fail and set errno [2]. Well-written code will therefore not assume that the lock is successful, and will instead check the returned value and jump to error handling if necessary. In other words, well-written code will include "conditionally held locks".

[1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#conditional-locks
[2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_trylock.html

(BTW, we already have -Wno-pthread-safety-analys in spiped, for precisely this reason; the false warning arises from https://github.com/Tarsnap/spiped/blob/master/lib/dnsthread/dnsthread.c#L176)

I'm guessing this file should be marked as ignore?

Untracked files:
  (use "git add <file>..." to include in what will be committed)

        tests/buildsingles/test_buildsingles

Attempting to compile libcperciva on OpenBSD with make CC=gcc produces:

gcc -std=c99 -D_POSIX_C_SOURCE=200809L -D_XOPEN_SOURCE=700 -DCPUSUPPORT_CONFIG_FILE=\"cpusupport-config.h\"  -I../.. -I../../alg -I../../aws -I../../cpusupport -I../../util  -O2 -c ../../alg/sha256.c -o sha256.o
In file included from ../../alg/sha256.c:7:
../../alg/sha256_shani.h:14: error: static or type qualifiers in abstract declarator
../../alg/sha256_shani.h:15: error: static or type qualifiers in abstract declarator
../../alg/sha256.c:13: error: static or type qualifiers in abstract declarator
../../alg/sha256.c:14: error: static or type qualifiers in abstract declarator
../../alg/sha256.c:14: error: static or type qualifiers in abstract declarator
../../alg/sha256.c:15: error: static or type qualifiers in abstract declarator
*** Error 1 in tests/aws (Makefile:27 'sha256.o')
*** Error 1 in tests/aws (Makefile:16 'all')
*** Error 2 in /home/td/src/libcperciva (Makefile:39 'all')
openbsd$ gcc --version
gcc (GCC) 4.2.1 20070719 
Copyright (C) 2007 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

openbsd$ 

This is an old bug in gcc -std=c99, which was fixed in 4.3.0 (March 5, 2008). Unfortunately, 4.2.1 was the last version before gcc switched to GPLv3, and thus is still version that's shipped with OpenBSD. We have no reason to care about this, as OpenBSD's cc is clang... except that autotools defaults to using gcc. So scrypt and tarsnap will not compile "out of the box" on openbsd.

I can't think of any non-hideous programming solution, and I'm not inspired to propose a hideous solution to avoid a bug that was fixed 12 years ago but is still around due to licensing and/or ego problems.

The best approach I can think of right now is to slap a note in the BUILDING file of scrypt and tarsnap, warning OpenBSD people to use configure CC=cc. (I suppose that I might as well warn about spiped as well, in case somebody weirdly does make CC=gcc.)

(BTW, I re-discover this bug once a year or so, and it takes 5-10 minutes for me to remember what's happening. So this time, I think it's at least worth documented it in BUILDING.)

Notes mainly to myself. I've run out of steam right now, but I'll come back to it later (albeit possibly after clang 12.0 is out).

The clang static analyzer produces the scary-looking:

../../crypto/crypto_aesctr_shared.c:47:30: warning: The right operand of '^' is a garbage value [core.UndefinedBinaryOperatorResult]
                (*outbuf)[i] = (*inbuf)[i] ^ stream->buf[bytemod + i];
                                           ^ ~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

The first instance of this arose in 9ff90c2, which is baffling since there's nothing there that would suggest this flaw.

Tracing it backwards, it appears that the analyzer doesn't believe that be64enc() initializes the memory. This is even true if we use #include <sys/endian.h> intsead of our own be64enc.

1. first level of investigation: if we do

diff --git a/crypto/crypto_aesctr_shared.c b/crypto/crypto_aesctr_shared.c
index 531bbeb8..a27e4e02 100644
--- a/crypto/crypto_aesctr_shared.c
+++ b/crypto/crypto_aesctr_shared.c
@@ -32,6 +32,7 @@ crypto_aesctr_stream_cipherblock_generate(struct crypto_aesctr * stream)
 
        /* Encrypt the cipherblock. */
        crypto_aes_encrypt_block(stream->pblk, stream->buf, stream->key);
+       stream->buf[0] = stream->buf[0];
 }
 
 /* Encrypt ${nbytes} bytes, then update ${inbuf}, ${outbuf}, and ${buflen}. */

Then we get this warning instead of the original one:

../../crypto/crypto_aesctr_shared.c:35:17: warning: Assigned value is garbage or undefined [core.uninitialized.Assign]
        stream->buf[0] = stream->buf[0];
                       ^ ~~~~~~~~~~~~~~
1 warning generated.

So at least it doesn't have anything to do with using the cipherblock.

2. We can silence the warning entirely by doing:

diff --git a/crypto/crypto_aesctr.c b/crypto/crypto_aesctr.c
index 2b4e018e..b6483f29 100644
--- a/crypto/crypto_aesctr.c
+++ b/crypto/crypto_aesctr.c
@@ -86,6 +86,10 @@ crypto_aesctr_init2(struct crypto_aesctr * stream,
        be64enc(stream->pblk, nonce);
        stream->bytectr = 0;
 
+       /* Silence the warning?!?! */
+       for (int i = 0; i < 4; i++)
+               stream->pblk[i] = stream->pblk[i];
+
        /*
         * Set the counter such that the least significant byte will wrap once
         * incremented.

This should be a no-op, but it's enough to make clang realize that we've initialized the first 4 bytes of stream->pblk. (And yes, we must use i < 4; 3 is not enough.)

So: it appears that clang's static analyzer isn't convinced that be64enc() initializes the first 32 bits of its output. I wonder if this it's getting caught up in the compiler's "try to recognize a byte-swap" code? The assembly output shows that it did recognize it as a byte-swap:

;               stream->key = key;
                mov     qword ptr [rdi], rsi
;       p[0] = (x >> 56) & 0xff;
                bswap   rdx
;       stream->bytectr = 0;
                mov     qword ptr [rdi + 8], 0
;       p[0] = (x >> 56) & 0xff;
                mov     qword ptr [rdi + 32], rdx
;       stream->pblk[15] = 0xff;
                mov     byte ptr [rdi + 47], -1

but I know that some implementations do a pair of be32enc(). For example, FreeBSD's /usr/include/sys/endian.h contains the alignment-agnostic:

static __inline void
be64enc(void *pp, uint64_t u)
{
        uint8_t *p = (uint8_t *)pp;

        be32enc(p, (uint32_t)(u >> 32));
        be32enc(p + 4, (uint32_t)(u & 0xffffffffU));
}

So maybe there's a convoluted path of the analyzer recognizing that the first 4 bytes come from nonce >> 32, noticing that the hard-coded nonce in our test file isn't bigger than 2^32, and then... forgetting that a right-shift will introduce 0s, and is thus well-defined? Hmm, no, that's nonsense. At least, if it was the case, we'd see these warnings in earlier versions of our code, not introduced at that particular commit.

FWIW, I see this warning on linux t4g as well, so it's not just a FreeBSD amd64 thing.