Page MenuHomeFreeBSD

crypt_r(3): fix reentrancy problems with DES
ClosedPublic

Authored by trasz on Mon, Jun 7, 2:55 PM.

Details

Summary

This code was originally written for non-reentrant crypt(3).
In 5f521d7ba72, a thread-safe crypt_r(3) was introduced. However,
it looks like the DES implementation is still not re-entrant;
routines like setup_salt() or des_setkey() still use global
variables.

Instead of something drastic, eg removing DES support altogether,
just mark those variables as thread-local.

Given that this only applies to DES, I think the impact is minimal.

Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

trasz requested review of this revision.Mon, Jun 7, 2:55 PM
markj added inline comments.
secure/lib/libcrypt/crypt-des.c
77

It looks like this and some of the other tables below should be marked const. Can we do that as a part of this change?

83–84

Doesn't this need to be thread-local as well?

Add const, move 'static' first, and add __thread for stuff touched
by des_init().

trasz marked 2 inline comments as done.Tue, Jun 8, 1:31 PM
trasz added inline comments.
secure/lib/libcrypt/crypt-des.c
77

Good idea, done.

83–84

These only seem to be written to by des_init(), so they should be equal for all threads, but... yeah, you're right.

I don't see any problems with the change, but note that this adds something like 30KB of data per thread. Probably not the end of the world but not ideal either.

This revision is now accepted and ready to land.Tue, Jun 8, 1:59 PM

Yeah; I think it's acceptable, given it's a separate library and not libc.

(Tinderboxed.)