Serf / SSL coredumps during apr_terminate

[Evgeny Kotkov]

Hi,

I recently stumbled upon the svnrdump crash in Subversion 1.8.1 and
Subversion@trunk around r1514208. It looks like the problem is in the way
serf works with APR pools during SSL initialization / deinitialization.

The crash can be easily reproduced by issuing the
'svnrdump dump https://svn.apache.org' command (which coredumps in the
'apr_terminate' call) and happens with serf-1.2.1, serf-1.3.0 and serf@trunk
around r2135:

[[[
svnrdump dump https://svn.apache.org

svnrdump: E175002: Unexpected HTTP status 405 'Method Not Allowed' on '/'
svnrdump: E175002: Additional errors:
svnrdump: E175002: PROPFIND request on '/' failed: 405 Method Not Allowed
Segmentation fault (core dumped)
]]]

Digging a bit shows that the following things happen:

1. subversion/svnrdump/svnrdump.c:main() : main()
Creates top-level APR pool (MAIN_POOL).

2. buckets/ssl_buckets.c : ssl_init_context()
Creates another top-level pool (SSL_POOL) and ALLOCATOR within this pool,
binds them to the serf_ssl_context_t CONTEXT object.

3. subversion/svnrdump/svnrdump.c:main() : main()
SSL encryption / decryption happens and memory is allocated from SSL_POOL
via ALLOCATOR.

4. subversion/svnrdump/svnrdump.c:main() : main()
Error happens and svnrdump exits without explicitly destroying the MAIN_POOL
(its destruction is postponed until apr_terminate).

5. apr_terminate()
Pools are destroyed in reverse order: SSL_POOL is destroyed before the
MAIN_POOL. MAIN_POOL destruction closes connections and bang, you're dead:
closing connections attempts to reclaim free memory using the
CONTEXT->ALLOCATOR. CONTEXT->ALLOCATOR is invalid at this moment (because
SSL_POOL is already destroyed), so this leads to a crash.

I found a way to reproduce this problem using the serf test framework, see
the attached patch. It is not something you can commit, but I guess it might
prove useful during investigation.

I've also attached two related backtraces (crash itself and the
ALLOCATOR creation) for svrdump built with APR_POOL_DEBUG.


Thanks and regards,
Evgeny Kotkov

[Ivan Zhakov]

On Thu, Aug 15, 2013 at 8:42 PM, Evgeny Kotkov
<evgeny...@visualsvn.com> wrote:
> Hi,
>
> I recently stumbled upon the svnrdump crash in Subversion 1.8.1 and
> Subversion@trunk around r1514208. It looks like the problem is in the way
> serf works with APR pools during SSL initialization / deinitialization.
>
> The crash can be easily reproduced by issuing the
> 'svnrdump dump https://svn.apache.org' command (which coredumps in the
> 'apr_terminate' call) and happens with serf-1.2.1, serf-1.3.0 and serf@trunk
> around r2135:
>
> [[[
> svnrdump dump https://svn.apache.org
>
> svnrdump: E175002: Unexpected HTTP status 405 'Method Not Allowed' on '/'
> svnrdump: E175002: Additional errors:
> svnrdump: E175002: PROPFIND request on '/' failed: 405 Method Not Allowed
> Segmentation fault (core dumped)
> ]]]
>
> Digging a bit shows that the following things happen:
>
> 1. subversion/svnrdump/svnrdump.c:main() : main()
> Creates top-level APR pool (MAIN_POOL).
>
> 2. buckets/ssl_buckets.c : ssl_init_context()
> Creates another top-level pool (SSL_POOL) and ALLOCATOR within this pool,
> binds them to the serf_ssl_context_t CONTEXT object.
>

It's not clear why top-level pool is used in ssl_init_context(). The
attached patch removes this pool and seems fixing problem reported.
Could you please test it and confirm that problem is fixed?

Justin: do you remember why global pool is used in ssl_init_context()?

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

[Lieven Govaerts]

I assume it's because serf doesn't have access to a pool there, in the
bucket creation code only an allocator is provided.

Lieven

[Ivan Zhakov]

No, because bucket allocator has a function to retun pool used for this allocator: setf_bucket_allocator_pool_get()

 

----

Ivan Zhakov

[Evgeny Kotkov]

I can confirm that this patch fixes the problem.

I could not reproduce the crash with svnrdump built against serf@trunk, with my
quick-and-dirty patch (provoke-serf-coredump-with-ssl.patch) and with other
serf tests in 'test/test_context.c'.

[Ivan Zhakov]

> On Thu, Aug 15, 2013 at 8:56 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On Thu, Aug 15, 2013 at 8:42 PM, Evgeny Kotkov
>> <evgeny...@visualsvn.com> wrote:
>> It's not clear why top-level pool is used in ssl_init_context(). The
>> attached patch removes this pool and seems fixing problem reported.
>> Could you please test it and confirm that problem is fixed?
>>

On Tue, Aug 20, 2013 at 12:02 PM, Evgeny Kotkov
<evgeny...@visualsvn.com> wrote:
> I can confirm that this patch fixes the problem.
>
> I could not reproduce the crash with svnrdump built against serf@trunk, with my
> quick-and-dirty patch (provoke-serf-coredump-with-ssl.patch) and with other
> serf tests in 'test/test_context.c'.
>

Thanks for testing! I've commited my patch in r2146.