You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by da...@apache.org on 2013/07/11 20:08:23 UTC
svn commit: r1502305 - in /subversion/trunk/subversion:
bindings/cxxhl/src/exception.cpp
bindings/swig/python/libsvn_swig_py/swigutil_py.c libsvn_subr/error.c
libsvn_subr/nls.c mod_dav_svn/mod_dav_svn.c
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):
* subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
(svn_swig_py_release_py_lock):
* subversion/libsvn_subr/error.c
(make_error_internal, svn_error_dup, svn_handle_error2):
* subversion/libsvn_subr/nls.c
(svn_nls_init):
* subversion/mod_dav_svn/mod_dav_svn.c
(merge_xml_in_filter):
s/apr_pool_create/svn_pool_create/
Modified:
subversion/trunk/subversion/bindings/cxxhl/src/exception.cpp
subversion/trunk/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
subversion/trunk/subversion/libsvn_subr/error.c
subversion/trunk/subversion/libsvn_subr/nls.c
subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Modified: subversion/trunk/subversion/bindings/cxxhl/src/exception.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/cxxhl/src/exception.cpp?rev=1502305&r1=1502304&r2=1502305&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/cxxhl/src/exception.cpp (original)
+++ subversion/trunk/subversion/bindings/cxxhl/src/exception.cpp Thu Jul 11 18:08:23 2013
@@ -334,7 +334,7 @@ Error::MessageList Error::compile_messag
empties.reserve(max_length);
apr_pool_t* pool = NULL;
- apr_pool_create(&pool, NULL);
+ pool = svn_pool_create(NULL);
try
{
for (const Error* err = this; err; err = err->m_nested.get())
Modified: subversion/trunk/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c?rev=1502305&r1=1502304&r2=1502305&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (original)
+++ subversion/trunk/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c Thu Jul 11 18:08:23 2013
@@ -93,7 +93,7 @@ void svn_swig_py_release_py_lock(void)
if (_saved_thread_key == NULL)
{
/* Obviously, creating a top-level pool for this is pretty stupid. */
- apr_pool_create(&_saved_thread_pool, NULL);
+ _saved_thread_pool = svn_pool_create(NULL);
apr_threadkey_private_create(&_saved_thread_key, NULL,
_saved_thread_pool);
}
Modified: subversion/trunk/subversion/libsvn_subr/error.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/error.c?rev=1502305&r1=1502304&r2=1502305&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/error.c (original)
+++ subversion/trunk/subversion/libsvn_subr/error.c Thu Jul 11 18:08:23 2013
@@ -110,7 +110,8 @@ make_error_internal(apr_status_t apr_err
pool = child->pool;
else
{
- if (apr_pool_create(&pool, NULL))
+ pool = svn_pool_create(NULL);
+ if (!pool)
abort();
}
@@ -339,7 +340,8 @@ svn_error_dup(svn_error_t *err)
apr_pool_t *pool;
svn_error_t *new_err = NULL, *tmp_err = NULL;
- if (apr_pool_create(&pool, NULL))
+ pool = svn_pool_create(NULL);
+ if (!pool)
abort();
for (; err; err = err->child)
@@ -559,7 +561,7 @@ svn_handle_error2(svn_error_t *err,
preferring apr_pool_*() instead. I can't remember why -- it may
be an artifact of r843793, or it may be for some deeper reason --
but I'm playing it safe and using apr_pool_*() here too. */
- apr_pool_create(&subpool, err->pool);
+ subpool = svn_pool_create(err->pool);
empties = apr_array_make(subpool, 0, sizeof(apr_status_t));
tmp_err = err;
Modified: subversion/trunk/subversion/libsvn_subr/nls.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/nls.c?rev=1502305&r1=1502304&r2=1502305&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/nls.c (original)
+++ subversion/trunk/subversion/libsvn_subr/nls.c Thu Jul 11 18:08:23 2013
@@ -58,7 +58,7 @@ svn_nls_init(void)
apr_pool_t* pool;
apr_size_t inwords, outbytes, outlength;
- apr_pool_create(&pool, 0);
+ pool = svn_pool_create(0);
/* get exe name - our locale info will be in '../share/locale' */
inwords = GetModuleFileNameW(0, ucs2_path,
sizeof(ucs2_path) / sizeof(ucs2_path[0]));
Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=1502305&r1=1502304&r2=1502305&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Jul 11 18:08:23 2013
@@ -40,6 +40,7 @@
#include "svn_utf.h"
#include "svn_ctype.h"
#include "svn_dso.h"
+#include "svn_pools.h"
#include "mod_dav_svn.h"
#include "private/svn_fspath.h"
@@ -1009,7 +1010,7 @@ merge_xml_in_filter(ap_filter_t *f,
f->ctx = ctx = apr_palloc(r->pool, sizeof(*ctx));
ctx->parser = apr_xml_parser_create(r->pool);
ctx->bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
- apr_pool_create(&ctx->pool, r->pool);
+ ctx->pool = svn_pool_create(r->pool);
}
rv = ap_get_brigade(f->next, ctx->bb, mode, block, readbytes);
Re: svn commit: r1502305 - in /subversion/trunk/subversion: bindings/cxxhl/src/exception.cpp
bindings/swig/python/libsvn_swig_py/swigutil_py.c libsvn_subr/error.c libsvn_subr/nls.c
mod_dav_svn/mod_dav_svn.c
Posted by Branko Čibej <br...@wandisco.com>.
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, danielsh@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@wandisco.com
Re: svn commit: r1502305 - in /subversion/trunk/subversion:
bindings/cxxhl/src/exception.cpp
bindings/swig/python/libsvn_swig_py/swigutil_py.c libsvn_subr/error.c
libsvn_subr/nls.c mod_dav_svn/mod_dav_svn.c
Posted by Daniel Shahaf <da...@apache.org>.
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, danielsh@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()?
Re: svn commit: r1502305 - in /subversion/trunk/subversion:
bindings/cxxhl/src/exception.cpp bindings/swig/python/libsvn_swig_py/swigutil_py.c
libsvn_subr/error.c libsvn_subr/nls.c mod_dav_svn/mod_dav_svn.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Jul 11, 2013 at 3:53 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 11.07.2013 20:08, danielsh@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?
Cheers,
-g
Re: svn commit: r1502305 - in /subversion/trunk/subversion: bindings/cxxhl/src/exception.cpp
bindings/swig/python/libsvn_swig_py/swigutil_py.c libsvn_subr/error.c libsvn_subr/nls.c
mod_dav_svn/mod_dav_svn.c
Posted by Branko Čibej <br...@wandisco.com>.
On 11.07.2013 20:08, danielsh@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. I seem to recall
we've been through this before, we avoid using the svn_pool APIs there
on purpose.
Not to mention that your change breaks the cxxhl build.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1502305 - in /subversion/trunk/subversion: bindings/cxxhl/src/exception.cpp
bindings/swig/python/libsvn_swig_py/swigutil_py.c libsvn_subr/error.c libsvn_subr/nls.c
mod_dav_svn/mod_dav_svn.c
Posted by Branko Čibej <br...@wandisco.com>.
On 11.07.2013 20:08, danielsh@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. I seem to recall
we've been through this before, we avoid using the svn_pool APIs there
on purpose.
Not to mention that your change breaks the cxxhl build.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com