Skip to content

Commit

Permalink
Fix Bytes, Vec and String buffer ownership (#6142)
Browse files Browse the repository at this point in the history
## Description

This PR fixes a problem with `std::Bytes`, `std::Vec` and `std::String`
buffer ownership. It also fixes a problem with buffer overflow when
encoding huge types.

## Buffer Ownership

Currently, types like `Bytes`, `Vec` and `String` do not guarantee
ownership of their buffer. That means that we can very easily alias the
underlying buffer and change its mutability by simply creating a new
instance.

For example:

```sway
let some_slice = ...;
let buffer = Bytes::from(some_slice);
let mut buffer2 = buffer.clone();
// Now I can change `some_slice`
```

## Type Inference bug

I also had to fix a small problem with the type inference of method
applications. The problem is that sometimes the type check does not
return an error on the first pass, but the type is still not concrete.

For example `x.get(0) == Some(2)`, becomes `eq(x.get(0), Some(2))`.

After the first pass, `x.get(0)` is correctly inferred to be
`Option<u8>`; but `Some(2)` is only typed as `Option<Numeric>`. This
happens because the first pass starts with `TypeInfo::Unknown`. We can
use the argument types here because they may still be non-concrete types
like "Self".

What the fix is doing is that it checks if the inferred type
`is_concrete`, assuming that `Numeric` is not. This will make "Some(2)"
be type-checked again at the second pass, after monomorphization and be
correctly typed as "Option<u8>".

## IR Verification errors

This PR also improves the error message for IR verification. Now the
error is shown inside the printed IR.


![image](https://github.com/FuelLabs/sway/assets/83425/4f9eae39-0ce2-428d-be46-a215797ee8dd)

## Script to update contract-ids (optional)

At `test/update-contract-ids.sh` there is a small script that
automatically updates contract ids. Unfortunately, given the indirect
nature of contract calls, it is impossible for the script to know which
contract it needs to compile. So we need to specify the path to the
contract.

We also need to pass the compiler flags, because in some cases we use
`debug` profile, and in others we use `release`.

```sway
const FUEL_COIN_CONTRACT_ID = 0x1a88d0982d216958d18378b6784614b75868a542dc05f8cc85cf3da44268c76c; // AUTO-CONTRACT-ID ../../test_contracts/test_fuel_coin_contract --release
```

In the example above, the path and flags are appended in the bash
script. I failed trying to inject commands, so I think the append is
safe.

I can remove this from the PR, if we find that this is not the solution
we want at the moment.

## Checklist

- [ ] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.
  • Loading branch information
xunilrj committed Jul 8, 2024
1 parent ba46821 commit 090187e
Show file tree
Hide file tree
Showing 33 changed files with 644 additions and 210 deletions.
2 changes: 1 addition & 1 deletion forc-plugins/forc-client/tests/deploy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ async fn simple_deploy() {
node.kill().unwrap();
let expected = vec![DeployedContract {
id: ContractId::from_str(
"428896412bda8530282a7b8fca5d20b2a73f30037612ca3a31750cf3bf0e976a",
"822c8d3672471f64f14f326447793c7377b6e430122db23b622880ccbd8a33ef",
)
.unwrap(),
}];
Expand Down
218 changes: 218 additions & 0 deletions sway-core/src/ir_generation/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1717,6 +1717,224 @@ impl<'eng> FnCompiler<'eng> {
Ok(increase_len(&mut s.current_block, context, len, 8 - offset))
}

fn grow_if_needed(
s: &mut FnCompiler<'_>,
context: &mut Context,
ptr: Value,
cap: Value,
len: Value,
needed_size: Value,
) -> (Value, Value) {
assert!(ptr.get_type(context).unwrap().is_ptr(context));
assert!(cap.get_type(context).unwrap().is_uint64(context));

let ptr_u8 = Type::new_ptr(context, Type::get_uint8(context));

// merge block has two arguments: ptr, cap
let merge_block = s.function.create_block(context, None);
let merge_block_ptr = Value::new_argument(
context,
BlockArgument {
block: merge_block,
idx: 0,
ty: ptr_u8,
},
);
merge_block.add_arg(context, merge_block_ptr);
let merge_block_cap = Value::new_argument(
context,
BlockArgument {
block: merge_block,
idx: 1,
ty: Type::get_uint64(context),
},
);
merge_block.add_arg(context, merge_block_cap);

let true_block_begin = s.function.create_block(context, None);
let false_block_begin = s.function.create_block(context, None);

// if len + needed_size > cap
let needed_cap = s.current_block.append(context).binary_op(
BinaryOpKind::Add,
len,
needed_size,
);
let needs_realloc = s.current_block.append(context).cmp(
Predicate::GreaterThan,
needed_cap,
cap,
);
s.current_block.append(context).conditional_branch(
needs_realloc,
true_block_begin,
false_block_begin,
vec![],
vec![],
);

// needs realloc block
// new_cap = cap * 2
// aloc new_cap
// mcp hp old_ptr len
// hp: ptr u8
s.current_block = true_block_begin;
let u8 = Type::get_uint8(context);
let ptr_u8 = Type::new_ptr(context, u8);

let two = Constant::new_uint(context, 64, 2);
let two = Value::new_constant(context, two);
let new_cap =
s.current_block
.append(context)
.binary_op(BinaryOpKind::Mul, cap, two);

let new_ptr = s.current_block.append(context).asm_block(
vec![
AsmArg {
name: Ident::new_no_span("new_cap".into()),
initializer: Some(new_cap),
},
AsmArg {
name: Ident::new_no_span("old_ptr".into()),
initializer: Some(ptr),
},
AsmArg {
name: Ident::new_no_span("len".into()),
initializer: Some(len),
},
],
vec![
AsmInstruction {
op_name: Ident::new_no_span("aloc".into()),
args: vec![Ident::new_no_span("new_cap".into())],
immediate: None,
metadata: None,
},
AsmInstruction {
op_name: Ident::new_no_span("mcp".into()),
args: vec![
Ident::new_no_span("hp".into()),
Ident::new_no_span("old_ptr".into()),
Ident::new_no_span("len".into()),
],
immediate: None,
metadata: None,
},
],
ptr_u8,
Some(Ident::new_no_span("hp".into())),
);

s.current_block
.append(context)
.branch(merge_block, vec![new_ptr, new_cap]);

// dont need realloc block
s.current_block = false_block_begin;
s.current_block
.append(context)
.branch(merge_block, vec![ptr, cap]);

s.current_block = merge_block;

assert!(merge_block_ptr.get_type(context).unwrap().is_ptr(context));
assert!(merge_block_cap
.get_type(context)
.unwrap()
.is_uint64(context));

(merge_block_ptr, merge_block_cap)
}

fn to_constant(
_s: &mut FnCompiler<'_>,
context: &mut Context,
needed_size: u64,
) -> Value {
let needed_size = Constant::new_uint(context, 64, needed_size);
Value::new_constant(context, needed_size)
}

// Grow the buffer if needed
let (ptr, cap) = match &*item_type {
TypeInfo::Boolean => {
let needed_size = to_constant(self, context, 1);
grow_if_needed(self, context, ptr, cap, len, needed_size)
}
TypeInfo::UnsignedInteger(IntegerBits::Eight) => {
let needed_size = to_constant(self, context, 1);
grow_if_needed(self, context, ptr, cap, len, needed_size)
}
TypeInfo::UnsignedInteger(IntegerBits::Sixteen) => {
let needed_size = to_constant(self, context, 2);
grow_if_needed(self, context, ptr, cap, len, needed_size)
}
TypeInfo::UnsignedInteger(IntegerBits::ThirtyTwo) => {
let needed_size = to_constant(self, context, 4);
grow_if_needed(self, context, ptr, cap, len, needed_size)
}
TypeInfo::UnsignedInteger(IntegerBits::SixtyFour) => {
let needed_size = to_constant(self, context, 8);
grow_if_needed(self, context, ptr, cap, len, needed_size)
}
TypeInfo::UnsignedInteger(IntegerBits::V256) | TypeInfo::B256 => {
let needed_size = to_constant(self, context, 32);
grow_if_needed(self, context, ptr, cap, len, needed_size)
}
TypeInfo::StringArray(string_len) => {
let needed_size = to_constant(self, context, string_len.val() as u64);
grow_if_needed(self, context, ptr, cap, len, needed_size)
}
TypeInfo::StringSlice | TypeInfo::RawUntypedSlice => {
let uint64 = Type::get_uint64(context);
let u64_u64_type = Type::new_struct(context, vec![uint64, uint64]);

// convert "item" to { u64, u64 }
let item = self.current_block.append(context).asm_block(
vec![AsmArg {
name: Ident::new_no_span("item".into()),
initializer: Some(item),
}],
vec![],
u64_u64_type,
Some(Ident::new_no_span("item".into())),
);

// save item to local _anon
let name = self.lexical_map.insert_anon();
let item_local = self
.function
.new_local_var(context, name, u64_u64_type, None, false)
.map_err(|ir_error| {
CompileError::InternalOwned(ir_error.to_string(), Span::dummy())
})?;
let ptr_to_local_item =
self.current_block.append(context).get_local(item_local);
self.current_block
.append(context)
.store(ptr_to_local_item, item);

// _anon.1 = len
let needed_size = self.current_block.append(context).get_elem_ptr_with_idx(
ptr_to_local_item,
uint64,
1,
);
let needed_size = self.current_block.append(context).load(needed_size);
let eight = to_constant(self, context, 8);
let needed_size = self.current_block.append(context).binary_op(
BinaryOpKind::Add,
needed_size,
eight,
);

grow_if_needed(self, context, ptr, cap, len, needed_size)
}
_ => return Err(CompileError::EncodingUnsupportedType { span: item_span }),
};

// Append the value into the buffer
let new_len = match &*item_type {
TypeInfo::Boolean => {
assert!(item.get_type(context).unwrap().is_bool(context));
Expand Down
12 changes: 9 additions & 3 deletions sway-core/src/language/ty/declaration/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -565,9 +565,15 @@ impl TyFunctionSig {
}

pub fn is_concrete(&self, engines: &Engines) -> bool {
self.return_type.is_concrete(engines)
&& self.parameters.iter().all(|p| p.is_concrete(engines))
&& self.type_parameters.iter().all(|p| p.is_concrete(engines))
self.return_type.is_concrete(engines, IncludeNumeric::No)
&& self
.parameters
.iter()
.all(|p| p.is_concrete(engines, IncludeNumeric::No))
&& self
.type_parameters
.iter()
.all(|p| p.is_concrete(engines, IncludeNumeric::No))
}

/// Returns a String representing the function.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,21 +247,25 @@ fn type_check_encode_append(
};

// only supported types
match &*engines.te().get(item_type) {
TypeInfo::Boolean
| TypeInfo::UnsignedInteger(IntegerBits::Eight)
| TypeInfo::UnsignedInteger(IntegerBits::Sixteen)
| TypeInfo::UnsignedInteger(IntegerBits::ThirtyTwo)
| TypeInfo::UnsignedInteger(IntegerBits::SixtyFour)
| TypeInfo::UnsignedInteger(IntegerBits::V256)
| TypeInfo::B256
| TypeInfo::StringArray(_)
| TypeInfo::StringSlice
| TypeInfo::RawUntypedSlice => {}
_ => {
return Err(handler.emit_err(CompileError::EncodingUnsupportedType { span: item_span }))
}
};
if item_type.is_concrete(engines, IncludeNumeric::Yes) {
match &*engines.te().get(item_type) {
TypeInfo::Boolean
| TypeInfo::UnsignedInteger(IntegerBits::Eight)
| TypeInfo::UnsignedInteger(IntegerBits::Sixteen)
| TypeInfo::UnsignedInteger(IntegerBits::ThirtyTwo)
| TypeInfo::UnsignedInteger(IntegerBits::SixtyFour)
| TypeInfo::UnsignedInteger(IntegerBits::V256)
| TypeInfo::B256
| TypeInfo::StringArray(_)
| TypeInfo::StringSlice
| TypeInfo::RawUntypedSlice => {}
_ => {
return Err(
handler.emit_err(CompileError::EncodingUnsupportedType { span: item_span })
)
}
};
}

let kind = ty::TyIntrinsicFunctionKind {
kind,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,25 @@ pub(crate) fn type_check_method_application(
.by_ref()
.with_help_text("")
.with_type_annotation(type_engine.insert(engines, TypeInfo::Unknown, None));

// Ignore errors in method parameters
// On the second pass we will throw the errors if they persist.
let arg_handler = Handler::default();
let arg_opt = ty::TyExpression::type_check(&arg_handler, ctx, arg).ok();

// Check this type needs a second pass
let has_errors = arg_handler.has_errors();
let is_not_concrete = arg_opt
.as_ref()
.map(|x| {
x.return_type
.extract_inner_types(engines, IncludeSelf::Yes)
.iter()
.any(|x| !x.is_concrete(engines, IncludeNumeric::Yes))
})
.unwrap_or_default();
let needs_second_pass = has_errors || is_not_concrete;

if index == 0 {
// We want to emit errors in the self parameter and ignore TraitConstraintNotSatisfied with Placeholder
// which may be recoverable on the second pass.
Expand All @@ -66,7 +80,8 @@ pub(crate) fn type_check_method_application(
});
handler.append(arg_handler);
}
args_opt_buf.push_back((arg_opt, has_errors));

args_opt_buf.push_back((arg_opt, needs_second_pass));
}

// resolve the method name to a typed function declaration and type_check
Expand Down Expand Up @@ -115,6 +130,7 @@ pub(crate) fn type_check_method_application(
} else {
index
};

let ctx = if let Some(param) = method.parameters.get(param_index) {
// We now try to type check it again, this time with the type annotation.
ctx.by_ref()
Expand All @@ -127,6 +143,7 @@ pub(crate) fn type_check_method_application(
.with_help_text("")
.with_type_annotation(type_engine.insert(engines, TypeInfo::Unknown, None))
};

args_buf.push_back(
ty::TyExpression::type_check(handler, ctx, arg)
.unwrap_or_else(|err| ty::TyExpression::error(err, span.clone(), engines)),
Expand Down
22 changes: 18 additions & 4 deletions sway-core/src/type_system/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ pub enum IncludeSelf {
No,
}

pub enum IncludeNumeric {
Yes,
No,
}

/// A identifier to uniquely refer to our type terms
#[derive(PartialEq, Eq, Hash, Clone, Copy, Ord, PartialOrd, Debug)]
pub struct TypeId(usize);
Expand Down Expand Up @@ -456,17 +461,26 @@ impl TypeId {
}))
}

pub(crate) fn is_concrete(&self, engines: &Engines) -> bool {
pub(crate) fn is_concrete(&self, engines: &Engines, include_numeric: IncludeNumeric) -> bool {
let nested_types = (*self).extract_nested_types(engines);
!nested_types.into_iter().any(|x| {
matches!(
!nested_types.into_iter().any(|x| match include_numeric {
IncludeNumeric::Yes => matches!(
x,
TypeInfo::UnknownGeneric { .. }
| TypeInfo::Custom { .. }
| TypeInfo::Placeholder(..)
| TypeInfo::TraitType { .. }
| TypeInfo::TypeParam(..)
| TypeInfo::Numeric
),
IncludeNumeric::No => matches!(
x,
TypeInfo::UnknownGeneric { .. }
| TypeInfo::Custom { .. }
| TypeInfo::Placeholder(..)
| TypeInfo::TraitType { .. }
| TypeInfo::TypeParam(..)
)
),
})
}

Expand Down
2 changes: 1 addition & 1 deletion sway-core/src/type_system/priv_prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ pub use super::{
type_parameter::TypeParameter,
},
engine::TypeEngine,
id::{IncludeSelf, TypeId},
id::{IncludeNumeric, IncludeSelf, TypeId},
info::{AbiEncodeSizeHint, AbiName, TypeInfo, TypeSourceInfo},
};
Loading

0 comments on commit 090187e

Please sign in to comment.