Use-after-free when having addRef on ReadYourWritesTransaction object

Development FoundationDB Core
1

I accidentally have code like

ReadYourWritesTransaction tr(cx); // cx is the Database object
state Reference<ReadYourWritesTransaction> trReference = Reference<ReadYourWritesTransaction>::addRef(&tr);
// ....
wait(delay(1)); // The lifetime of tr ends here
// ....
trReference->get(...); // Error since the transaction object has been deleted.

Not sure if this is okay. I searched in the codebase, there is no existing use case like this and nearly all of ReadYourWritesTransaction are declared with state, which makes the scenario that a reference lives longer than the ryw object is much less likely to happen.
However, since ReadYourWritesTransaction is ReferenceCounted, if there is an existing Reference points to it, by intuition, it should not be deleted.

2

The idea behind the Reference type is that the creation and destruction of the References results in the count stored in the ReferenceCounted object to go up and down, respectively, with the last destructed Reference reducing the count to 0 and deleting the object. This only works if the instance of the ReferenceCounted object is something you can actually delete, which is not the case for objects with automatic storage duration such as in the example above.

In your case, I believe it would be undefined behavior for us to call delete explicitly, and additionally the object is going to be automatically deleted when it goes out of scope.

For this reason, it really only makes sense to use a Reference for objects you’ve dynamically allocated using new.

1 Like
3

Thanks for the clarification. I came across the scenario where it needs to create a reference on a ryw object which has no guarantee to be dynamically allocated or not. In that case, I think using a reference has this issue.
IIUC anytime my interface needs a pointer to ryw, I have this issue. If we can force the ryw to be dynamically allocated, then the issue is solved. I know it is not convenient to do so. Thus, I need to avoid having an interface that accepts a reference to ryw. Or the user who uses the interface needs to know this constraint and always pass a dynamically allocated ryw to the interface.

4

I am a bit confused by this problem - I might misunderstand. The following should work:

state ReadYourWritesTransaction tr(cx); // cx is the Database object
// ....
wait(delay(1)); // The lifetime of tr ends here
// ....
tr.doSomething(); // will work

If you allocate the object on the stack, you can’t make it a reference (reference counting stack objects doesn’t make sense). If you need to pass pointers around you can still do that but you don’t need to reference count (instead you need to make sure that the actor that creates the object survives all actors and functions that get a pointer to it).

If you need a reference, you need to allocate it on the heap:

state Reference<ReadYourWritesTransaction> trReference(new ReadYourWritesTransaction(cx));
// ....
wait(delay(1)); // The lifetime of tr ends here
// ....
trReference->get(...); // Error since the transaction object has been deleted.
5

Yeah, this one works.
The reason I am asking is right now in special-key-space we have something like

return getDatabase()->specialKeySpace->getRange(Reference<ReadYourWritesTransaction>::addRef(this), begin,
			                                                end, limits, reverse);

Actually, here we do not know whether this is on stack or on heap.
If the user has something like(Only in native API)

ReadYourWritesTransaction tr(cx); // state ReadYourWritesTransaction tr(cx) or Reference<ReadYourWritesTransaction> trReference(new ReadYourWritesTransaction(cx)) is fine
tr->get("\xff\xff/status/json");
// Error since the code in special-key-space still holds reference to this *ryw* object

I am wondering whether this is a problem since the user may not know this.

6

I think we can restructure some of the special key space stuff so that we don’t need to use addref anymore. Also it’d be great if calling addref on an object whose lifetime is not managed by reference counting would be an internal_error

2 Likes