Update indices of functions in name section while injecting gas function #146

clearloop · 2021-01-06T03:13:50Z

Desc

Here we forgetting to update the indices in the name section

src/gas/mod.rs

pepyakin · 2021-01-06T13:48:07Z

I am curious, why do you need this?
I am asking because this part of code is certainly sensitive since it's used in on-chain logic, and on the other hand, we strive to avoid dealing with the names section since it's tricky (and perhaps even not sound in the current impl).

clearloop · 2021-01-06T15:45:13Z

Ref to use-ink/cargo-contract#131, paritytech/parity-wasm#300 and wasmi-labs/wasmi@master...patractlabs:v0.6.2

I am curious, why do you need this?
I am asking because this part of code is certainly sensitive since it's used in on-chain logic, and on the other hand, we strive to avoid dealing with the names section since it's tricky (and perhaps even not sound in the current impl).

We have implemented wasm backtrace for contracts in @patractlabs/europa, for example, panic in the deploy function of flipper, the result is like:

Trap(
    Trap {
        kind: Unreachable,
        wasm_trace: [
            "deploy[17]",
            "<unknown>[27]",
        ],
    },
)

And there will be more backtraces if we enable the debug build.

src/gas/mod.rs

pepyakin · 2021-01-06T16:02:15Z

Just to err on the safe side, do you think it's possible to demarcate the execution for debugging and on-chain use-cases? FWIW, I am not so keen to see a complete reallocation and rebuilding of the name section during (already heavy-ish) contract preparation step

Also, @athei what do you think about this? Does this concern Seal? Do name sections ever pass thru the preparation/put code stage, and does gas injection observe it?

athei · 2021-01-06T16:34:30Z

Also, @athei what do you think about this? Does this concern Seal? Do name sections ever pass thru the preparation/put code stage, and does gas injection observe it?

There is no exhaustive matching on the sections. Any unknown section will just pass through to gas injection and hence will be affected by this code. Maybe we should change that to deny deployment if any unknown section (like name section) is encountered.

src/gas/mod.rs

athei

We can't add more allocations to on-chain critical pathes.

clearloop · 2021-01-06T17:20:29Z

There is no exhaustive matching on the sections. Any unknown section will just pass through to gas injection and hence will be affected by this code. Maybe we should change that to deny deployment if any unknown section (like name section) is encountered.

It would be nice to add a field in the runtime Config for enabling or not for the name section(debug contracts), people write contracts, why we deny them debug their contracts using the name sections?

athei · 2021-01-07T11:01:09Z

It is just a security precaution to not have sections in wasm-blobs on-chain which we do not make use of. So why should we allow deploying this section when it is currently unused?

athei · 2021-01-07T13:27:14Z

I would be fine with merging if you add a process_name_section: ProcessNames parameter to inject_gas_counter with a big fat warning that this should never ever set to true whenever untrusted code is processed:

enum ProcessNames {
    No,
    UnsafeYes,
}

…njecting gas counter

atenjin · 2021-01-08T03:57:08Z

@athei @pepyakin
Hi, we(PatractLabs) is developing europa which is a tool to debug pallet-contracts. So when run a contract and panic in WASM, in europa the WASM trace could be printed to show where the code meet the panic. And in future, we would try to add WASM debug in Europa env, and could use gdb or lldb to debug contract in a chain runtime env. Therefore we need name section to bring in the function symbol for source file. And europa v0.2 report for proposal will be summitted next week, guys could see what we do, it's a nice tool to location the bug in code.

though ink! provides a whole offchain env to run test case to debug code, but this just a could debug in one contract. Like current defi in ETH, the real useful thing is the contract could call another contract and combine together, so that europa is a good tool to debug crossing different contracts.

And on the other hand, wasm-utils is a common lib to provide some useful tools to modify WASM, I think in this level, we should not assume the below level (like pallet-contracts, or someone create another contract runtime module) whether needs the name section. Whether should deny name section is depends on the below level, for example, pallet-contracts should check whether needs name section, and in our forked version pallet-contracts, we could ignore this check.
And in another way, I think this feature could be provided by conditional compilation, like we could design this as a features #[name_section_support] or some way.

By the way, currently, we implement this in our forked wasm-utils repo, and used in our forked pallet-contracts:
https://github.com/patractlabs/substrate/blob/europa-contracts/frame/contracts/Cargo.toml#L35 .
If you guys think you do not need this feature, it doesn't matter, do not affect our developing. We create this pr just think you maybe need this feature, or this feature could be used by other people. Thanks!

…as_counter

athei

The inject_gas_counter never calls parse_names. This means the name section is not there unless the caller did call this function. That should be at least documented with a big note.

The tests must be extended to include a name section. This new functionality is completely untested as it stands. This should be easily achieve by adding a name section to the fixtures.

athei · 2021-01-08T09:10:57Z

src/gas/mod.rs

