[verifier] Added type-checking of templated type arguments for entry functions #469

awelc · 2022-02-17T05:26:23Z

This implements type-checking of templated type arguments for entry functions to avoid integrity violations described in issue #348.

There are some potentially debatable points there (e.g., the one I outlined in a comment), but I think this PR is at least ready for some discussion.

…functions

awelc · 2022-02-17T05:31:58Z

sui_programmability/verifier/tests/param_typecheck_verification_test.rs

+    /*
+    struct ObjStruct has key
+
+    public foo<Ty0>(loc0: ObjStruct, loc1: u64, loc2: Ty0, loc3: &mut TxContext) {


I wonder if we should consider a function with this type of signature as a verification error. My justification for not doing so is that type check in the adapter rejects functions that feature templated arguments (or templated vector arguments) in the "pure args" part of the signature, and this mirrors this decision by simply not considering such functions to be callable from Sui.

I think this is the right thing to do. It's totally legitimate to write a function like this, it just can't be an entrypoint.

Curious why can't this function be an entry point? If one calls it by instantiating Ty0 to u64, would it not work today?

There was a reason why I put this comment right after pushing the code, as it is indeed debatable what to do in this case.

Currently, unless someone implements their own client, I don't think this function is callable from Sui due to the following check in the adapter where is_primitive will return false if param type is Type::TypeParameter:

// check that the non-object parameters are primitive types for param_type in &function_signature.parameters[args.len()..function_signature.parameters.len() - 1] { if !is_primitive(param_type) { return Err(SuiError::TypeError { error: format!("Expected primitive type, but found {}", param_type), }); } }

The tradeoff here is how aggressively we wan the verifier to reject otherwise legit Move functions. The current implementation errs on the side of accepting legit Move functions, but the decision could certainly go the other way - treat such functions as potential entry functions and reject the in the verifier.

Please note that there is another subtlety here, I think, if we consider such functions candidate entry functions. In this case the following function will be rejected by the verifier as there is no guarantee that T will be instantiated with a primitive type.

public fun foo<T>(a: u64, b: T, _ctx: &mut TxContext)

One (the only?) way to make it accepted is to make T have the key ability:

public fun foo<T: Key>(a: u64, b: T, _ctx: &mut TxContext)

Does it make sense to have a "keyd" T type parameter in the primitive parameters part, though? I think that technically we could still instantiate it with a primitive type, but the presence of the key ability implies that T should represent an object, so should we treat the second version of the function as an entry candidate or not?

Ah I see. We don't allow templated primitive types when calling entry functions. I missed that part.

oxade · 2022-02-17T06:17:13Z

Nice work!...adding myself as a fly on the wall to understand if our data model from the client might need some changes.

lxfind · 2022-02-17T06:30:35Z

sui_programmability/verifier/src/param_typecheck_verifier.rs

+/// This checks if parameters of functions that can become entry
+/// functions (functions called directly from Sui) have correct types.


Some detailed comments explaining what exactly we are checking here would be good.

Added! Please let me know what you think.

lxfind · 2022-02-17T06:46:28Z

sui_programmability/verifier/src/param_typecheck_verifier.rs

+                // function as it would get rejected by the
+                // resolve_and_type_check function in the adapter upon its
+                // call attempt from Sui
+                return (false, obj_params_num);


What is the semantics of is_entry_candidate? Is it to detect whether this function could be an entry function, or is it to detect whether this could be an entry function that has the potential to violate integrity?

Consider this one:

public fun foo<T>(a: u64, b: T, _ctx: &mut TxContext)

If foo is called with T being a primitive type, it can certainly be an entry function, but it will fail here and return false.

It checks if the function can be an entry candidate without checking correctness of parameter types (added a comment to that effect).

The example you give is almost the exact one I mentioned in my comment - under current implementation this function would not be considered an entry function candidate, much like it would throw an validation error (I think) upon being called from Sui.

lxfind · 2022-02-17T06:47:45Z

sui_programmability/verifier/src/param_typecheck_verifier.rs

+    Ok(())
+}
+
+fn is_entry_candidate(view: &BinaryIndexedView, params: &Signature) -> (bool, usize) {


If the usize return value is only needed when the first bool is true, then this is perfect case to return Option<usize>.

Still wearing my Go hat I guess... Thank you!

lxfind · 2022-02-17T06:48:02Z

sui_programmability/verifier/src/param_typecheck_verifier.rs

+/// Checks if a given parameter is of ground type. It's a mirror of
+/// the is_primitive function in the adapter module that operates on
+/// Type-s.
+fn is_ground_type(p: &SignatureToken) -> bool {


Is there a reason we call it ground instead of primitive?

Not really, other than @sblackshear called them as such in one of our Slack conversations and I thought that's what I should use :-)

sblackshear · 2022-02-17T18:16:40Z

sui_programmability/adapter/src/unit_tests/adapter_tests.rs

@@ -924,6 +927,24 @@ fn publish_from_src(
    module_path.push(src_path);
    let modules = sui_framework::build_move_package(&module_path, build_config, false).unwrap();

+    let m = modules.get(0).unwrap().clone();
+
+    for f in &m.function_handles {


Do we want to keep this printing?

Certainly not! :-) Sorry it has sneaked in...

sblackshear · 2022-02-17T18:17:37Z

sui_programmability/verifier/src/param_typecheck_verifier.rs

+                if !is_template_param_ok(handle, p) {
+                    return Err(SuiError::ModuleVerificationFailure {
+                        error: format!(
+                            "No Key ability for the template parameter at position {} in entry function {}",


Suggested change

"No Key ability for the template parameter at position {} in entry function {}",

"No `key` ability for the template parameter at position {} in entry function {}",

awelc · 2022-02-17T21:11:21Z

Are there any additional concerns/suggestions or is it ready to land?

lxfind

LGTM! Some nit comments

lxfind · 2022-02-17T21:12:48Z

sui_programmability/verifier/src/param_typecheck_verifier.rs

+            continue;
+        }
+        let params = view.signature_at(handle.parameters);
+        let param_nums = match is_entry_candidate(&view, params) {


nit: rename to object_param_num?

lxfind · 2022-02-17T21:17:45Z

sui_programmability/verifier/src/param_typecheck_verifier.rs

+                module_name == TX_CONTEXT_MODULE_NAME
+                    && module_addr == &SUI_FRAMEWORK_ADDRESS
+                    && struct_name == TX_CONTEXT_STRUCT_NAME
+                    && struct_handle.type_parameters.is_empty()


type param check is not necessary I think. There is only one unique struct given address, module and struct name.

This function mirrors is_param_tx_context in adapter which has the same checks (but on the Type enum rather than SignatureToken).

I think it can be removed there too. On a side note, we are starting to have more duplicate code between adapter and verifier. Latter we probably should move some of the common code to a separate place.

[verifier] Added type-checking of templated type arguments for entry …

9c278de

…functions

awelc commented Feb 17, 2022

View reviewed changes

awelc self-assigned this Feb 17, 2022

awelc linked an issue Feb 17, 2022 that may be closed by this pull request

[Move] Properly allow templated type arguments in entry functions #348

Closed

awelc requested review from lxfind and sblackshear February 17, 2022 05:41

awelc marked this pull request as ready for review February 17, 2022 05:41

oxade self-requested a review February 17, 2022 06:15

lxfind reviewed Feb 17, 2022

View reviewed changes

sblackshear reviewed Feb 17, 2022

View reviewed changes

Addressed review comments

fd595f3

lxfind approved these changes Feb 17, 2022

View reviewed changes

awelc added 2 commits February 17, 2022 13:27

Addressed additional review comments

d2e992f

Removed unnecesary checks

0f0f1b2

awelc merged commit 070c4ff into main Feb 17, 2022

awelc deleted the aw/move-param-typecheck branch February 17, 2022 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[verifier] Added type-checking of templated type arguments for entry functions #469

[verifier] Added type-checking of templated type arguments for entry functions #469

awelc commented Feb 17, 2022 •

edited

Loading

awelc Feb 17, 2022 •

edited

Loading

sblackshear Feb 17, 2022

lxfind Feb 17, 2022

awelc Feb 17, 2022

lxfind Feb 17, 2022

oxade commented Feb 17, 2022

lxfind Feb 17, 2022

awelc Feb 17, 2022

lxfind Feb 17, 2022 •

edited

Loading

awelc Feb 17, 2022

lxfind Feb 17, 2022

awelc Feb 17, 2022

lxfind Feb 17, 2022

awelc Feb 17, 2022

sblackshear Feb 17, 2022

awelc Feb 17, 2022

sblackshear Feb 17, 2022

awelc Feb 17, 2022

awelc commented Feb 17, 2022

lxfind left a comment

lxfind Feb 17, 2022

awelc Feb 17, 2022

lxfind Feb 17, 2022

awelc Feb 17, 2022

lxfind Feb 17, 2022

awelc Feb 17, 2022

		/// This checks if parameters of functions that can become entry
		/// functions (functions called directly from Sui) have correct types.

	"No Key ability for the template parameter at position {} in entry function {}",
	"No `key` ability for the template parameter at position {} in entry function {}",

[verifier] Added type-checking of templated type arguments for entry functions #469

[verifier] Added type-checking of templated type arguments for entry functions #469

Conversation

awelc commented Feb 17, 2022 • edited Loading

awelc Feb 17, 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

oxade commented Feb 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxfind Feb 17, 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

Choose a reason for hiding this comment

awelc commented Feb 17, 2022

lxfind left a 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

awelc commented Feb 17, 2022 •

edited

Loading

awelc Feb 17, 2022 •

edited

Loading

lxfind Feb 17, 2022 •

edited

Loading