[Object Ownership #4] Prevent circular ownership #295

lxfind · 2022-01-28T18:24:32Z

This PR is also part of #99.
One potential issue that could happen is creating circular ownership: after transferring the owner of an object to another object, the ownership chain forms a cycle.
There are two ways to go about this:

We constrain the input objects such that only the object at the bottom of the ownership chain can be by-value, all other objects must be by-reference. This way you cannot transfer any objects to another object that would point to something lower on the chain.
We catch it when processing a transfer event, and detect cycles at that time.

This PR goes with 2 for now, mostly because 1 would put limitations on the expressiveness of the programmability. For instance, we cannot express an intent where we want to either give out a bundle of objects or give out a specific item in that bundle.
It comes with the trade-off that we are only able to catch circular ownership after finishing the execution, instead of at order handling phase, leading to more gas charges for a failed transaction.

TODO: There are a few more things left:

Make native transfer also support transfer to object.
Make Client storage support object owning other objects.
Add tests.

sblackshear · 2022-01-29T03:12:56Z

fastx_programmability/adapter/src/adapter.rs

@@ -364,6 +382,20 @@ fn process_successful_execution<
                    // Charge extra gas based on object size if we are creating a new object.
                    gas_used += gas::calculate_object_creation_cost(&obj);
                }
+                let obj_id = obj.id();


Will this prevent circular ownership in all cases? E.g., wondering about the following scenario:

initial state: { obj1 => A1, obj2 => A2 }
tx1 [signed by A1]: transfer obj1 to obj2
tx2 [signed by A2]: transfer obj2 to obj1

In tx2, I think:

object_owner_map is {}` at line 90

at line 390, obj_id is obj2 parent is obj1

this means the loop at 391 won't be entered + the else branch will be taken at 394

In tx2, object_owner_map won't be {} at line 90, because obj1 is already owned by obj2, hence object_owner_map will start with {obj1: obj2}.

Got it, thanks. I think my scenario is an issue if we expose transfer_to_id (sorry, was still in that mental model), but not if we go with your (much wiser, much safer) transfer_to_object<T: key, R: key>(obj: T, owner: &mut R) approach of requiring a mutable reference to the owner object.

sblackshear · 2022-01-29T17:17:58Z

fastx_programmability/adapter/src/adapter.rs

+                    // If we are transfering the ownership to an object,
+                    // we need to check for circular ownership.
+                    let mut parent = owner_object_id;
+                    while parent != obj_id && object_owner_map.contains_key(&parent) {


I think a comment here pointing to transfer_to_object + explaining how the invariants it enforces relate to algorithm here would help future readers understand why this is safe (and instill appropriate fear for anyone who tries to modify this)

I think we could allow transfer_to_id directly and it should be safe too. The object (that corresponds to the id) to be transferred to must also be in the mutable input object list as well (i.e. any object that could be mutated in an order must be in the input and is mutable). So in that regard, whether we expose transfer_to_object or transfer_to_id shouldn't affect the algorithm here. The only reason I chose to expose transfer_to_object is it seems easier to use.

huitseeker · 2022-01-31T15:28:32Z

fastx_programmability/adapter/src/adapter.rs

+    for object in object_args.iter().filter(|obj| !obj.is_read_only()) {
+        if let Authenticator::Object(owner_object_id) = object.owner {
+            object_owner_map.insert(object.id(), owner_object_id);
+        }
+    }


Isn't the ownership controlled by the client? As a consequence, could this be exploited to create a memory blowup?

We could, as a defense:

use an O(1) space algorithm for cycle detection, à la Floyd, though it does not protect against the time complexity issue,

better yet, tell users that ownership chains are bounded by a constant (e.g. 100), reject transfers creating a longer chain.

better yet, since all input objects are mutable, tell users that past a certain boundary we will forcibly implement path compression (i.e. the 101th owning objects and all its ancestors are forcibly transferred to the eventual transitive owner).

Maybe not for this PR, but do we have an issue to track the resource abuse?

Good question! Yes I do plan to bound the ownership chain by a small constant. Will add TODO there.

huitseeker · 2022-01-31T15:32:55Z

fastx_programmability/adapter/src/adapter.rs

@@ -364,6 +382,20 @@ fn process_successful_execution<
                    // Charge extra gas based on object size if we are creating a new object.
                    gas_used += gas::calculate_object_creation_cost(&obj);
                }
+                let obj_id = obj.id();
+                object_owner_map.remove(&obj_id);


If we return with CircularObjectOwnership at 392, this side effect is still in the owner_object_map.

Are we sure that in all future versions of the code base, nobody will reuse that map? Should we take ownership of that argument?

This function already took ownership of owner_object_map, so nothing outside can reuse it.

lxfind requested review from huitseeker, patrickkuo, sblackshear, gdanezis and oxade January 28, 2022 18:24

lxfind changed the title ~~Prevent circular ownership~~ [Object Ownership #4] Prevent circular ownership Jan 28, 2022

sblackshear reviewed Jan 29, 2022

View reviewed changes

sblackshear approved these changes Jan 29, 2022

View reviewed changes

huitseeker reviewed Jan 31, 2022

View reviewed changes

lxfind force-pushed the check-object-ownership-mutability branch 2 times, most recently from 6821ce5 to 3a28d0a Compare January 31, 2022 23:43

lxfind force-pushed the check-circular-ownership branch from 3447bf8 to a8b1593 Compare February 1, 2022 00:09

lxfind force-pushed the check-object-ownership-mutability branch from 3a28d0a to 56246e5 Compare February 1, 2022 00:11

Base automatically changed from check-object-ownership-mutability to main February 1, 2022 00:39

lxfind force-pushed the check-circular-ownership branch from a8b1593 to 2a3922b Compare February 1, 2022 00:41

Prevent circular ownership

5ca9191

lxfind force-pushed the check-circular-ownership branch from 2a3922b to 5ca9191 Compare February 1, 2022 00:43

lxfind merged commit bfcf1b9 into main Feb 1, 2022

lxfind deleted the check-circular-ownership branch February 1, 2022 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Object Ownership #4] Prevent circular ownership #295

[Object Ownership #4] Prevent circular ownership #295

lxfind commented Jan 28, 2022 •

edited

Loading

sblackshear Jan 29, 2022

lxfind Jan 29, 2022

sblackshear Jan 29, 2022

sblackshear Jan 29, 2022

lxfind Jan 29, 2022

huitseeker Jan 31, 2022

lxfind Jan 31, 2022

huitseeker Jan 31, 2022

lxfind Jan 31, 2022

[Object Ownership #4] Prevent circular ownership #295

[Object Ownership #4] Prevent circular ownership #295

Conversation

lxfind commented Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxfind commented Jan 28, 2022 •

edited

Loading