Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix potential NoDelta leak into IReplicatedData.Merge() #4811

Merged
merged 2 commits into from
Mar 5, 2021
Merged

Fix potential NoDelta leak into IReplicatedData.Merge() #4811

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87618b9
Fix potential NoDelta leak into IReplicatedData.Merge()
Arkatufus Mar 4, 2021
f7e46af
Merge branch 'dev' into #4806_NoDelta_leaked_into_ORDictionary_minima…
Arkatufus Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 16 additions & 19 deletions src/contrib/cluster/Akka.DistributedData/DeltaPropagationSelector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Akka.Actor;
using Akka.DistributedData.Internal;
using Akka.Event;
using Akka.Util.Internal;

namespace Akka.DistributedData
{
Expand Down Expand Up @@ -68,13 +69,11 @@ public ImmutableDictionary<Address, DeltaPropagation> CollectPropagations()
if (all.Length <= sliceSize) slice = all;
else
{
var start = (int)(_deltaNodeRoundRobinCounter % all.Length);
Copy link
Contributor Author

@Arkatufus Arkatufus Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the code also looks suspicious, so I re-ported it with a cleaner one.
Reference scala code:

val i = (deltaNodeRoundRobinCounter % all.size).toInt
val first = all.slice(i, i + sliceSize)
if (first.size == sliceSize) first
else first ++ all.take(sliceSize - first.size)

var buffer = new Address[sliceSize];
for (var i = 0; i < sliceSize; i++)
{
buffer[i] = all[(start + i) % all.Length];
}
slice = ImmutableArray.CreateRange(buffer);
var i = (int)(_deltaNodeRoundRobinCounter % all.Length);
slice = all.Slice(i, sliceSize).ToImmutableArray();
Copy link
Contributor Author

@Arkatufus Arkatufus Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks different than the reference scala because in scala, slice parameters are from and to, while in C#, slice parameters are from and count

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I was just about to ask


if (slice.Length != sliceSize)
slice = slice.AddRange(all.Take(sliceSize - slice.Length));
}

_deltaNodeRoundRobinCounter += sliceSize;
Expand Down Expand Up @@ -105,19 +104,17 @@ public ImmutableDictionary<Address, DeltaPropagation> CollectPropagations()
var cacheKey = (key, fromSeqNr, toSeqNr);
if (!cache.TryGetValue(cacheKey, out var deltaGroup))
{
using (var e = deltaEntriesAfterJ.Values.GetEnumerator())
deltaGroup = deltaEntriesAfterJ.Values.Aggregate((d1, d2) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the suspicious looking code block with one that makes a lot more sense.
This is the scala code for reference:

val group = deltaEntriesAfterJ.valuesIterator.reduceLeft { (d1, d2) =>
  val merged = d2 match {
    case NoDeltaPlaceholder => NoDeltaPlaceholder
    case _                  =>
      // this is fine also if d1 is a NoDeltaPlaceholder
      d1.merge(d2.asInstanceOf[d1.T])
  }
  merged match {
    case s: ReplicatedDeltaSize if s.deltaSize >= maxDeltaSize =>
      // discard too large deltas
      NoDeltaPlaceholder
    case _ => merged
  }
}

reduceLeft works exactly the same as LINQ Aggregate

{
e.MoveNext();
deltaGroup = e.Current;
while (e.MoveNext())
{
deltaGroup = deltaGroup.Merge(e.Current);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug happened here, a NoDelta null delta marker got passed into this line and ORDictionary could not cast it to DeltaGroup

if (deltaGroup is IReplicatedDeltaSize s && s.DeltaSize > MaxDeltaSize)
{
deltaGroup = DeltaPropagation.NoDeltaPlaceholder;
}
}
}
var merged = ReferenceEquals(d2, DeltaPropagation.NoDeltaPlaceholder)
? DeltaPropagation.NoDeltaPlaceholder
: d1.Merge(d2);

if (merged is IReplicatedDeltaSize s && s.DeltaSize >= MaxDeltaSize)
return DeltaPropagation.NoDeltaPlaceholder; // discard too large deltas

return merged;
});

cache[cacheKey] = deltaGroup;
}
Expand Down