actix / actix/actix-web

Proposal: Fix brotli decompressor buffer typo, replace framing/header magic numbers

Abierto
#4,226 0 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
Rust
Estrellas
24.8k
Forks
1.9k
Merge medio
23 h 10 min
PR fusionados (30 d)
26

Descripción

### 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` |

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.