jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.06k stars 1.72k forks source link

crypto_pwhash_MEMLIMIT_MIN and crypto_pwhash_MEMLIMIT_MAX are different types #967

Closed ghost closed 4 years ago

ghost commented 4 years ago

The MEMLIMIT_MIN and MEMLIMIT_MAX constants are of different integer size. When trying to use them in formatted output, they need different placeholder types; otherwise the compiler issues warnings (correctly) when you refer to them in formatstrings with the same placeholder.

It's just a very minor issue: crypto_pwhash_MEMLIMIT_MIN needs %u placeholder, while crypto_pwhash_MEMLIMIT_MAX needs %lu placeholder.

This can be run into when parsing/verifying commandline arguments for memlimit values like in this example:

if(memlimit < crypto_pwhash_MEMLIMIT_MIN ||
   memlimit > crypto_pwhash_MEMLIMIT_MAX)
{
    fprintf(stderr, "memlimit needs to be between %u and %lu.\n",
            crypto_pwhash_MEMLIMIT_MIN, crypto_pwhash_MEMLIMIT_MAX);
    return 1;
}

I assume this can be fixed by changing #define crypto_pwhash_argon2id_MEMLIMIT_MIN 8192U to #define crypto_pwhash_argon2id_MEMLIMIT_MIN 8192ULL and setting it explicitly for crypto_pwhash_argon2id_MEMLIMIT_MAX too, and maybe for other constants as well.

Or maybe there is a good reason for not setting lengths explicitly?

jedisct1 commented 4 years ago

Constants are not explicitly typed, but you can use the functions instead.

Or maybe there is a good reason for not setting lengths explicitly?

Try to do it. For all of them. Think about them every single time you make a change. Taking care of size_t, uint64_t and unsigned long long mixes, and always thinking about updating them every single time a type changes.

This is horribly time consuming and degrades readability.