libxcrypt icon indicating copy to clipboard operation
libxcrypt copied to clipboard

Make crypt and crypt_gensalt use thread-local output buffers.

Open besser82 opened this issue 11 months ago • 7 comments

This change makes crypt and crypt_gensalt as thread-safe as they can be without changing their interfaces. Solaris already made this change, and it’s being discussed by glibc (with suggestion that it should be pushed upstream to the C and POSIX standards committees): https://sourceware.org/ml/libc-alpha/2018-10/msg00437.html

Portable programs should still use the r-variants, though, because this is not a guaranteed feature that is portable to other implementations of these functions. Also it doesn’t help threads to not clobber their corresponding output buffer on a second call from within the same thread.

The way I've implemented this mimicks the implementation from Solaris.

I've also taken into account some problems with -fno-plt code and thread-local storage on PPC64-architectures. See: https://sourceware.org/PR32387


This also fixes #62.

besser82 avatar Jan 06 '25 17:01 besser82

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.45%. Comparing base (353e507) to head (f76672a).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #201      +/-   ##
===========================================
+ Coverage    90.44%   90.45%   +0.01%     
===========================================
  Files           36       36              
  Lines         3988     3993       +5     
  Branches       747      748       +1     
===========================================
+ Hits          3607     3612       +5     
  Misses         242      242              
  Partials       139      139              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Jan 14 '25 13:01 codecov[bot]

@besser82 I should have time to look at this early next week, but not before that.

zackw avatar Jan 14 '25 20:01 zackw

I approve of the goal here. It's something I wanted to do myself quite some time ago

If implementations look so risky and/or complicated to you, then I'm puzzled that you think the benefits outweigh the risks and effort and complexity. Maybe just don't do this?

I think we should use pthread_getspecific to acquire one thread-local pointer for each function, and allocate the bulk of the data with malloc

This sounds somewhat similar to what I saw Solaris do, https://github.com/besser82/libxcrypt/pull/62#issuecomment-2588580961

solardiz avatar Jan 27 '25 00:01 solardiz

Why care about old glibc versions? It's not like people normally build libxcrypt to replace the one shipped by their distribution.

rfc1036 avatar Jan 27 '25 00:01 rfc1036

@rfc1036 The entire point of this library is to be a drop-in replacement for old glibc's libcrypt. If it didn't need to be that, it would have a completely different API.

zackw avatar Jan 28 '25 17:01 zackw

Of course, but reasonably nobody is replacing glibc's libcrypt on existing systems!

rfc1036 avatar Jan 28 '25 18:01 rfc1036

Not true, people do it all the time, just look through the outstanding bug reports.

zackw avatar Jan 28 '25 18:01 zackw