You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2012/10/31 15:20:00 UTC
svn commit: r1404159 -
/subversion/trunk/subversion/libsvn_subr/named_atomic.c
Author: stefan2
Date: Wed Oct 31 14:19:59 2012
New Revision: 1404159
URL: http://svn.apache.org/viewvc?rev=1404159&view=rev
Log:
"Harden" our named atomics against data file corruption. Even if
the memory block contains completely random data, we shall never
see an access outside that buffer.
* subversion/libsvn_subr/named_atomic.c
(svn_atomic_namespace__create): make sure we access only entries
inside the mmaped buffer.
(svn_named_atomic__get): never overflow on the last entry
Modified:
subversion/trunk/subversion/libsvn_subr/named_atomic.c
Modified: subversion/trunk/subversion/libsvn_subr/named_atomic.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/named_atomic.c?rev=1404159&r1=1404158&r2=1404159&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/named_atomic.c (original)
+++ subversion/trunk/subversion/libsvn_subr/named_atomic.c Wed Oct 31 14:19:59 2012
@@ -462,10 +462,15 @@ svn_atomic_namespace__create(svn_atomic_
if (!err && new_ns->data)
{
+ /* Sanitize (in case of data corruption)
+ */
+ if (new_ns->data->count > MAX_ATOMIC_COUNT)
+ new_ns->data->count = MAX_ATOMIC_COUNT;
+
/* Cache the number of existing, complete entries. There can't be
* incomplete ones from other processes because we hold the mutex.
* Our process will also not access this information since we are
- * wither being called from within svn_atomic__init_once or by
+ * either being called from within svn_atomic__init_once or by
* svn_atomic_namespace__create for a new object.
*/
new_ns->min_used = new_ns->data->count;
@@ -538,7 +543,7 @@ svn_named_atomic__get(svn_named_atomic__
/* We only need to check for new entries.
*/
for (i = count; i < ns->data->count; ++i)
- if (strcmp(ns->data->atomics[i].name, name) == 0)
+ if (strncmp(ns->data->atomics[i].name, name, len + 1) == 0)
{
return_atomic(atomic, ns, i);
Re: svn commit: r1404159 - /subversion/trunk/subversion/libsvn_subr/named_atomic.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Oct 31, 2012 at 5:38 PM, Julian Foad <ju...@btopenworld.com>wrote:
> Philip Martin:
>
> > stefan2@apache.org writes:
> >> + /* Sanitize (in case of data corruption)
> >> + */
> >> + if (new_ns->data->count > MAX_ATOMIC_COUNT)
> >> + new_ns->data->count = MAX_ATOMIC_COUNT;
> >
> > I'm still seeing a crash:
> >
> > 467 if (new_ns->data->count > MAX_ATOMIC_COUNT)
> > (gdb) p new_ns->data->count
> > $1 = -1382404098
>
> Also, if the count is "corrupted", I want to ask: are we sure it is then
> safe to adjust the count and carry on? (I haven't been paying close
> attention, I'm just asking.)
>
You are right. When we detect that someone messed
with our on-disk data, we should simply bail out instead
of trying to limp on. svnadmin recover is the place to
try to fix things (which we do since r1404163).
Changed in r1405772.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: svn commit: r1404159 - /subversion/trunk/subversion/libsvn_subr/named_atomic.c
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin:
> stefan2@apache.org writes:
>> + /* Sanitize (in case of data corruption)
>> + */
>> + if (new_ns->data->count > MAX_ATOMIC_COUNT)
>> + new_ns->data->count = MAX_ATOMIC_COUNT;
>
> I'm still seeing a crash:
>
> 467 if (new_ns->data->count > MAX_ATOMIC_COUNT)
> (gdb) p new_ns->data->count
> $1 = -1382404098
Also, if the count is "corrupted", I want to ask: are we sure it is then safe to adjust the count and carry on? (I haven't been paying close attention, I'm just asking.)
- Julian
Re: svn commit: r1404159 - /subversion/trunk/subversion/libsvn_subr/named_atomic.c
Posted by Philip Martin <ph...@wandisco.com>.
stefan2@apache.org writes:
> Author: stefan2
> Date: Wed Oct 31 14:19:59 2012
> New Revision: 1404159
>
> URL: http://svn.apache.org/viewvc?rev=1404159&view=rev
> Log:
> "Harden" our named atomics against data file corruption. Even if
> the memory block contains completely random data, we shall never
> see an access outside that buffer.
> + /* Sanitize (in case of data corruption)
> + */
> + if (new_ns->data->count > MAX_ATOMIC_COUNT)
> + new_ns->data->count = MAX_ATOMIC_COUNT;
I'm still seeing a crash:
467 if (new_ns->data->count > MAX_ATOMIC_COUNT)
(gdb) p new_ns->data->count
$1 = -1382404098
I suppose we could either test "count < 0" or make count unsigned?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download