From 82d7977672f4f691722b30708f35c6aee9f47e14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sun, 19 Nov 2023 20:24:32 -0500 Subject: [PATCH] style(cliprdr): clarify invariants --- STYLE.md | 225 ++++++++++++++++++++ crates/ironrdp-cliprdr-format/README.md | 9 +- crates/ironrdp-cliprdr-format/src/bitmap.rs | 89 ++++---- crates/ironrdp-cliprdr-format/src/html.rs | 36 ++-- 4 files changed, 296 insertions(+), 63 deletions(-) diff --git a/STYLE.md b/STYLE.md index 58e05d4c..5a3a91b6 100644 --- a/STYLE.md +++ b/STYLE.md @@ -287,6 +287,231 @@ If the line is too long, you want to split the sentence in two. [asciidoctor-practices]: https://asciidoctor.org/docs/asciidoc-recommended-practices/#one-sentence-per-line +## Invariants + +Recommended reads: + +- +- +- +- +- + +### Write down invariants clearly + +Write down invariants using `INVARIANT:` code comments. + +```rust +// GOOD + +// INVARIANT: for i in 0..lo: xs[i] < x + +// BAD + +// for i in 0..lo: xs[i] < x +``` + +**Rationale**: invariants should be upheld at all times. +It’s useful to keep invariants in mind when analyzing the flow of the code. +It’s easy to look up the local invariants when programming "in the small". + +For field invariants, a doc comment should come at the place where they are declared, inside the type definition. + +```rust +// GOOD +struct BitmapInfoHeader { + /// INVARIANT: `width.abs() <= u16::MAX` + width: i32, +} + +// BAD + +/// INVARIANT: `width.abs() <= u16::MAX` +struct BitmapInfoHeader { + width: i32, +} + +// BAD +struct BitmapInfoHeader { + width: i32, +} + +impl BitmapInfoHeader { + fn new(width: i32) -> Option { + // INVARIANT: width.abs() <= u16::MAX + if !(width.abs() <= i32::from(u16::MAX)) { + return None; + } + + Some(BitmapInfoHeader { width }) + } +} +``` + +**Rationale**: it’s easy to find about the invariant. +The invariant will show up in the documentation (typically available by hovering the item in IDEs). + +For loop invariants, the comment should come before or at the beginning of the loop. + +```rust +// GOOD + +/// Computes the smallest index such that, if `x` is inserted at this index, the array remains sorted. +fn insertion_point(xs: &[i32], x: i32) -> usize { + let mut lo = 0; + let mut hi = xs.len(); + + while lo < hi { + // INVARIANT: for i in 0..lo: xs[i] < x + // INVARIANT: for i in hi..: x <= xs[i] + + let mid = lo + (hi - lo) / 2; + if xs[mid] < x { + lo = mid + 1; + } else { + hi = mid; + } + } + + lo +} + +// BAD +fn insertion_point(xs: &[i32], x: i32) -> usize { + let mut lo = 0; + let mut hi = xs.len(); + + while lo < hi { + let mid = lo + (hi - lo) / 2; + if xs[mid] < x { + lo = mid + 1; + } else { + hi = mid; + } + } + + // INVARIANT: for i in 0..lo: xs[i] < x + // INVARIANT: for i in hi..: x <= xs[i] + + lo +} +``` + +**Rationale**: improved top-down readability, only read forward, no need to backtrack. + +For function output invariants, the comment should be specified in the doc comment. +(However, consider [enforcing this invariant][parse-dont-validate] using [the type system][type-safety] instead.) + +```rust +// GOOD + +/// Computes the stride of an uncompressed RGB bitmap. +/// +/// INVARIANT: `width <= output (stride) <= width * 4` +fn rgb_bmp_stride(width: u16, bit_count: u16) -> usize { + assert!(bit_count <= 32); + let stride = /* ... */; + stride +} + +// BAD + +/// Computes the stride of an uncompressed RGB bitmap. +fn rgb_bmp_stride(width: u16, bit_count: u16) -> usize { + assert!(bit_count <= 32); + // INVARIANT: width <= stride <= width * 4 + let stride = /* ... */; + stride +} +``` + +**Rationale**: it’s easy to find about the invariant. +The invariant will show up in the documentation (typically available by hovering the item in IDEs). + +[parse-dont-validate]: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/ +[type-safety]: https://www.parsonsmatt.org/2017/10/11/type_safety_back_and_forth.html + +### Explain non-obvious assumptions by referencing the invariants + +Explain clearly non-obvious assumptions and invariants relied upon (e.g.: when disabling a lint locally). +When referencing invariants, do not use the `INVARIANT:` comment prefix which is reserved for defining them. + +```rust +// GOOD + +// Per invariants: width * dst_n_samples <= 10_000 * 4 < usize::MAX +#[allow(clippy::arithmetic_side_effects)] +let dst_stride = usize::from(width) * dst_n_samples; + +// BAD +#[allow(clippy::arithmetic_side_effects)] +let dst_stride = usize::from(width) * dst_n_samples; + +// BAD + +// INVARIANT: width * dst_n_samples <= 10_000 * 4 < usize::MAX +#[allow(clippy::arithmetic_side_effects)] +let dst_stride = usize::from(width) * dst_n_samples; +``` + +**Rationale**: make the assumption obvious. +The code is easier to review. +No one will lose time refactoring based on the wrong assumption. + +### State invariants positively + +Establish invariants positively. +Prefer `if !invariant` to `if negated_invariant`. + +```rust +// GOOD +if !(idx < len) { + return None; +} + +// GOOD +check_invariant(idx < len)?; + +// GOOD +ensure!(idx < len); + +// GOOD +debug_assert!(idx < len); + +// GOOD +if idx < len { + /* ... */ +} else { + return None; +} + +// BAD +if idx >= len { + return None; +} +``` + +**Rationale:** it's useful to see the invariant relied upon by the rest of the function clearly spelled out. + +### Strongly prefer `<` and `<=` over `>` and `>=` + +Use `<` and `<=` operators instead of `>` and `>=`. + +```rust +/// GOOD +if lo <= x && x <= hi {} +if x < lo || hi < x {} + +/// BAD +if x >= lo && x <= hi {} +if x < lo || x > hi {} +``` + +**Rationale**: consistent, canonicalized form that is trivial to visualize by reading from left to right. +Things are naturally ordered from small to big like in the [number line]. + +[number line]: https://en.wikipedia.org/wiki/Number_line + ## Context parameters Some parameters are threaded unchanged through many function calls. diff --git a/crates/ironrdp-cliprdr-format/README.md b/crates/ironrdp-cliprdr-format/README.md index 9666d87f..7dfe8aad 100644 --- a/crates/ironrdp-cliprdr-format/README.md +++ b/crates/ironrdp-cliprdr-format/README.md @@ -3,5 +3,10 @@ This Library provides the conversion logic between RDP-specific clipboard formats and widely used formats like PNG for images, plain string for HTML etc. -### INVARIANTS -- This crate expects the target machine's pointer size (usize) to be equal or greater than 32bits \ No newline at end of file +### Overflows + +This crate has been audited by us and is guaranteed overflow-free on 32 and 64 bits architectures. +It would be easy to cause an overflow on a 16-bit architecture. +However, it’s hard to imagine an RDP client running on such machines. +Size of pointers on such architectures greatly limits the maximum size of the bitmap buffers. +It’s likely the RDP client will choke on a big payload before overflowing because of this crate. diff --git a/crates/ironrdp-cliprdr-format/src/bitmap.rs b/crates/ironrdp-cliprdr-format/src/bitmap.rs index 72fee08c..4a41ed12 100644 --- a/crates/ironrdp-cliprdr-format/src/bitmap.rs +++ b/crates/ironrdp-cliprdr-format/src/bitmap.rs @@ -144,14 +144,14 @@ impl PduEncode for CiexyzTriple { /// We don't use the optional `bmiColors` field, because it is only relevant for bitmaps with /// bpp < 24, which are not supported yet, therefore only fixed part of the header is implemented. /// -/// INVARIANT: `width` and `height` magnitudes are less than or equal to `u16::MAX`. -/// INVARIANT: `bit_count` is less than or equal to `32`. -/// /// [BITMAPINFO]: https://learn.microsoft.com/en-us/windows/win32/api/wingdi/ns-wingdi-bitmapinfo #[derive(Debug, Clone, Copy, PartialEq, Eq)] struct BitmapInfoHeader { + /// INVARIANT: `width.abs() <= 10_000` width: i32, + /// INVARIANT: `height.abs() <= 10_000` height: i32, + /// INVARIANT: `bit_count <= 32` bit_count: u16, compression: BitmapCompression, size_image: u32, @@ -176,21 +176,9 @@ impl BitmapInfoHeader { const NAME: &str = "BITMAPINFOHEADER"; - fn validate_invariants(&self) -> PduResult<()> { - check_invariant(self.width.abs() <= u16::MAX.into()) - .ok_or_else(|| invalid_message_err!("biWidth", "width is too big"))?; - check_invariant(self.height.abs() <= u16::MAX.into()) - .ok_or_else(|| invalid_message_err!("biHeight", "height is too big"))?; - check_invariant(self.bit_count <= 32).ok_or_else(|| invalid_message_err!("biBitCount", "invalid bit count"))?; - - Ok(()) - } - fn encode_with_size(&self, dst: &mut WriteCursor<'_>, size: u32) -> PduResult<()> { ensure_fixed_part_size!(in: dst); - self.validate_invariants()?; - dst.write_u32(size); dst.write_i32(self.width); dst.write_i32(self.height); @@ -212,12 +200,19 @@ impl BitmapInfoHeader { let size = src.read_u32(); let width = src.read_i32(); + check_invariant(width.abs() <= 10_000).ok_or_else(|| invalid_message_err!("biWidth", "width is too big"))?; + let height = src.read_i32(); + check_invariant(height.abs() <= 10_000).ok_or_else(|| invalid_message_err!("biHeight", "height is too big"))?; + let planes = src.read_u16(); if planes != 1 { return Err(invalid_message_err!("biPlanes", "invalid planes count")); } + let bit_count = src.read_u16(); + check_invariant(bit_count <= 32).ok_or_else(|| invalid_message_err!("biBitCount", "invalid bit count"))?; + let compression = BitmapCompression(src.read_u32()); let size_image = src.read_u32(); let x_pels_per_meter = src.read_i32(); @@ -237,19 +232,23 @@ impl BitmapInfoHeader { clr_important, }; - header.validate_invariants()?; - Ok((header, size)) } + // INVARIANT: output (width) < 10_000 fn width(&self) -> u16 { - // Cast is safe, invariant is checked in `encode_with_size` and `decode_with_size`. - u16::try_from(self.width.abs()).unwrap() + let abs = self.width.abs(); + debug_assert!(abs < 10_000); + // Per the invariant on self.width, this cast is infaillible. + u16::try_from(abs).unwrap() } + // INVARIANT: output (height) < 10_000 fn height(&self) -> u16 { - // Cast is safe, invariant is checked in `encode_with_size` and `decode_with_size`. - u16::try_from(self.height.abs()).unwrap() + let abs = self.height.abs(); + debug_assert!(abs < 10_000); + // Per the invariant on self.height, this cast is infaillible. + u16::try_from(abs).unwrap() } fn is_bottom_up(&self) -> bool { @@ -471,20 +470,31 @@ struct PngEncoderContext { color_type: png::ColorType, } -/// From MS docs: -/// For uncompressed RGB formats, the minimum stride is always the image width in bytes, rounded -/// up to the nearest DWORD (4 bytes). You can use the following formula to calculate the stride -/// and image size: +/// Computes the stride of an uncompressed RGB bitmap. +/// +/// INVARIANT: `width <= output (stride) <= width * 4` +/// +/// In an uncompressed bitmap, the stride is the number of bytes needed to go from the start of one +/// row of pixels to the start of the next row. The image format defines a minimum stride for an +/// image. In addition, the graphics hardware might require a larger stride for the surface that +/// contains the image. +/// +/// For uncompressed RGB formats, the minimum stride is always the image width in bytes, rounded up +/// to the nearest DWORD (4 bytes). The following formula is used to calculate the stride: +/// /// ``` -/// stride = ((((biWidth * biBitCount) + 31) & ~31) >> 3); -/// biSizeImage = abs(biHeight) * stride; +/// stride = ((((width * bit_count) + 31) & ~31) >> 3) /// ``` /// -/// INVARIANT: bit_count <= 32 -#[allow(clippy::arithmetic_side_effects)] +/// From Microsoft doc: https://learn.microsoft.com/en-us/windows/win32/api/wingdi/ns-wingdi-bitmapinfoheader fn rgb_bmp_stride(width: u16, bit_count: u16) -> usize { debug_assert!(bit_count <= 32); - (((usize::from(width) * usize::from(bit_count)) + 31) & !31) >> 3 + + // No side effects, because u16::MAX * 32 + 31 < u16::MAX * u16::MAX < u32::MAX + #[allow(clippy::arithmetic_side_effects)] + { + (((usize::from(width) * usize::from(bit_count)) + 31) & !31) >> 3 + } } fn bgra_to_top_down_rgba( @@ -508,9 +518,7 @@ fn bgra_to_top_down_rgba( (png::ColorType::Rgb, 3) }; - // INVARIANT: height * width * components <= u16::MAX * u16::MAX * 4 < usize::MAX - // This is always true because `components <= 4` is checked above, and width & height - // bounds are validated on PDU encode/decode + // Per invariants: height * width * dst_n_samples <= 10_000 * 10_000 * 4 < u32::MAX #[allow(clippy::arithmetic_side_effects)] let dst_bitmap_len = usize::from(height) * usize::from(width) * dst_n_samples; @@ -552,9 +560,7 @@ fn bgra_to_top_down_rgba( _ => unreachable!("possible values are restricted by header validation and logic above"), }; - // INVARIANT: width * components <= u16::MAX * 4 < usize::MAX - // - // + // Per invariants: width * dst_n_samples <= 10_000 * 4 < u32::MAX #[allow(clippy::arithmetic_side_effects)] let dst_stride = usize::from(width) * dst_n_samples; @@ -633,18 +639,15 @@ fn top_down_rgba_to_bottom_up_bgra( let width = u16::try_from(info.width).map_err(|_| BitmapError::WidthTooBig)?; let height = u16::try_from(info.height).map_err(|_| BitmapError::HeightTooBig)?; - #[allow(clippy::arithmetic_side_effects)] // width * 4 <= u16::MAX * 4 < usize::MAX + #[allow(clippy::arithmetic_side_effects)] // width * 4 <= 10_000 * 4 < u32::MAX let stride = usize::from(width) * 4; let src_rows = src_bitmap.chunks_exact(stride); - // INVARIANT: stride * height_unsigned <= usize::MAX. - // - // This never overflows, because stride can't be greater than `width_unsigned * 4`, - // and `width_unsigned * height_unsigned * 4` is guaranteed to be lesser or equal - // to `usize::MAX`. + // As per invariants: stride * height <= width * 4 * height <= 10_000 * 4 * 10_000 <= u32::MAX. #[allow(clippy::arithmetic_side_effects)] - let dst_len = u32::try_from(stride * usize::from(height)).map_err(|_| BitmapError::InvalidSize)?; + let dst_len = stride * usize::from(height); + let dst_len = u32::try_from(dst_len).map_err(|_| BitmapError::InvalidSize)?; let header = BitmapInfoHeader { width: i32::from(width), diff --git a/crates/ironrdp-cliprdr-format/src/html.rs b/crates/ironrdp-cliprdr-format/src/html.rs index 71ced6bc..a08df50e 100644 --- a/crates/ironrdp-cliprdr-format/src/html.rs +++ b/crates/ironrdp-cliprdr-format/src/html.rs @@ -93,11 +93,10 @@ pub fn plain_html_to_cf_html(fragment: &str) -> String { let mut buffer = String::new(); - // INVARIANT: key.len() + value.len() + ":\r\n".len() < usize::MAX - // This is always true because we know `key` and `value` used in code below are - // short and their sizes are far from `usize::MAX`. - #[allow(clippy::arithmetic_side_effects)] let mut write_header = |key: &str, value: &str| { + // This relation holds: key.len() + value.len() + ":\r\n".len() < usize::MAX + // Rationale: we know all possible values (see code below), and they are much smaller than `usize::MAX`. + #[allow(clippy::arithmetic_side_effects)] let size = key.len() + value.len() + ":\r\n".len(); buffer.reserve(size); @@ -112,10 +111,10 @@ pub fn plain_html_to_cf_html(fragment: &str) -> String { write_header("Version", "0.9"); - let start_html_header_pos = write_header("StartHTML", POS_PLACEHOLDER); - let end_html_header_pos = write_header("EndHTML", POS_PLACEHOLDER); - let start_fragment_header_pos = write_header("StartFragment", POS_PLACEHOLDER); - let end_fragment_header_pos = write_header("EndFragment", POS_PLACEHOLDER); + let start_html_header_value_pos = write_header("StartHTML", POS_PLACEHOLDER); + let end_html_header_value_pos = write_header("EndHTML", POS_PLACEHOLDER); + let start_fragment_header_value_pos = write_header("StartFragment", POS_PLACEHOLDER); + let end_fragment_header_value_pos = write_header("EndFragment", POS_PLACEHOLDER); let start_html_pos = buffer.len(); buffer.push_str("\r\n\r\n"); @@ -133,18 +132,19 @@ pub fn plain_html_to_cf_html(fragment: &str) -> String { let start_fragment_pos_value = format!("{:0>10}", start_fragment_pos); let end_fragment_pos_value = format!("{:0>10}", end_fragment_pos); - // INVARIANT: placeholder_pos + POS_PLACEHOLDER.len() < buffer.len() - // This is always valid because we know that placeholder is always present in the buffer - // after the header is written and placeholder is within the bounds of the buffer. - #[allow(clippy::arithmetic_side_effects)] - let mut replace_placeholder = |header_pos: usize, header_value: &str| { - buffer.replace_range(header_pos..header_pos + POS_PLACEHOLDER.len(), header_value); + let mut replace_placeholder = |value_begin_idx: usize, header_value: &str| { + // We know that: value_begin_idx + POS_PLACEHOLDER.len() < usize::MAX + // Rationale: the headers are written at the beginning, and we’re not indexing outside of the string. + #[allow(clippy::arithmetic_side_effects)] + let value_end_idx = value_begin_idx + POS_PLACEHOLDER.len(); + + buffer.replace_range(value_begin_idx..value_end_idx, header_value); }; - replace_placeholder(start_html_header_pos, &start_html_pos_value); - replace_placeholder(end_html_header_pos, &end_html_pos_value); - replace_placeholder(start_fragment_header_pos, &start_fragment_pos_value); - replace_placeholder(end_fragment_header_pos, &end_fragment_pos_value); + replace_placeholder(start_html_header_value_pos, &start_html_pos_value); + replace_placeholder(end_html_header_value_pos, &end_html_pos_value); + replace_placeholder(start_fragment_header_value_pos, &start_fragment_pos_value); + replace_placeholder(end_fragment_header_value_pos, &end_fragment_pos_value); buffer }