On 11.07.2013 23:31, Daniel Shahaf wrote:
> On Thu, Jul 11, 2013 at 05:18:28PM -0400, Greg Stein wrote:
>> On Thu, Jul 11, 2013 at 3:53 PM, Branko Čibej <brane_at_wandisco.com> wrote:
>>> On 11.07.2013 20:08, danielsh_at_apache.org wrote:
>>>> Author: danielsh
>>>> Date: Thu Jul 11 18:08:23 2013
>>>> New Revision: 1502305
>>>>
>>>> URL: http://svn.apache.org/r1502305
>>>> Log:
>>>> Use svn_pool_create() instead of apr_pool_create().
>>>>
>>>> Presently, this means that if an apr_pool_create() fails, abort_fn() will be
>>>> called. None of those plafces check for NULL results from the allocator,
>>>> so the net effect is changing a NULL dereference to calling our pool.c
>>>> function abort_on_pool_failure() (which is marginally better).
>>>>
>>>> * subversion/bindings/cxxhl/src/exception.cpp
>>>> (Error::compile_messages):
>>> This change is wrong, please revert it. I agree the code needs to check
>>> for the null return, however, replacing the current mode with an abort
>>> is not "marginally better", it's completely wrong.
>> How is a NULL dereference better? We can't catch that in some way, can we?
>>
>> Or will you simply be adding the NULL checks?
> exception.cpp does not allocate anything; it just passes that pool to
> libsvn_subr/utf.c functions. Perhaps Brane intends to catch the apr_status_t
> return value of apr_pool_create()?
I'm going to replace apr_pool_create by using the (private) APR::Pool
class that I added a couple days ago, and that will take care of
lifetime management and (eventually) error management. That currently
uses svn_pool_*, but may not at some later time; for now I've been
concentrating on getting the interface right and not worrying about
implementation details too much.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane_at_wandisco.com
Received on 2013-07-12 11:41:55 CEST