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 <br...@wandisco.com> wrote: >>> On 11.07.2013 20:08, danie...@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. br...@wandisco.com