CBaquero / delta-enabled-crdts

Reference implementations of state-based CRDTs that offer deltas for all mutations.
Apache License 2.0
318 stars 36 forks source link

dotkernel's add allows escape of reference to stack created data #8

Open sixlettervariables opened 6 years ago

sixlettervariables commented 6 years ago

I ran into a crash while walking through the example code in Visual Studio, and was able to get some help from the VC++ team in tracking it down:

https://github.com/CBaquero/delta-enabled-crdts/blob/master/delta-crdts.cc#L416-L427

       dotkernel<T, K> add(const K& id, const T& val)
       {
              dotkernel<T, K> res;
              // get new dot
              pair<K, int> dot = c.makedot(id);
              // add under new dot
              ds.insert(pair<pair<K, int>, T>(dot, val));
              // make delta
              res.ds.insert(pair<pair<K, int>, T>(dot, val));
              res.c.insertdot(dot);
              return res;
       }

res is created on the stack, which ends up using the stack allocated res.cbase when assigning the reference to res.c in the ctor.

https://github.com/CBaquero/delta-enabled-crdts/blob/master/delta-crdts.cc#L312-L315

  dotkernel() : c(cbase) {} 
  // if supplied, use a shared causal context
  dotkernel(dotcontext<K> &jointc) : c(jointc) {} 
//  dotkernel(const dotkernel<T,K> &adk) : c(adk.c), ds(adk.ds) {}

Once res is copied by value on return the stack allocated value is destroyed and c is left with a dangling reference and an eventual crash. I'm not proficient enough in C++ to advise on a fix, but figured I'd pass this along.

The crash occurs in ccounter::inc when r.dk joins the result of dk.add or dk.rmv: https://github.com/CBaquero/delta-enabled-crdts/blob/master/delta-crdts.cc#L670-L687

  ccounter<V,K> inc (const V& val=1) 
  {
...
    for(const auto & dot : dots)
      r.dk.join(dk.rmv(dot));
    r.dk.join(dk.add(id,base+val));
    return r;
  }
DePizzottri commented 6 years ago

The problem occurs because of dotkernel has a reference field dotcontext and returning res in add function ends in broken (or dangling) reference.

Simple changing from reference to value fixes the problem, but i'm not 100% sure that it preserves the correct semantics of dotkernel. Hope @CBaquero will check and fix it in the right way.