Skip to content

Commit

Permalink
Prevent non-clone types from entering cloneable layer (#2834)
Browse files Browse the repository at this point in the history
## Motivation and Context
There is a bug in cloneable layer that allows non-clone types to enter

## Description
Remove `DerefMut` and resolve the consequences

## Testing
- new doc test
- existing orchestrator CI

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: John DiSanti <jdisanti@amazon.com>
  • Loading branch information
rcoh and jdisanti authored Jul 10, 2023
1 parent 7c9c283 commit 3a133b1
Showing 1 changed file with 25 additions and 14 deletions.
39 changes: 25 additions & 14 deletions rust-runtime/aws-smithy-types/src/config_bag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use std::borrow::Cow;
use std::fmt::{Debug, Formatter};
use std::iter::Rev;
use std::marker::PhantomData;
use std::ops::{Deref, DerefMut};
use std::ops::Deref;
use std::slice::Iter;
use std::sync::Arc;

Expand Down Expand Up @@ -64,6 +64,21 @@ impl<T: Default> Default for Value<T> {
///
/// While [`FrozenLayer`] is also cloneable, which is a shallow clone via `Arc`, `CloneableLayer`
/// performs a deep clone that newly allocates all the items stored in it.
///
/// Cloneable enforces that non clone items cannot be added
/// ```rust,compile_fail
/// use aws_smithy_types::config_bag::Storable;
/// use aws_smithy_types::config_bag::StoreReplace;
/// use aws_smithy_types::config_bag::CloneableLayer;
/// #[derive(Debug)]
/// struct MyNotCloneStruct;
///
/// impl Storable for MyNotCloneStruct {
/// type Storer = StoreReplace<MyNotCloneStruct>;
/// }
/// let mut layer = CloneableLayer::new("layer");
/// layer.store_put(MyNotCloneStruct);
/// ```
#[derive(Debug, Default)]
pub struct CloneableLayer(Layer);

Expand All @@ -75,12 +90,6 @@ impl Deref for CloneableLayer {
}
}

impl DerefMut for CloneableLayer {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}

impl Clone for CloneableLayer {
fn clone(&self) -> Self {
Self(
Expand Down Expand Up @@ -110,15 +119,16 @@ impl CloneableLayer {

/// Removes `T` from this bag
pub fn unset<T: Send + Sync + Clone + Debug + 'static>(&mut self) -> &mut Self {
self.put_directly::<StoreReplace<T>>(Value::ExplicitlyUnset(type_name::<T>()));
self.0.unset::<T>();
self
}

fn put_directly<T: Store>(&mut self, value: T::StoredType) -> &mut Self
fn put_directly_cloneable<T: Store>(&mut self, value: T::StoredType) -> &mut Self
where
T::StoredType: Clone,
{
self.props
self.0
.props
.insert(TypeId::of::<T>(), TypeErasedBox::new_with_clone(value));
self
}
Expand All @@ -128,7 +138,7 @@ impl CloneableLayer {
where
T: Storable<Storer = StoreReplace<T>> + Clone,
{
self.put_directly::<StoreReplace<T>>(Value::Set(item));
self.put_directly_cloneable::<StoreReplace<T>>(Value::Set(item));
self
}

Expand All @@ -142,7 +152,7 @@ impl CloneableLayer {
Some(item) => Value::Set(item),
None => Value::ExplicitlyUnset(type_name::<T>()),
};
self.put_directly::<StoreReplace<T>>(item);
self.put_directly_cloneable::<StoreReplace<T>>(item);
self
}

Expand All @@ -164,14 +174,15 @@ impl CloneableLayer {
where
T: Storable<Storer = StoreAppend<T>> + Clone,
{
self.put_directly::<StoreAppend<T>>(Value::ExplicitlyUnset(type_name::<T>()));
self.put_directly_cloneable::<StoreAppend<T>>(Value::ExplicitlyUnset(type_name::<T>()));
}

fn get_mut_or_default<T: Send + Sync + Store + 'static>(&mut self) -> &mut T::StoredType
where
T::StoredType: Default + Clone,
{
self.props
self.0
.props
.entry(TypeId::of::<T>())
.or_insert_with(|| TypeErasedBox::new_with_clone(T::StoredType::default()))
.downcast_mut()
Expand Down

0 comments on commit 3a133b1

Please sign in to comment.