+/// If process name section while injecting gas counter
+pub enum ProcessNames {
+	No,
+	UnsafeYes,
+}
+


Would be nice to have some more comprehensive documentation for the enum variants. This was the whole point for having it.

athei · 2021-01-08T09:24:39Z

src/gas/mod.rs

@@ -601,7 +629,7 @@ mod tests {
 				.build()
 			.build();

-		let injected_module = inject_gas_counter(module, &rules::Set::default(), "env").unwrap();
+		let injected_module = inject_gas_counter(module, &rules::Set::default(), "env", ProcessNames::No).unwrap();


Why disable? Overlong line.

athei · 2021-01-08T09:24:52Z

src/gas/mod.rs

@@ -652,7 +680,7 @@ mod tests {
 				.build()
 			.build();

-		let injected_module = inject_gas_counter(module, &rules::Set::default(), "env").unwrap();
+		let injected_module = inject_gas_counter(module, &rules::Set::default(), "env", ProcessNames::No).unwrap();


Overlong line.

athei · 2021-01-08T09:24:59Z

src/gas/mod.rs

@@ -721,7 +749,7 @@ mod tests {
 				let input_module = parse_wat($input);
 				let expected_module = parse_wat($expected);

-				let injected_module = inject_gas_counter(input_module, &rules::Set::default(), "env")
+				let injected_module = inject_gas_counter(input_module, &rules::Set::default(), "env", ProcessNames::No)


Overlong line.

athei · 2021-01-08T09:25:55Z

tests/diff.rs

@@ -110,7 +110,7 @@ mod gas {
 					let rules = utils::rules::Set::default();

 					let module = elements::deserialize_buffer(input).expect("Failed to deserialize");
-					let instrumented = utils::inject_gas_counter(module, &rules, "env")
+					let instrumented = utils::inject_gas_counter(module, &rules, "env", utils::ProcessNames::No)


You should add a function name to the fixtures and enable it here. Also you may need to call parse_names on the module.

athei · 2021-04-21T12:15:07Z

Closed for lack of activity.

clearloop added 2 commits December 29, 2020 10:33

feat(gas): update index of name section while injecting gas function

04ace63

fix(gas): add import of string in gas functions

371c439

pepyakin reviewed Jan 6, 2021

View reviewed changes

src/gas/mod.rs Outdated Show resolved Hide resolved

pepyakin reviewed Jan 6, 2021

View reviewed changes

src/gas/mod.rs Outdated Show resolved Hide resolved

perf(tabify): use tab and fix the comparison of gas function

bdf7b86

athei reviewed Jan 6, 2021

View reviewed changes

src/gas/mod.rs Outdated Show resolved Hide resolved

athei suggested changes Jan 6, 2021

View reviewed changes

feat(gas): Add enum process_names for processing name section while i…

783243c

…njecting gas counter

clearloop added 2 commits January 8, 2021 12:12

fix(gas): the related tests after add param process_names to inject_g…

7fcfa5c

…as_counter

fix(gas): the related modification of inject_gas_counter in main.rs

1cc87d1

athei suggested changes Jan 8, 2021

View reviewed changes

athei closed this Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update indices of functions in name section while injecting gas function #146

Update indices of functions in name section while injecting gas function #146

clearloop commented Jan 6, 2021

pepyakin commented Jan 6, 2021

clearloop commented Jan 6, 2021 •

edited

Loading

pepyakin commented Jan 6, 2021

athei commented Jan 6, 2021

athei left a comment

clearloop commented Jan 6, 2021

athei commented Jan 7, 2021

athei commented Jan 7, 2021

atenjin commented Jan 8, 2021 •

edited

Loading

athei left a comment

athei Jan 8, 2021

athei Jan 8, 2021

athei Jan 8, 2021

athei Jan 8, 2021

athei Jan 8, 2021

athei commented Apr 21, 2021

Update indices of functions in name section while injecting gas function #146

Update indices of functions in name section while injecting gas function #146

Conversation

clearloop commented Jan 6, 2021

Desc

pepyakin commented Jan 6, 2021

clearloop commented Jan 6, 2021 • edited Loading

pepyakin commented Jan 6, 2021

athei commented Jan 6, 2021

athei left a comment

Choose a reason for hiding this comment

clearloop commented Jan 6, 2021

athei commented Jan 7, 2021

athei commented Jan 7, 2021

atenjin commented Jan 8, 2021 • edited Loading

athei left a comment

Choose a reason for hiding this comment

athei Jan 8, 2021

Choose a reason for hiding this comment

athei Jan 8, 2021

Choose a reason for hiding this comment

athei Jan 8, 2021

Choose a reason for hiding this comment

athei Jan 8, 2021

Choose a reason for hiding this comment

athei Jan 8, 2021

Choose a reason for hiding this comment

athei commented Apr 21, 2021

clearloop commented Jan 6, 2021 •

edited

Loading

atenjin commented Jan 8, 2021 •

edited

Loading