Proposal: Fix brotli decompressor buffer typo, replace framing/header magic numbers
- Lingua principale
- Rust
- Stelle
- 24.8k
- Fork
- 1.9k
- Merge medio
- 23h 10m
- PR unite (30g)
- 26
Descrizione
### Summary & Motivation
This proposal tracks eliminating unexplained magic numeric literals across the `actix-web` workspace, fixing buffer alignment anomalies, and replacing hardcoded RFC framing constants with self-documenting compile-time constants (`const`).
> **Audit Telemetry**: This scan and static analysis audit was performed across all 318 `.rs` files (86,266 LOC) using **GEMINI-3.7-FLASH**.
---
### Implementation Plan & Note for Maintainers
If maintainers approve of this cleanup, **I will implement these changes iteratively across small, focused PRs** (e.g. splitting the Brotli buffer fix, RFC 7231 Date header cleanup, and WebSocket constants) so reviewing remains straightforward and modular.
Additionally, **minor magic numbers** (e.g. trivial 1-byte offsets, string split delimiters, or obvious test assertions) will be intentionally left as-is to avoid unnecessary abstraction churn. The focus is solely on allocator-impacting buffer sizes, protocol framing thresholds, and timeouts.
---
### Major Issues Identified
#### 1. The Brotli Decompressor Buffer Typo (`actix-http/src/encoding/decoder.rs`)
```rust
// Current code in actix-http/src/encoding/decoder.rs:
#[cfg(feature = "compress-brotli")]
ContentEncoding::Brotli => Some(ContentDecoder::Brotli(Box::new(
brotli::DecompressorWriter::new(Writer::new(), 8_096), // <-- Typo
))),
```
* **The Problem**: $8,096$ is neither a power of two nor an integer multiple of the standard $4\text{ KiB}$ memory page ($8,192 - 96$). CPU cache lines and memory allocators cry themselves to sleep whenever an unaligned $8,096$-byte chunk is requested. The rest of the encoding module uses $8\text{ KiB}$ or $32\text{ KiB}$ buffers.
* **The Fix**: Define `const DEFAULT_DECODER_CAPACITY: usize = 8 * 1024;` ($8\text{ KiB}$) to restore clean page alignment.
#### 2. The Date Header Scratchpad Buffer (`actix-http/src/config.rs`)
Unit tests were already importing `use crate::date::DATE_VALUE_LENGTH;` (29), but the production date writer is doing manual index math:
```rust
// Current code in actix-http/src/config.rs:
let mut buf: [u8; 37] = [0; 37];
buf[..6].copy_from_slice(if camel_case { b"Date: " } else { b"date: " });
self.0.date_service.with_date(|date| buf[6..35].copy_from_slice(&date.bytes));
buf[35..].copy_from_slice(b"\r\n");
```
* **The Problem**: If the date format ever shifted by even 1 byte, this function would silently slice corrupted HTTP headers or panic at runtime on an index out of bounds.
* **The Fix**:
```rust
const PREFIX_LEN: usize = 6; // b"Date: ".len()
const CRLF_LEN: usize = 2; // b"\r\n".len()
const DATE_HEADER_LEN: usize = PREFIX_LEN + DATE_VALUE_LENGTH + CRLF_LEN; // 37
```
Bound all slice offsets to compiler-verified `const` arithmetic.
#### 3. RFC 6455 WebSocket Framing Literals (`actix-http/src/ws/frame.rs`)
Replace raw binary bitmasks and protocol thresholds with standardized named constants:
- Masks: `FIN_MASK = 0x80`, `MASK_BIT = 0x80`, `OPCODE_MASK = 0x0F`, `PAYLOAD_LEN_MASK = 0x7F`.
- Length thresholds: `EXT_LEN_U16 = 126`, `EXT_LEN_U64 = 127`, `MAX_CONTROL_FRAME_PAYLOAD = 125`, `U16_PAYLOAD_LIMIT = 65_535`.
- RFC 6455 Section 7.4.1 Close status codes mapped cleanly rather than using naked numeric literals.
---
### Detailed Audit: Magic Numbers by Category
#### 1. Buffer Capacities, Pre-allocations & Limits
| Crate | File : Line | Value | Code Snippet / Context |
|---|---|---|---|
| `actix-http` | `encoding/decoder.rs:46`| `8_096` | `DecompressorWriter::new(Writer::new(), 8_096)` (Typo -> 8192) |
| `actix-http` | `encoding/mod.rs:22` | `8192` | `buf: BytesMut::with_capacity(8192)` |
| `actix-http` | `encoding/encoder.rs:408` | `32, 1024` | `32 * 1024` (32 KiB Brotli buffer) |
| `actix-web` | `types/json.rs:370` | `8192` | `buf: BytesMut::with_capacity(8192)` (JSON body pre-allocation) |
| `actix-web` | `types/payload.rs:395` | `8192` | `buf: BytesMut::with_capacity(8192)` |
| `actix-web` | `types/form.rs:380` | `8192` | `let mut body = BytesMut::with_capacity(8192);` |
| `actix-web` | `types/form.rs:257` | `16_384` | `limit: 16_384` (Default 16 KiB form payload limit) |
| `actix-web` | `types/form.rs:326, 336` | `32_768` | `limit: 32_768` (32 KiB form limit) |
| `actix-web` | `types/readlines.rs:44, 45`| `262_144`| `buf: BytesMut::with_capacity(262_144)`, `limit: 262_144` (256 KiB) |
| `actix-multipart`| `payload.rs:67` | `1_024` | `buf: BytesMut::with_capacity(1_024)` |
| `actix-multipart`| `form/bytes.rs:31` | `131_072` | `BytesMut::with_capacity(131_072)` (128 KiB) |
| `actix-multipart`| `form/mod.rs:563` | `52_428_800` | `total_limit: 52_428_800` (50 MiB default upload cap) |
| `actix-multipart`| `form/mod.rs:564` | `2_097_152` | `memory_limit: 2_097_152` (2 MiB before tempfile spool) |
| `actix-files` | `chunked.rs:126` | `65_536` | `cmp::min(size.saturating_sub(counter), 65_536)` (64 KiB file chunk) |
| `actix-http` | `requests/head.rs:32` | `16` | `headers: HeaderMap::with_capacity(16)` |
| `actix-http` | `responses/head.rs:27` | `12` | `headers: HeaderMap::with_capacity(12)` |
| `actix-http` | `message.rs:79, 103` | `128` | Request head pool capacity `128`, limit `pool.len() < 128` |
| `actix-http` | `responses/head.rs:173, 197`| `128` | Response head pool capacity `128`, limit `pool.len() < 128` |
| `actix-http` | `header/map.rs:56..878` | `4` | `SmallVec<[HeaderValue; 4]>` (Inline multi-value header capacity) |
| `actix-web` | `request.rs:48, 50, 66` | `4` | `SmallVec<[u16; 4]>`, `SmallVec<[Rc; 4]>` |
| `actix-web` | `request.rs:678` | `128` | `Self::with_capacity(128)` (Path string buffer) |
#### 2. WebSocket Framing & Protocol Constants
| Crate | File : Line | Value | Code Snippet / Context |
|---|---|---|---|
| `actix-http` | `ws/codec.rs:89` | `65_536` | `max_size: 65_536` (Default 64 KiB WebSocket frame limit) |
| `awc` | `ws.rs:95` | `65_536` | `max_size: 65_536` (Default client WS frame limit) |
| `actix-http` | `ws/mask.rs:5, 11, 19` | `4` | `mask: [u8; 4]` (RFC 6455 4-byte client mask) |
| `actix-http` | `ws/mask.rs:13, 30` | `3` | `*byte ^= mask[i & 3];`, `prefix.len() & 3` |
| `actix-http` | `ws/mask.rs:33, 35` | `8` | `mask_u32.rotate_left(8 * head as u32)` |
| `actix-http` | `ws/frame.rs:24, 49, 58`| `2, 4, 10` | Frame header size checks: `chunk_len < 2`, `< 4`, `< 10` |
| `actix-http` | `ws/frame.rs:30, 33` | `0x80` | `first & 0x80 != 0` (FIN bit), `second & 0x80 != 0` (MASK bit) |
| `actix-http` | `ws/frame.rs:41, 44` | `0x0F` | `first & 0x0F` (Opcode 4-bit nibble mask) |
| `actix-http` | `ws/frame.rs:47` | `0x7F` | `second & 0x7F` (Payload len 7-bit mask) |
| `actix-http` | `ws/frame.rs:48, 57` | `126, 127` | Extended payload thresholds: `len == 126`, `len == 127` |
| `actix-http` | `ws/frame.rs:135, 138` | `125` | `length > 125` (Max payload limit for control frames) |
| `actix-http` | `ws/frame.rs:193` | `65_535` | `payload_len <= 65_535` (u16 payload frame threshold) |
| `actix-http` | `ws/proto.rs:56-59` | `2, 8, 9, 10` | Frame Opcodes: Binary(`2`), Close(`8`), Ping(`9`), Pong(`10`) |
| `actix-http` | `ws/proto.rs:156-191` | `1000..1015` | Close Codes: Normal(`1000`), Away(`1001`), etc. |
| `actix-http` | `ws/proto.rs:232, 244` | `28` | `[u8; 28]` (Sec-WebSocket-Accept SHA-1 Base64 length) |
| `awc` | `ws.rs:340` | `16` | `rand::random::<[u8; 16]>()` (Sec-WebSocket-Key nonce) |
#### 3. HTTP Protocol Framing, Slicing & Parsing
| Crate | File : Line | Value | Code Snippet / Context |
|---|---|---|---|
| `actix-http` | `h1/encoder.rs:166` | `4` | `let len = k_len + v_len + 4;` (`: ` + `\r\n`) |
| `actix-http` | `h1/encoder.rs:196, 202`| `2` | `write_data(b": ", buf, 2)`, `write_data(b"\r\n", buf, 2)` |
| `actix-http` | `h1/encoder.rs:275, 307`| `256` | `dst.reserve(256 + head.headers.len() * ...)` |
| `actix-http` | `h1/encoder.rs:513, 524`| `0b1101_1111`| `buffer[0] = c & 0b1101_1111;` (ASCII upper-case conversion) |
| `actix-http` | `h1/chunked.rs:55` | `16` | `let radix = 16;` (Hex chunk size radix) |
| `actix-http` | `h1/chunked.rs:59, 60` | `10` | `b + 10 - b'a'`, `b + 10 - b'A'` (Hex nibble math) |
| `actix-http` | `h1/chunked.rs:105` | `0x00..=0x1f, 0x7f` | Forbidden control chars in chunk extension |
| `actix-http` | `h1/decoder.rs:179` | `4` | `&bytes[0..4] == b"100-"` (HTTP 100 Continue) |
| `actix-http` | `helpers.rs:18-20` | `100, 10` | `(n / 100) as u8`, `((n / 10) % 10) as u8`, `(n % 10) as u8` |
| `actix-http` | `config.rs:333-341` | `37, 6, 35` | Hardcoded indices for `Date: \r\n` |
| `actix-multipart`| `field.rs:305-348` | `4, 2, 3` | Delimiter byte lengths for `\r\n--` multipart boundaries |
| `actix-web` | `http/header/entity.rs:13`| `0x21, 0x23, 0x7e, 0x80` | Valid ASCII byte ranges for HTTP ETags |
| `actix-web` | `http/header/entity.rs:151-169`| `2, 4, 3` | Weak/strong quote stripping offsets (`W/"..."`) |
| `actix-web` | `http/header/accept_encoding.rs:252-255`| `5, 4, 3, 2` | Hardcoded priority ranking: Brotli(`5`), Zstd(`4`), Gzip(`3`), Deflate(`2`) |
| `actix-files` | `named.rs:404` | `1_000_000_000` | `1_000_000_000 - dur.subsec_nanos()` |
| `actix-files` | `named.rs:429` | `253_402_300_800` | Max Unix timestamp before year 10000 |
| `actix-web` | `middleware/logger.rs:630` | `1_000_000.0` | `(rt.whole_nanoseconds() as f64) / 1_000_000.0;` (ns to ms) |
#### 4. Client Configuration & Pool Lifetimes
| Crate | File : Line | Value | Code Snippet / Context |
|---|---|---|---|
| `awc` | `builder.rs:61` | `5` | `timeout: Some(Duration::from_secs(5))` |
| `awc` | `client/config.rs:23, 24` | `5` | `timeout`, `handshake_timeout`: `Duration::from_secs(5)` |
| `awc` | `client/config.rs:25` | `75` | `conn_lifetime: Duration::from_secs(75)` (Pool max lifetime) |
| `awc` | `client/config.rs:26` | `15` | `conn_keep_alive: Duration::from_secs(15)` |
| `awc` | `client/config.rs:27` | `3000` | `disconnect_timeout: Some(Duration::from_millis(3000))` |
| `awc` | `client/config.rs:28` | `100` | `limit: 100` (Max pooled connections per host) |
| `awc` | `builder.rs:65`, `redirect.rs:35` | `10` | `max_redirects: 10`, `max_redirect_times: 10` |
#### 5. Compression Parameters & Codecs
| Crate | File : Line | Value | Code Snippet / Context |
|---|---|---|---|
| `actix-http` | `encoding/encoder.rs:310` | `3` | `ZstdEncoder::new(Writer::new(), 3)` (Zstd default level) |
| `actix-http` | `encoding/encoder.rs:409` | `3` | Brotli `BROTLI_PARAM_QUALITY = 3` |
| `actix-http` | `encoding/encoder.rs:410` | `22` | Brotli `BROTLI_PARAM_LGWIN = 22` |
Guida per i contributori
Apri la guida per i contributori
Valutazione
Questa issue non è ancora stata valutata.