Thread issues in svnserve (was Re: 1.8.0-rc1 up for testing/signing)

On Sat, May 4, 2013 at 4:01 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On Sat, May 4, 2013 at 4:34 AM, Ben Reser <ben_at_reser.org> wrote:
>> Here it is: the first Release Candidate for Subversion 1.8.0. You can
>> fetch the proposed tarballs from here:
>> https://dist.apache.org/repos/dist/dev/subversion
>>
> I've got crash in svnserve when running 32-bit test suite over svn://
> protocol on Windows 7 (x64).
[..]
>
> I'm running test suite again to check if this issue has stable reproduction.
>
The crash happened again.

It seems problem in apr_thread_* implementation and svnserve pool
management. Problem that we allocate apr_thread_t and apr_threadattr_t
in connection_pool which is destroyed when created thread completes.
This lead access to freed memory in apr_thread_create() when
connection thread completes very fast. See apr_thread_create()
implementation on Windows (apr\threadproc\win32\thread.c:82)
[[[
APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
                                            apr_threadattr_t *attr,
                                            apr_thread_start_t func,
                                            void *data, apr_pool_t *pool)
{
    apr_status_t stat;
        unsigned temp;
    HANDLE handle;

    (*new) = (apr_thread_t *)apr_palloc(pool, sizeof(apr_thread_t));

    if ((*new) == NULL) {
        return APR_ENOMEM;
    }

    (*new)->data = data;
    (*new)->func = func;
    (*new)->td = NULL;
    stat = apr_pool_create(&(*new)->pool, pool);
    if (stat != APR_SUCCESS) {
        return stat;
    }

    /* Use 0 for default Thread Stack Size, because that will
     * default the stack to the same size as the calling thread.
     */
#ifndef _WIN32_WCE
    if ((handle = (HANDLE)_beginthreadex(NULL,
                        (DWORD) (attr ? attr->stacksize : 0),
                        (unsigned int (APR_THREAD_FUNC *)(void *))dummy_worker,
                        (*new), 0, &temp)) == 0) {
        return APR_FROM_OS_ERROR(_doserrno);
    }
#else
   if ((handle = CreateThread(NULL,
                        attr && attr->stacksize > 0 ? attr->stacksize : 0,
                        (unsigned int (APR_THREAD_FUNC *)(void *))dummy_worker,
                        (*new), 0, &temp)) == 0) {
        return apr_get_os_error();
    }
#endif
    if (attr && attr->detach) {
    ^^^^^^^^^^^^^^ crash here because attr is allocated in
connection_pool which can be destroyed at this time
        CloseHandle(handle);
    }
    else
        (*new)->td = handle;

    return APR_SUCCESS;
}
]]]

-- 
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com