From 55729c968f70ed29de128f80f8cc515cee3d3e74 Mon Sep 17 00:00:00 2001 From: Andre-Philippe Paquet Date: Fri, 11 Sep 2020 09:44:24 -0400 Subject: [PATCH 1/4] Atomic read limiter --- capnp/src/private/arena.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/capnp/src/private/arena.rs b/capnp/src/private/arena.rs index 73a0ece5e..e5dc5fdfb 100644 --- a/capnp/src/private/arena.rs +++ b/capnp/src/private/arena.rs @@ -19,7 +19,8 @@ // THE SOFTWARE. use alloc::vec::Vec; -use core::cell::{Cell, RefCell}; +use core::cell::RefCell; +use core::sync::atomic::{AtomicU64, Ordering}; use core::slice; use core::u64; @@ -31,21 +32,25 @@ use crate::{Error, OutputSegments, Result}; pub type SegmentId = u32; pub struct ReadLimiter { - pub limit: Cell, + pub limit: u64, + pub read: AtomicU64, } impl ReadLimiter { pub fn new(limit: u64) -> ReadLimiter { - ReadLimiter { limit: Cell::new(limit) } + ReadLimiter { + limit, + read: AtomicU64::new(0), + } } #[inline] pub fn can_read(&self, amount: u64) -> Result<()> { - let current = self.limit.get(); - if amount > current { + let read = self.read.fetch_add(amount, Ordering::Relaxed) + amount; + + if read > self.limit { Err(Error::failed(format!("read limit exceeded"))) } else { - self.limit.set(current - amount); Ok(()) } } From 04b21106c97da1d46983a5c24c4a9e9b14bd7603 Mon Sep 17 00:00:00 2001 From: Andre-Philippe Paquet Date: Mon, 21 Sep 2020 20:26:40 -0400 Subject: [PATCH 2/4] Use AtomicUsize for read limiter + feature gate it --- capnp/Cargo.toml | 7 ++- capnp/src/private/arena.rs | 31 +---------- capnp/src/private/mod.rs | 1 + capnp/src/private/read_limiter.rs | 92 +++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 29 deletions(-) create mode 100644 capnp/src/private/read_limiter.rs diff --git a/capnp/Cargo.toml b/capnp/Cargo.toml index 36629f1ba..4c790c926 100644 --- a/capnp/Cargo.toml +++ b/capnp/Cargo.toml @@ -25,7 +25,7 @@ quickcheck = { version = "0.9", optional = true } quickcheck = "0.9" [features] -default = ["std"] +default = ["std", "sync_reader"] rpc_try = [] @@ -37,3 +37,8 @@ unaligned = [] # with the Rust standard library. std = [] +# If disabled, message reader will not be `Sync`. This can be used +# to support platforms where `AtomicUsize` is not available. +# To be replaced by #[cfg(target_has_atomic = ...)] once it lands. +# See: https://github.com/rust-lang/rust/issues/32976 +sync_reader = [] \ No newline at end of file diff --git a/capnp/src/private/arena.rs b/capnp/src/private/arena.rs index e5dc5fdfb..2071ec30c 100644 --- a/capnp/src/private/arena.rs +++ b/capnp/src/private/arena.rs @@ -20,42 +20,17 @@ use alloc::vec::Vec; use core::cell::RefCell; -use core::sync::atomic::{AtomicU64, Ordering}; use core::slice; use core::u64; use crate::private::units::*; +use crate::private::read_limiter::ReadLimiter; use crate::message; use crate::message::{Allocator, ReaderSegments}; use crate::{Error, OutputSegments, Result}; pub type SegmentId = u32; -pub struct ReadLimiter { - pub limit: u64, - pub read: AtomicU64, -} - -impl ReadLimiter { - pub fn new(limit: u64) -> ReadLimiter { - ReadLimiter { - limit, - read: AtomicU64::new(0), - } - } - - #[inline] - pub fn can_read(&self, amount: u64) -> Result<()> { - let read = self.read.fetch_add(amount, Ordering::Relaxed) + amount; - - if read > self.limit { - Err(Error::failed(format!("read limit exceeded"))) - } else { - Ok(()) - } - } -} - pub trait ReaderArena { // return pointer to start of segment, and number of words in that segment fn get_segment(&self, id: u32) -> Result<(*const u8, u32)>; @@ -134,12 +109,12 @@ impl ReaderArena for ReaderArenaImpl where S: ReaderSegments { if !(start >= this_start && start - this_start + size <= this_size) { Err(Error::failed(format!("message contained out-of-bounds pointer"))) } else { - self.read_limiter.can_read(size_in_words as u64) + self.read_limiter.can_read(size_in_words) } } fn amplified_read(&self, virtual_amount: u64) -> Result<()> { - self.read_limiter.can_read(virtual_amount) + self.read_limiter.can_read(virtual_amount as usize) } } diff --git a/capnp/src/private/mod.rs b/capnp/src/private/mod.rs index 8d6465031..252fc19d1 100644 --- a/capnp/src/private/mod.rs +++ b/capnp/src/private/mod.rs @@ -29,6 +29,7 @@ mod primitive; pub mod layout; mod mask; pub mod units; +mod read_limiter; mod zero; #[cfg(test)] diff --git a/capnp/src/private/read_limiter.rs b/capnp/src/private/read_limiter.rs new file mode 100644 index 000000000..c47784d87 --- /dev/null +++ b/capnp/src/private/read_limiter.rs @@ -0,0 +1,92 @@ +// Copyright (c) 2013-2015 Sandstorm Development Group, Inc. and contributors +// Licensed under the MIT License: +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +#[cfg(feature = "sync_reader")] +pub use sync::ReadLimiter; + +#[cfg(feature = "sync_reader")] +mod sync { + use crate::{Error, Result}; + use core::sync::atomic::{AtomicUsize, Ordering}; + + pub struct ReadLimiter { + pub limit: usize, + pub read: AtomicUsize, + } + + impl ReadLimiter { + pub fn new(limit: u64) -> ReadLimiter { + if limit > core::usize::MAX as u64 { + panic!("traversal_limit_in_words cannot be bigger than core::usize::MAX") + } + + ReadLimiter { + limit: limit as usize, + read: AtomicUsize::new(0), + } + } + + #[inline] + pub fn can_read(&self, amount: usize) -> Result<()> { + let read = self.read.load(Ordering::Relaxed) + amount; + + if read > self.limit { + Err(Error::failed(format!("read limit exceeded"))) + } else { + self.read.fetch_add(amount, Ordering::Relaxed); + Ok(()) + } + } + } +} + +#[cfg(not(feature = "sync_reader"))] +pub use unsync::ReadLimiter; + +#[cfg(not(feature = "sync_reader"))] +mod unsync { + use core::cell::Cell; + use crate::{Error, Result}; + + pub struct ReadLimiter { + pub limit: Cell, + } + + impl ReadLimiter { + pub fn new(limit: u64) -> ReadLimiter { + ReadLimiter { + limit: Cell::new(limit), + } + } + + #[inline] + pub fn can_read(&self, amount: usize) -> Result<()> { + let amount = amount as u64; + let current = self.limit.get(); + if amount > current { + Err(Error::failed(format!("read limit exceeded"))) + } else { + self.limit.set(current - amount); + Ok(()) + } + } + } +} From f6c86c75c82ad653fa79f79ddea1262257af90bb Mon Sep 17 00:00:00 2001 From: Andre-Philippe Paquet Date: Thu, 26 Nov 2020 21:02:46 -0500 Subject: [PATCH 3/4] Atomic read limiter: better logic --- capnp/Cargo.toml | 5 ++--- capnp/src/private/read_limiter.rs | 30 ++++++++++++++++++------------ 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/capnp/Cargo.toml b/capnp/Cargo.toml index 4c790c926..1512c040f 100644 --- a/capnp/Cargo.toml +++ b/capnp/Cargo.toml @@ -25,7 +25,7 @@ quickcheck = { version = "0.9", optional = true } quickcheck = "0.9" [features] -default = ["std", "sync_reader"] +default = ["std"] rpc_try = [] @@ -37,8 +37,7 @@ unaligned = [] # with the Rust standard library. std = [] -# If disabled, message reader will not be `Sync`. This can be used -# to support platforms where `AtomicUsize` is not available. +# If enabled, message reader will be `Sync`. # To be replaced by #[cfg(target_has_atomic = ...)] once it lands. # See: https://github.com/rust-lang/rust/issues/32976 sync_reader = [] \ No newline at end of file diff --git a/capnp/src/private/read_limiter.rs b/capnp/src/private/read_limiter.rs index c47784d87..c0bf2b34d 100644 --- a/capnp/src/private/read_limiter.rs +++ b/capnp/src/private/read_limiter.rs @@ -25,11 +25,11 @@ pub use sync::ReadLimiter; #[cfg(feature = "sync_reader")] mod sync { use crate::{Error, Result}; - use core::sync::atomic::{AtomicUsize, Ordering}; + use core::sync::atomic::{AtomicUsize, AtomicBool, Ordering}; pub struct ReadLimiter { - pub limit: usize, - pub read: AtomicUsize, + pub limit: AtomicUsize, + pub limit_reached: AtomicBool, } impl ReadLimiter { @@ -39,21 +39,27 @@ mod sync { } ReadLimiter { - limit: limit as usize, - read: AtomicUsize::new(0), + limit: AtomicUsize::new(limit as usize), + limit_reached: AtomicBool::new(false), } } #[inline] pub fn can_read(&self, amount: usize) -> Result<()> { - let read = self.read.load(Ordering::Relaxed) + amount; + let limit_reached = self.limit_reached.load(Ordering::Relaxed); + if limit_reached { + return Err(Error::failed(format!("read limit exceeded"))); + } - if read > self.limit { - Err(Error::failed(format!("read limit exceeded"))) - } else { - self.read.fetch_add(amount, Ordering::Relaxed); - Ok(()) + let prev_limit = self.limit.fetch_sub(amount, Ordering::Relaxed); + if prev_limit == amount { + self.limit_reached.store(true, Ordering::Relaxed); + } else if prev_limit < amount { + self.limit_reached.store(true, Ordering::Relaxed); + return Err(Error::failed(format!("read limit exceeded"))); } + + Ok(()) } } } @@ -63,8 +69,8 @@ pub use unsync::ReadLimiter; #[cfg(not(feature = "sync_reader"))] mod unsync { - use core::cell::Cell; use crate::{Error, Result}; + use core::cell::Cell; pub struct ReadLimiter { pub limit: Cell, From 25f9948387a5991cc99b4a93971c900c8ad7a33d Mon Sep 17 00:00:00 2001 From: Andre-Philippe Paquet Date: Sat, 28 Nov 2020 09:52:19 -0500 Subject: [PATCH 4/4] Atomic read limiter: better logic --- capnp/src/private/read_limiter.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/capnp/src/private/read_limiter.rs b/capnp/src/private/read_limiter.rs index c0bf2b34d..7522869a5 100644 --- a/capnp/src/private/read_limiter.rs +++ b/capnp/src/private/read_limiter.rs @@ -25,11 +25,10 @@ pub use sync::ReadLimiter; #[cfg(feature = "sync_reader")] mod sync { use crate::{Error, Result}; - use core::sync::atomic::{AtomicUsize, AtomicBool, Ordering}; + use core::sync::atomic::{AtomicUsize, Ordering}; pub struct ReadLimiter { pub limit: AtomicUsize, - pub limit_reached: AtomicBool, } impl ReadLimiter { @@ -40,22 +39,21 @@ mod sync { ReadLimiter { limit: AtomicUsize::new(limit as usize), - limit_reached: AtomicBool::new(false), } } #[inline] pub fn can_read(&self, amount: usize) -> Result<()> { - let limit_reached = self.limit_reached.load(Ordering::Relaxed); - if limit_reached { + let cur_limit = self.limit.load(Ordering::Relaxed); + if cur_limit < amount { return Err(Error::failed(format!("read limit exceeded"))); } let prev_limit = self.limit.fetch_sub(amount, Ordering::Relaxed); - if prev_limit == amount { - self.limit_reached.store(true, Ordering::Relaxed); - } else if prev_limit < amount { - self.limit_reached.store(true, Ordering::Relaxed); + if prev_limit < amount { + // if the previous limit was lower than the amount we read, the limit has underflowed + // and wrapped around so we need to reset it to 0 for next reader to fail + self.limit.store(0, Ordering::Relaxed); return Err(Error::failed(format!("read limit exceeded"))); }