Skip to content

Commit 7a9f0cc

Browse files
committed
Simplify _crypt_extended_init_r, and fix redundant initialization on Win32/Solaris
Looking at the history of this function, the original implementation had a bug where it would return from the middle of the function without unlocking the mutex first. The author attempted to fix this by incrementing the `initialized` flag atomically, which is not necessary, since the section which modifies the flag is protected by a mutex. Coincidentally, at the same time that all this unnecessary 'atomic' machinery was introduced, the code was also changed so that it didn't return without unlocking the mutex. So it looks like the bug was fixed by accident. It's not necessary to declare the flag as `volatile` either, since it is protected by a mutex. Further, the 'fixed' implementation was also wrong in another respect: on Windows and Solaris, the `initialized` flag was not even declared as `static`!! So the initialization of the static tables for S-boxes, P-boxes, etc. was repeated on each call to `php_crypt`, completely defeating the purpose of this function.
1 parent 9bd648b commit 7a9f0cc

File tree

3 files changed

+3
-32
lines changed

3 files changed

+3
-32
lines changed

configure.ac

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -643,13 +643,6 @@ if test "$ac_cv_func_getaddrinfo" = yes; then
643643
AC_DEFINE(HAVE_GETADDRINFO,1,[Define if you have the getaddrinfo function])
644644
fi
645645

646-
dnl Check for the __sync_fetch_and_add builtin.
647-
AC_CACHE_CHECK([for __sync_fetch_and_add], ac_cv_func_sync_fetch_and_add,
648-
[AC_LINK_IFELSE([AC_LANG_PROGRAM([], [[int x;__sync_fetch_and_add(&x,1);]])],[ac_cv_func_sync_fetch_and_add=yes],[ac_cv_func_sync_fetch_and_add=no])])
649-
if test "$ac_cv_func_sync_fetch_and_add" = yes; then
650-
AC_DEFINE(HAVE_SYNC_FETCH_AND_ADD,1,[Define if you have the __sync_fetch_and_add function])
651-
fi
652-
653646
AC_REPLACE_FUNCS(strlcat strlcpy explicit_bzero getopt)
654647
AC_FUNC_ALLOCA
655648
PHP_TIME_R_TYPE

ext/standard/config.m4

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -388,11 +388,6 @@ if test "$ac_cv_strptime_decl_fails" = "yes"; then
388388
AC_DEFINE([HAVE_STRPTIME_DECL_FAILS], 1, [whether strptime() declaration fails])
389389
fi
390390

391-
dnl
392-
dnl Check for atomic operation API availability in Solaris
393-
dnl
394-
AC_CHECK_HEADERS([atomic.h])
395-
396391
dnl
397392
dnl Check for arc4random on BSD systems
398393
dnl

ext/standard/php_crypt_r.c

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,6 @@
3939
# include <Wincrypt.h>
4040
#endif
4141

42-
#ifdef HAVE_ATOMIC_H /* Solaris 10 defines atomic API within */
43-
# include <atomic.h>
44-
#else
45-
# include <signal.h>
46-
#endif
4742
#include "php_crypt_r.h"
4843
#include "crypt_freesec.h"
4944

@@ -76,29 +71,17 @@ void php_shutdown_crypt_r()
7671

7772
void _crypt_extended_init_r(void)
7873
{
79-
#ifdef PHP_WIN32
80-
LONG volatile initialized = 0;
81-
#elif defined(HAVE_ATOMIC_H) /* Solaris 10 defines atomic API within */
82-
volatile unsigned int initialized = 0;
83-
#else
84-
static volatile sig_atomic_t initialized = 0;
85-
#endif
74+
static int initialized = 0;
8675

8776
#ifdef ZTS
8877
tsrm_mutex_lock(php_crypt_extended_init_lock);
8978
#endif
9079

9180
if (!initialized) {
92-
#ifdef PHP_WIN32
93-
InterlockedIncrement(&initialized);
94-
#elif defined(HAVE_SYNC_FETCH_AND_ADD)
95-
__sync_fetch_and_add(&initialized, 1);
96-
#elif defined(HAVE_ATOMIC_H) /* Solaris 10 defines atomic API within */
97-
membar_producer();
98-
atomic_add_int(&initialized, 1);
99-
#endif
81+
initialized = 1;
10082
_crypt_extended_init();
10183
}
84+
10285
#ifdef ZTS
10386
tsrm_mutex_unlock(php_crypt_extended_init_lock);
10487
#endif

0 commit comments

Comments
 (0)