On 'xmllite' branch: Merge changes from trunk.

git-svn-id: https://svn.apache.org/repos/asf/apr/apr/branches/xmllite@1860054 13f79535-47bb-0310-9956-ffa450edef68

9 files changed, 109 insertions, 185 deletions

diff --git a/CHANGES b/CHANGES
index 0ba7700de..7699e9119 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,12 @@
                                                      -*- coding: utf-8 -*-
 Changes for APR 2.0.0
 
+  *) apr_rwlock_t: Use native Slim Reader/Writer (SRW) locks on Windows.
+     PR 51360. [Ivan Zhakov]
+
+  *) Fix possible race condition in the Win32 apr_rwlock_t implementation.
+     PR 45455. [Ivan Zhakov]
+
   *) apr_thread_once: Fix problem that apr_thread_once can return before
      the other thread completes initialization on Windows.
 
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 004452d6b..61eb436b9 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -162,6 +162,7 @@ SET(APR_SYSTEM_LIBS
   ws2_32
   mswsock
   rpcrt4
+  bcrypt
 )
 
 INCLUDE_DIRECTORIES(${APR_INCLUDE_DIRECTORIES} ${XMLLIB_INCLUDE_DIR})
diff --git a/dso/win32/dso.c b/dso/win32/dso.c
index 9681d91b5..ef5503b98 100644
--- a/dso/win32/dso.c
+++ b/dso/win32/dso.c
@@ -57,7 +57,7 @@ APR_DECLARE(apr_status_t) apr_dso_load(struct apr_dso_handle_t **res_handle,
     HINSTANCE os_handle;
     apr_status_t rv;
 #ifndef _WIN32_WCE
-    UINT em;
+    DWORD em;
 #endif
 
     apr_wchar_t wpath[APR_PATH_MAX];
diff --git a/file_io/win32/filestat.c b/file_io/win32/filestat.c
index cb0a32595..45dd7604d 100644
--- a/file_io/win32/filestat.c
+++ b/file_io/win32/filestat.c
@@ -793,68 +793,50 @@ APR_DECLARE(apr_status_t) apr_file_attrs_set(const char *fname,
                                              apr_fileattrs_t attr_mask,
                                              apr_pool_t *pool)
 {
-    DWORD flags;
+    DWORD old_flags;
+    DWORD new_flags;
     apr_status_t rv;
-#if APR_HAS_UNICODE_FS
     apr_wchar_t wfname[APR_PATH_MAX];
-#endif
 
     /* Don't do anything if we can't handle the requested attributes */
     if (!(attr_mask & (APR_FILE_ATTR_READONLY
                        | APR_FILE_ATTR_HIDDEN)))
         return APR_SUCCESS;
 
-#if APR_HAS_UNICODE_FS
-    IF_WIN_OS_IS_UNICODE
-    {
-        if ((rv = utf8_to_unicode_path(wfname,
-                                       sizeof(wfname) / sizeof(wfname[0]),
-                                       fname)))
-            return rv;
-        flags = GetFileAttributesW(wfname);
-    }
-#endif
-#if APR_HAS_ANSI_FS
-    ELSE_WIN_OS_IS_ANSI
-    {
-        flags = GetFileAttributesA(fname);
-    }
-#endif
+    if ((rv = utf8_to_unicode_path(wfname,
+                                    sizeof(wfname) / sizeof(wfname[0]),
+                                    fname)))
+        return rv;
 
-    if (flags == 0xFFFFFFFF)
+    old_flags = GetFileAttributesW(wfname);
+    if (old_flags == 0xFFFFFFFF)
         return apr_get_os_error();
 
+    new_flags = old_flags;
     if (attr_mask & APR_FILE_ATTR_READONLY)
     {
         if (attributes & APR_FILE_ATTR_READONLY)
-            flags |= FILE_ATTRIBUTE_READONLY;
+            new_flags |= FILE_ATTRIBUTE_READONLY;
         else
-            flags &= ~FILE_ATTRIBUTE_READONLY;
+            new_flags &= ~FILE_ATTRIBUTE_READONLY;
     }
 
     if (attr_mask & APR_FILE_ATTR_HIDDEN)
     {
         if (attributes & APR_FILE_ATTR_HIDDEN)
-            flags |= FILE_ATTRIBUTE_HIDDEN;
+            new_flags |= FILE_ATTRIBUTE_HIDDEN;
         else
-            flags &= ~FILE_ATTRIBUTE_HIDDEN;
+            new_flags &= ~FILE_ATTRIBUTE_HIDDEN;
     }
 
-#if APR_HAS_UNICODE_FS
-    IF_WIN_OS_IS_UNICODE
-    {
-        rv = SetFileAttributesW(wfname, flags);
-    }
-#endif
-#if APR_HAS_ANSI_FS
-    ELSE_WIN_OS_IS_ANSI
-    {
-        rv = SetFileAttributesA(fname, flags);
+    /* Don't do anything if we are not going to change attributes. */
+    if (new_flags == old_flags) {
+        return APR_SUCCESS;
     }
-#endif
 
-    if (rv == 0)
+    if (!SetFileAttributesW(wfname, new_flags)) {
         return apr_get_os_error();
+    }
 
     return APR_SUCCESS;
 }
diff --git a/file_io/win32/open.c b/file_io/win32/open.c
index b6c01432e..f3b93f5ae 100644
--- a/file_io/win32/open.c
+++ b/file_io/win32/open.c
@@ -260,7 +260,7 @@ static apr_status_t make_sparse_file(apr_file_t *file)
             } while (res == WAIT_ABANDONED);
 
             if (res != WAIT_OBJECT_0) {
-                CancelIo(file->filehand);
+                CancelIoEx(file->filehand, file->pOverlapped);
             }
 
             if (GetOverlappedResult(file->filehand, file->pOverlapped, 
diff --git a/file_io/win32/readwrite.c b/file_io/win32/readwrite.c
index 99c4bdfe9..bf7cbfb30 100644
--- a/file_io/win32/readwrite.c
+++ b/file_io/win32/readwrite.c
@@ -99,7 +99,7 @@ static apr_status_t read_with_timeout(apr_file_t *file, void *buf, apr_size_t le
              * the operation in progress.
              */
             if (res != WAIT_OBJECT_0) {
-                CancelIo(file->filehand);
+                CancelIoEx(file->filehand, file->pOverlapped);
             }
 
             /* Ignore any failures above.  Attempt to complete
@@ -490,10 +490,10 @@ APR_DECLARE(apr_status_t) apr_file_write(apr_file_t *thefile, const void *buf, a
             (*nbytes) = 0;
             rv = apr_get_os_error();
 
-            /* XXX: This must be corrected, per the apr_file_read logic!!! */
             if (rv == APR_FROM_OS_ERROR(ERROR_IO_PENDING)) {
  
                 DWORD timeout_ms;
+                DWORD res;
 
                 if (thefile->timeout == 0) {
                     timeout_ms = 0;
@@ -505,26 +505,37 @@ APR_DECLARE(apr_status_t) apr_file_write(apr_file_t *thefile, const void *buf, a
                     timeout_ms = (DWORD)(thefile->timeout / 1000);
                 }
 	       
-                rv = WaitForSingleObject(thefile->pOverlapped->hEvent, timeout_ms);
-                switch (rv) {
-                    case WAIT_OBJECT_0:
-                        GetOverlappedResult(thefile->filehand, thefile->pOverlapped, 
-                                            &bwrote, TRUE);
-                        *nbytes = bwrote;
-                        rv = APR_SUCCESS;
-                        break;
-                    case WAIT_TIMEOUT:
-                        rv = (timeout_ms == 0) ? APR_EAGAIN : APR_TIMEUP;
-                        break;
-                    case WAIT_FAILED:
-                        rv = apr_get_os_error();
-                        break;
-                    default:
-                        break;
+                res = WaitForSingleObject(thefile->pOverlapped->hEvent, timeout_ms);
+
+                /* There is one case that represents entirely
+                 * successful operations, otherwise we will cancel
+                 * the operation in progress.
+                 */
+                if (res != WAIT_OBJECT_0) {
+                    CancelIoEx(thefile->filehand, thefile->pOverlapped);
                 }
-                if (rv != APR_SUCCESS) {
-                    if (apr_os_level >= APR_WIN_98)
-                        CancelIo(thefile->filehand);
+
+                /* Ignore any failures above.  Attempt to complete
+                 * the overlapped operation and use only _its_ result.
+                 * For example, CancelIo or WaitForSingleObject can
+                 * fail if the handle is closed, yet the read may have
+                 * completed before we attempted to CancelIo...
+                 */
+                if (GetOverlappedResult(thefile->filehand, thefile->pOverlapped,
+                                        &bwrote, TRUE)) {
+                    *nbytes = bwrote;
+                    rv = APR_SUCCESS;
+                }
+                else {
+                    rv = apr_get_os_error();
+                    if (((rv == APR_FROM_OS_ERROR(ERROR_IO_INCOMPLETE))
+                        || (rv == APR_FROM_OS_ERROR(ERROR_OPERATION_ABORTED)))
+                        && (res == WAIT_TIMEOUT))
+                        rv = APR_TIMEUP;
+
+                    if (rv == APR_TIMEUP && timeout_ms == 0) {
+                        rv = APR_EAGAIN;
+                    }
                 }
             }
         }
diff --git a/include/arch/win32/apr_arch_thread_rwlock.h b/include/arch/win32/apr_arch_thread_rwlock.h
index 1177e5291..7633bf56d 100644
--- a/include/arch/win32/apr_arch_thread_rwlock.h
+++ b/include/arch/win32/apr_arch_thread_rwlock.h
@@ -21,9 +21,8 @@
 
 struct apr_thread_rwlock_t {
     apr_pool_t *pool;
-    HANDLE      write_mutex;
-    HANDLE      read_event;
-    LONG        readers;
+    SRWLOCK lock;
+    int has_wrlock;
 };
 
 #endif  /* THREAD_RWLOCK_H */
diff --git a/locks/win32/thread_rwlock.c b/locks/win32/thread_rwlock.c
index fd9d579f1..009b6fd43 100644
--- a/locks/win32/thread_rwlock.c
+++ b/locks/win32/thread_rwlock.c
@@ -21,104 +21,32 @@
 #include "apr_arch_thread_rwlock.h"
 #include "apr_portable.h"
 
-static apr_status_t thread_rwlock_cleanup(void *data)
+APR_DECLARE(apr_status_t) apr_thread_rwlock_create(apr_thread_rwlock_t **rwlock_p,
+                                                   apr_pool_t *pool)
 {
-    apr_thread_rwlock_t *rwlock = data;
-    
-    if (! CloseHandle(rwlock->read_event))
-        return apr_get_os_error();
-
-    if (! CloseHandle(rwlock->write_mutex))
-        return apr_get_os_error();
-    
-    return APR_SUCCESS;
-}
-
-APR_DECLARE(apr_status_t)apr_thread_rwlock_create(apr_thread_rwlock_t **rwlock,
-                                                  apr_pool_t *pool)
-{
-    *rwlock = apr_palloc(pool, sizeof(**rwlock));
+    apr_thread_rwlock_t* rwlock = apr_palloc(pool, sizeof(*rwlock));
 
-    (*rwlock)->pool        = pool;
-    (*rwlock)->readers     = 0;
+    rwlock->pool = pool;
+    rwlock->has_wrlock = 0;
+    InitializeSRWLock(&rwlock->lock);
 
-    if (! ((*rwlock)->read_event = CreateEvent(NULL, TRUE, FALSE, NULL))) {
-        *rwlock = NULL;
-        return apr_get_os_error();
-    }
-
-    if (! ((*rwlock)->write_mutex = CreateMutex(NULL, FALSE, NULL))) {
-        CloseHandle((*rwlock)->read_event);
-        *rwlock = NULL;
-        return apr_get_os_error();
-    }
+    *rwlock_p = rwlock;
 
-    apr_pool_cleanup_register(pool, *rwlock, thread_rwlock_cleanup,
-                              apr_pool_cleanup_null);
-
-    return APR_SUCCESS;
-}
-
-static apr_status_t apr_thread_rwlock_rdlock_core(apr_thread_rwlock_t *rwlock,
-                                                  DWORD  milliseconds)
-{
-    DWORD   code = WaitForSingleObject(rwlock->write_mutex, milliseconds);
-
-    if (code == WAIT_FAILED || code == WAIT_TIMEOUT)
-        return APR_FROM_OS_ERROR(code);
-
-    /* We've successfully acquired the writer mutex, we can't be locked
-     * for write, so it's OK to add the reader lock.  The writer mutex
-     * doubles as race condition protection for the readers counter.   
-     */
-    InterlockedIncrement(&rwlock->readers);
-    
-    if (! ResetEvent(rwlock->read_event))
-        return apr_get_os_error();
-    
-    if (! ReleaseMutex(rwlock->write_mutex))
-        return apr_get_os_error();
-    
     return APR_SUCCESS;
 }
 
 APR_DECLARE(apr_status_t) apr_thread_rwlock_rdlock(apr_thread_rwlock_t *rwlock)
 {
-    return apr_thread_rwlock_rdlock_core(rwlock, INFINITE);
-}
+    AcquireSRWLockShared(&rwlock->lock);
 
-APR_DECLARE(apr_status_t) 
-apr_thread_rwlock_tryrdlock(apr_thread_rwlock_t *rwlock)
-{
-    return apr_thread_rwlock_rdlock_core(rwlock, 0);
+    return APR_SUCCESS;
 }
 
-static apr_status_t 
-apr_thread_rwlock_wrlock_core(apr_thread_rwlock_t *rwlock, DWORD milliseconds)
+APR_DECLARE(apr_status_t)
+apr_thread_rwlock_tryrdlock(apr_thread_rwlock_t *rwlock)
 {
-    DWORD   code = WaitForSingleObject(rwlock->write_mutex, milliseconds);
-
-    if (code == WAIT_FAILED || code == WAIT_TIMEOUT)
-        return APR_FROM_OS_ERROR(code);
-
-    /* We've got the writer lock but we have to wait for all readers to
-     * unlock before it's ok to use it.
-     */
-    if (rwlock->readers) {
-        /* Must wait for readers to finish before returning, unless this
-         * is an trywrlock (milliseconds == 0):
-         */
-        code = milliseconds
-          ? WaitForSingleObject(rwlock->read_event, milliseconds)
-          : WAIT_TIMEOUT;
-        
-        if (code == WAIT_FAILED || code == WAIT_TIMEOUT) {
-            /* Unable to wait for readers to finish, release write lock: */
-            if (! ReleaseMutex(rwlock->write_mutex))
-                return apr_get_os_error();
-            
-            return APR_FROM_OS_ERROR(code);
-        }
+    if (!TryAcquireSRWLockShared(&rwlock->lock)) {
+        return APR_EBUSY;
     }
 
     return APR_SUCCESS;
@@ -126,40 +54,43 @@ apr_thread_rwlock_wrlock_core(apr_thread_rwlock_t *rwlock, DWORD milliseconds)
 
 APR_DECLARE(apr_status_t) apr_thread_rwlock_wrlock(apr_thread_rwlock_t *rwlock)
 {
-    return apr_thread_rwlock_wrlock_core(rwlock, INFINITE);
+    AcquireSRWLockExclusive(&rwlock->lock);
+    rwlock->has_wrlock = 1;
+
+    return APR_SUCCESS;
 }
 
 APR_DECLARE(apr_status_t)apr_thread_rwlock_trywrlock(apr_thread_rwlock_t *rwlock)
 {
-    return apr_thread_rwlock_wrlock_core(rwlock, 0);
+    if (!TryAcquireSRWLockExclusive(&rwlock->lock)) {
+        return APR_EBUSY;
+    }
+
+    rwlock->has_wrlock = 1;
+
+    return APR_SUCCESS;
 }
 
 APR_DECLARE(apr_status_t) apr_thread_rwlock_unlock(apr_thread_rwlock_t *rwlock)
 {
-    apr_status_t rv = 0;
-
-    /* First, guess that we're unlocking a writer */
-    if (! ReleaseMutex(rwlock->write_mutex))
-        rv = apr_get_os_error();
-    
-    if (rv == APR_FROM_OS_ERROR(ERROR_NOT_OWNER)) {
-        /* Nope, we must have a read lock */
-        if (rwlock->readers &&
-            ! InterlockedDecrement(&rwlock->readers) &&
-            ! SetEvent(rwlock->read_event)) {
-            rv = apr_get_os_error();
-        }
-        else {
-            rv = 0;
-        }
+    if (rwlock->has_wrlock) {
+        /* Someone holds the write lock. It MUST be the calling thread, since
+           the caller wants to unlock. */
+        rwlock->has_wrlock = 0;
+        ReleaseSRWLockExclusive(&rwlock->lock);
+    }
+    else {
+        /* Calling thread holds the read lock. */
+        ReleaseSRWLockShared(&rwlock->lock);
     }
 
-    return rv;
+    return APR_SUCCESS;
 }
 
 APR_DECLARE(apr_status_t) apr_thread_rwlock_destroy(apr_thread_rwlock_t *rwlock)
 {
-    return apr_pool_cleanup_run(rwlock->pool, rwlock, thread_rwlock_cleanup);
+    /* SRW lock is statically allocated. */
+    return APR_SUCCESS;
 }
 
 APR_POOL_IMPLEMENT_ACCESSOR(thread_rwlock)
diff --git a/misc/win32/rand.c b/misc/win32/rand.c
index cb5a6537f..855fbe4fd 100644
--- a/misc/win32/rand.c
+++ b/misc/win32/rand.c
@@ -16,7 +16,8 @@
 
 #include "apr.h"
 #include <rpc.h>
-#include <wincrypt.h>
+#include <winnt.h>
+#include <bcrypt.h>
 #include "apr_private.h"
 #include "apr_general.h"
 #include "apr_portable.h"
@@ -26,27 +27,20 @@
 APR_DECLARE(apr_status_t) apr_generate_random_bytes(unsigned char * buf,
                                                     apr_size_t length)
 {
-    HCRYPTPROV hProv;
-    apr_status_t res = APR_SUCCESS;
+    NTSTATUS ntstatus;
 
-    /* 0x40 bit = CRYPT_SILENT, only introduced in more recent PSDKs 
-     * and will only work for Win2K and later.
-     */
-    DWORD flags = CRYPT_VERIFYCONTEXT
-                | ((apr_os_level >= APR_WIN_2000) ? 0x40 : 0);
+    ntstatus = BCryptGenRandom(NULL, buf, (DWORD) length,
+                               BCRYPT_USE_SYSTEM_PREFERRED_RNG);
 
-    if (!CryptAcquireContext(&hProv, NULL, NULL, PROV_RSA_FULL, flags)) {
-	return apr_get_os_error();
+    if (BCRYPT_SUCCESS(ntstatus)) {
+        return APR_SUCCESS;
     }
-    /* XXX: An ugly hack for Win64, randomness is such that noone should
-     * ever expect > 2^31 bytes of data at once without the prng
-     * coming to a complete halt.
-     */
-    if (!CryptGenRandom(hProv, (DWORD)length, buf)) {
-    	res = apr_get_os_error();
+    else if (ntstatus == STATUS_INVALID_PARAMETER) {
+        return APR_FROM_OS_ERROR(ERROR_INVALID_PARAMETER);
+    }
+    else {
+        return APR_EGENERAL;
     }
-    CryptReleaseContext(hProv, 0);
-    return res;
 }