Implement crypt_r in libroot

[davidkaroly]

We have crypt() implemented in libroot/posix/crypt/crypt.cpp but it uses a statically defined outBuffer.

I think it would make it easier to port existing software to Haiku if we implement crypt_r() as well, and probably it won't be too difficult - unless I overlooked something.

Was there any particular reason for not implementing crypt_r() or is it only that it was not needed yet?

[waddlesplash]

Probably it just wasn't needed yet. What actually uses crypt_r?

[davidkaroly]

I stumbled on this while investigating if filezilla could be ported to Haiku.

Anyway I see now that the UFC implementation in crypt_legacy.c and crypt_legacy_util.c uses a bunch of statics and global variables.

[davidkaroly]

something strange in crypto_scrypt.cpp - it seems that some code was adapted from Tarsnap scrypt library but only part of it was used:

static void (*smix_func)(uint8_t *, size_t, uint64_t, void *, void *) = NULL;

static void
selectsmix(void)
{
        /* If generic smix works, use it. */
        if (!testsmix(crypto_scrypt_smix)) {
                smix_func = crypto_scrypt_smix;
                return;
        }

        /* If we get here, something really bad happened. */
        abort();
}

Here we are trying to select the optimal crypto_scrypt implementation but only the reference implementation is available, no optimized version. (upstream also has an SSE2 optimized version ​here)

So we end up with additional complexity (global variable etc) but no performance gain. Perhaps this code path could be simplified?

[davidkaroly]

I think we should also replace UFC crypt with MUSL crypt_des. Maybe better than what glibc did with _ufc_doit_r or FreeBSD declaring a bunch of threadlocal variables.

see:

• ​https://git.musl-libc.org/cgit/musl/tree/src/crypt/crypt_des.c
• ​https://github.com/freebsd/freebsd-src/commit/7d681ad774f00cf06c4ef910add91e0f8a79f7ae
• I won't even add the link to glibc here

[waddlesplash]

Note that our passwd files use this "crypt" to hash login passwords, so any change will have to preserve backwards compatibility for at least some time.

[davidkaroly]

Yep, understood.

I prepared a first draft that attempts to make scrypt reentrant. I'd like to upload it for review in the coming days.

CryptTest still passes - even though I'm not sure how much test coverage we have.

[davidkaroly]

most of it has been merged:

• hrev56758 - libroot: introduce crypt_r
• hrev56766 - libroot/crypt: replace crypt_des implementation with musl's
• hrev56771 - libroot/crypt: remove selectsmix

with these in place, crypt_r now seems threadsafe enough.

I have a small test program that invokes crypt_r on multiple threads, recently it hasn't found any issues. I'll try to integrate it into the unit tests located in haiku repo src/tests/ folder.

[waddlesplash]

Tests have now been merged.