disallow cpi#4
Conversation
samkim-crypto
left a comment
samkim-crypto
left a comment
There was a problem hiding this comment.
Looks good to me. Just a style suggestion and a clarifying question regarding makefile changes.
| let instruction_data = | ||
| core::slice::from_raw_parts(instruction_data_addr, instruction_data_len as usize); | ||
| processor::verify_secp256k1_instruction(instruction_data) | ||
| match processor::reject_cpi_stack_height(processor::current_stack_height()) { |
There was a problem hiding this comment.
This is a nit style issue, but this stack height check seems a bit more complicated than necessary. What do you think about adding an in_cpi function like here and just invoking in_cpi() in the beginning of the entrypoint?
| @if [ -f target/deploy/$(call make-so,$*).so ]; then \ | ||
| SBF_OUT_DIR=$(PWD)/target/deploy cargo test \ | ||
| --locked \ | ||
| --manifest-path $(call make-path,$*)/Cargo.toml \ | ||
| $(ARGS); \ | ||
| else \ | ||
| cargo test \ | ||
| --locked \ | ||
| --manifest-path $(call make-path,$*)/Cargo.toml \ | ||
| $(ARGS); \ | ||
| fi |
There was a problem hiding this comment.
Just for my own knowledge, are these changes in the makefile related to the logic of disallowing CPI?
There was a problem hiding this comment.
No, this is not related to the CPI.
CI failed after I merged main from a dirty branch, because make test-program requires cargo build-sbf installed. So this was to address the CI failure.
it is now splitted into a dedicated commit.
There was a problem hiding this comment.
Oh I see. It seems like we need this for mollusk. What do you think about simplifying it as follows?
test-%: build-sbf-%
SBF_OUT_DIR=$(PWD)/target/deploy cargo test \
--locked \
--manifest-path $(call make-path,$*)/Cargo.toml \
$(ARGS)
There was a problem hiding this comment.
hmmm did not work... turns out that we still need a fall back path for mollusk
| processor::verify_secp256k1_instruction(instruction_data) | ||
| } | ||
| if processor::in_cpi() { | ||
| Err(ProgramError::InvalidArgument) |
There was a problem hiding this comment.
what about doing something like:
if processor::in_cpi() {
return Err(ProgramError::InvalidArgument);
}
(... rest of the logic)
We can probably remove the unsafe here too since unsafe is wrapped into the in_cpi() function.
| @if [ -f target/deploy/$(call make-so,$*).so ]; then \ | ||
| SBF_OUT_DIR=$(PWD)/target/deploy cargo test \ | ||
| --locked \ | ||
| --manifest-path $(call make-path,$*)/Cargo.toml \ | ||
| $(ARGS); \ | ||
| else \ | ||
| cargo test \ | ||
| --locked \ | ||
| --manifest-path $(call make-path,$*)/Cargo.toml \ | ||
| $(ARGS); \ | ||
| fi |
There was a problem hiding this comment.
Oh I see. It seems like we need this for mollusk. What do you think about simplifying it as follows?
test-%: build-sbf-%
SBF_OUT_DIR=$(PWD)/target/deploy cargo test \
--locked \
--manifest-path $(call make-path,$*)/Cargo.toml \
$(ARGS)
samkim-crypto
left a comment
samkim-crypto
left a comment
There was a problem hiding this comment.
Okay, I think the entrypoint can be simplified a bit more, but since this will require some restructuring for abiv2 anyway, I think this is fine to merge. Thanks for the updates!
2 participants
this PR disallows this program to be called via CPI