Skip to content

Commit

Permalink
reorder check back & rename attribute
Browse files Browse the repository at this point in the history
  • Loading branch information
georgemitenkov committed Mar 7, 2024
1 parent f0f08d6 commit aee7614
Show file tree
Hide file tree
Showing 12 changed files with 31 additions and 32 deletions.
5 changes: 2 additions & 3 deletions aptos-move/aptos-vm/src/aptos_vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::{
system_module_names::*,
transaction_metadata::TransactionMetadata,
transaction_validation, verifier,
verifier::randomness::is_entry_function_unbiasable,
verifier::randomness::has_randomness_attribute,
VMExecutor, VMValidator,
};
use anyhow::anyhow;
Expand Down Expand Up @@ -746,8 +746,7 @@ impl AptosVM {
entry_fn.ty_args(),
)?;

// TODO(George): change the order once performance is confirmed.
if is_entry_function_unbiasable(resolver, session, entry_fn)? && is_friend_or_private {
if is_friend_or_private && has_randomness_attribute(resolver, session, entry_fn)? {

Check warning on line 749 in aptos-move/aptos-vm/src/aptos_vm.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/aptos_vm.rs#L749

Added line #L749 was not covered by tests
let txn_context = session
.get_native_extensions()
.get_mut::<RandomnessContext>();
Expand Down
6 changes: 3 additions & 3 deletions aptos-move/aptos-vm/src/verifier/randomness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use crate::move_vm_ext::{AptosMoveResolver, SessionExt};
use aptos_types::transaction::EntryFunction;
use move_binary_format::errors::VMResult;

/// Returns true if function has an annotation that it is unbiasable.
pub(crate) fn is_entry_function_unbiasable(
/// Returns true if function has an attribute that it uses randomness.
pub(crate) fn has_randomness_attribute(
resolver: &impl AptosMoveResolver,
session: &mut SessionExt,
entry_fn: &EntryFunction,
Expand All @@ -19,7 +19,7 @@ pub(crate) fn is_entry_function_unbiasable(
Ok(metadata
.fun_attributes
.get(entry_fn.function().as_str())
.map(|attrs| attrs.iter().any(|attr| attr.is_unbiasable()))
.map(|attrs| attrs.iter().any(|attr| attr.is_randomness()))
.unwrap_or(false))

Check warning on line 23 in aptos-move/aptos-vm/src/verifier/randomness.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/verifier/randomness.rs#L9-L23

Added lines #L9 - L23 were not covered by tests
} else {
Ok(false)

Check warning on line 25 in aptos-move/aptos-vm/src/verifier/randomness.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/verifier/randomness.rs#L25

Added line #L25 was not covered by tests
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module 0x1::invalid {
#[unbiasable]
#[randomness]
public entry fun foo() {
// Do nothing.
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module 0x1::invalid {
#[unbiasable]
#[randomness]
public fun foo() {
// Do nothing.
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ module 0x1::test {
// Do nothing.
}

#[unbiasable]
#[randomness]
entry fun ok_if_annotated_and_not_using_randomness() {
// Do nothing.
}

#[unbiasable]
#[randomness]
entry fun ok_if_annotated_and_using_randomness() {
let _ = randomness::u64_integer();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module 0x1::some_randapp {
use aptos_framework::randomness;

#[unbiasable]
#[randomness]
entry fun safe_private_entry_call() {
let _ = randomness::u64_integer();
}
Expand All @@ -10,7 +10,7 @@ module 0x1::some_randapp {
let _ = randomness::u64_integer();
}

#[unbiasable]
#[randomness]
public(friend) entry fun safe_friend_entry_call() {
let _ = randomness::u64_integer();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module 0x1::some_randapp {
use aptos_framework::randomness;

#[unbiasable]
#[randomness]
entry fun safe_private_entry_call() {
let _ = randomness::u64_integer();
}
Expand All @@ -10,7 +10,7 @@ module 0x1::some_randapp {
let _ = randomness::u64_integer();
}

#[unbiasable]
#[randomness]
public(friend) entry fun safe_friend_entry_call() {
let _ = randomness::u64_integer();
}
Expand Down
4 changes: 2 additions & 2 deletions aptos-move/framework/aptos-framework/doc/randomness.md
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ Event emitted every time a public randomness API in this module is called.
<a id="0x1_randomness_E_API_USE_SUSCEPTIBLE_TO_TEST_AND_ABORT"></a>

Randomness APIs calls must originate from a private entry function with
<code>#[unbiasable]</code> annotation. Otherwise, test-and-abort attacks are possible.
<code>#[<a href="randomness.md#0x1_randomness">randomness</a>]</code> annotation. Otherwise, test-and-abort attacks are possible.


<pre><code><b>const</b> <a href="randomness.md#0x1_randomness_E_API_USE_SUSCEPTIBLE_TO_TEST_AND_ABORT">E_API_USE_SUSCEPTIBLE_TO_TEST_AND_ABORT</a>: u64 = 1;
Expand Down Expand Up @@ -973,7 +973,7 @@ Aborts with <code><a href="randomness.md#0x1_randomness_E_API_USE_SUSCEPTIBLE_TO
Called in each randomness generation function to ensure certain safety invariants, namely:
1. The transaction that led to the call of this function had a private (or friend) entry
function as its payload.
2. The entry function had <code>#[unbiasable]</code> annotation.
2. The entry function had <code>#[<a href="randomness.md#0x1_randomness">randomness</a>]</code> annotation.


<pre><code><b>fun</b> <a href="randomness.md#0x1_randomness_is_unbiasable">is_unbiasable</a>(): bool
Expand Down
4 changes: 2 additions & 2 deletions aptos-move/framework/aptos-framework/sources/randomness.move
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ module aptos_framework::randomness {
const DST: vector<u8> = b"APTOS_RANDOMNESS";

/// Randomness APIs calls must originate from a private entry function with
/// `#[unbiasable]` annotation. Otherwise, test-and-abort attacks are possible.
/// `#[randomness]` annotation. Otherwise, test-and-abort attacks are possible.
const E_API_USE_SUSCEPTIBLE_TO_TEST_AND_ABORT: u64 = 1;

const MAX_U256: u256 = 115792089237316195423570985008687907853269984665640564039457584007913129639935;
Expand Down Expand Up @@ -384,7 +384,7 @@ module aptos_framework::randomness {
/// Called in each randomness generation function to ensure certain safety invariants, namely:
/// 1. The transaction that led to the call of this function had a private (or friend) entry
/// function as its payload.
/// 2. The entry function had `#[unbiasable]` annotation.
/// 2. The entry function had `#[randomness]` annotation.
native fun is_unbiasable(): bool;

#[test]
Expand Down
12 changes: 6 additions & 6 deletions aptos-move/framework/src/extended_checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ const INIT_MODULE_FUN: &str = "init_module";
const LEGACY_ENTRY_FUN_ATTRIBUTE: &str = "legacy_entry_fun";
const ERROR_PREFIX: &str = "E";
const EVENT_STRUCT_ATTRIBUTE: &str = "event";
const RANDOMNESS_ATTRIBUTE: &str = "randomness";
const RESOURCE_GROUP: &str = "resource_group";
const RESOURCE_GROUP_MEMBER: &str = "resource_group_member";
const RESOURCE_GROUP_NAME: &str = "group";
const RESOURCE_GROUP_SCOPE: &str = "scope";
const UNBIASABLE_ATTRIBUTE: &str = "unbiasable";
const VIEW_FUN_ATTRIBUTE: &str = "view";

// top-level attribute names, only.
Expand All @@ -52,7 +52,7 @@ pub fn get_all_attribute_names() -> &'static BTreeSet<String> {
RESOURCE_GROUP_MEMBER,
VIEW_FUN_ATTRIBUTE,
EVENT_STRUCT_ATTRIBUTE,
UNBIASABLE_ATTRIBUTE,
RANDOMNESS_ATTRIBUTE,
];

fn extended_attribute_names() -> BTreeSet<String> {
Expand Down Expand Up @@ -460,26 +460,26 @@ impl<'a> ExtendedChecker<'a> {
impl<'a> ExtendedChecker<'a> {
fn check_and_record_unbiasabale_entry_functions(&mut self, module: &ModuleEnv) {
for ref fun in module.get_functions() {
if !self.has_attribute(fun, UNBIASABLE_ATTRIBUTE) {
if !self.has_attribute(fun, RANDOMNESS_ATTRIBUTE) {
continue;
}

if !fun.is_entry() || fun.visibility().is_public() {
self.env.error(
&fun.get_id_loc(),
"only private or public(friend) entry functions can have #[unbiasable] attribute",
"only private or public(friend) entry functions can have #[randomness] attribute",
)
}

Check warning on line 472 in aptos-move/framework/src/extended_checks.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/framework/src/extended_checks.rs#L465-L472

Added lines #L465 - L472 were not covered by tests

// Record functions which are unbiasable.
// Record functions which use randomness.
let module_id = self.get_runtime_module_id(module);
self.output
.entry(module_id)
.or_default()
.fun_attributes
.entry(fun.get_simple_name_string().to_string())
.or_default()
.push(KnownAttribute::unbiasable());
.push(KnownAttribute::randomness());

Check warning on line 482 in aptos-move/framework/src/extended_checks.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/framework/src/extended_checks.rs#L475-L482

Added lines #L475 - L482 were not covered by tests
}
}
}
Expand Down
14 changes: 7 additions & 7 deletions aptos-move/framework/src/module_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ pub enum KnownAttributeKind {
ResourceGroup = 2,
ResourceGroupMember = 3,
Event = 4,
Unbiasable = 5,
Randomness = 5,
}

impl KnownAttribute {
Expand Down Expand Up @@ -149,15 +149,15 @@ impl KnownAttribute {
self.kind == KnownAttributeKind::Event as u8
}

pub fn unbiasable() -> Self {
pub fn randomness() -> Self {
Self {
kind: KnownAttributeKind::Unbiasable as u8,
kind: KnownAttributeKind::Randomness as u8,
args: vec![],
}
}

Check warning on line 157 in aptos-move/framework/src/module_metadata.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/framework/src/module_metadata.rs#L152-L157

Added lines #L152 - L157 were not covered by tests

pub fn is_unbiasable(&self) -> bool {
self.kind == KnownAttributeKind::Unbiasable as u8
pub fn is_randomness(&self) -> bool {
self.kind == KnownAttributeKind::Randomness as u8
}

Check warning on line 161 in aptos-move/framework/src/module_metadata.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/framework/src/module_metadata.rs#L159-L161

Added lines #L159 - L161 were not covered by tests
}

Expand Down Expand Up @@ -359,7 +359,7 @@ pub fn is_valid_unbiasable_function(

Err(AttributeValidationError {
key: fun.to_string(),
attribute: KnownAttributeKind::Unbiasable as u8,
attribute: KnownAttributeKind::Randomness as u8,
})
}

Check warning on line 364 in aptos-move/framework/src/module_metadata.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/framework/src/module_metadata.rs#L360-L364

Added lines #L360 - L364 were not covered by tests

Expand Down Expand Up @@ -448,7 +448,7 @@ pub fn verify_module_metadata(
for attr in attrs {
if attr.is_view_function() {
is_valid_view_function(&functions, fun)?;
} else if attr.is_unbiasable() {
} else if attr.is_randomness() {
is_valid_unbiasable_function(&functions, fun)?;

Check warning on line 452 in aptos-move/framework/src/module_metadata.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/framework/src/module_metadata.rs#L450-L452

Added lines #L450 - L452 were not covered by tests
} else {
return Err(AttributeValidationError {
Expand Down
2 changes: 1 addition & 1 deletion aptos-move/framework/src/natives/randomness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub struct RandomnessContext {
// blob is requested.
txn_local_state: Vec<u8>,
// True if the current transaction's payload was a public(friend) or
// private entry function, which also had `#[unbiasable]` annotation.
// private entry function, which also has `#[randomness]` annotation.
unbiasable: bool,
}

Expand Down

0 comments on commit aee7614

Please sign in to comment.