mirrors/IronRDP
1
0
Fork 0
mirror of https://github.com/Devolutions/IronRDP.git synced 2025-07-07 17:45:01 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

style(cliprdr): clarify invariants

Browse source
This commit is contained in:
Benoît CORTIER 2023-11-19 20:24:32 -05:00 • committed by Benoît Cortier
parent 162695534f
commit 82d7977672
4 changed files with 296 additions and 63 deletions
Download patch file Download diff file Expand all files Collapse all files

225
STYLE.md
View file

@ -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:
- <https://en.wikipedia.org/wiki/Invariant_(mathematics)#Invariants_in_computer_science>
- <https://en.wikipedia.org/wiki/Loop_invariant>
- <https://en.wikipedia.org/wiki/Class_invariant>
- <https://matklad.github.io/2023/10/06/what-is-an-invariant.html>
- <https://matklad.github.io/2023/09/13/comparative-analysis.html>
### 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<BitmapInfoHeader> {
// 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.

9
crates/ironrdp-cliprdr-format/README.md
View file

@ -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
### 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.

87
crates/ironrdp-cliprdr-format/src/bitmap.rs
View file

@ -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);
// 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),

34
crates/ironrdp-cliprdr-format/src/html.rs
View file

@ -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("<html>\r\n<body>\r\n<!--StartFragment-->");
@ -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.
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 mut replace_placeholder = |header_pos: usize, header_value: &str| {
buffer.replace_range(header_pos..header_pos + POS_PLACEHOLDER.len(), header_value);
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
}