-
Notifications
You must be signed in to change notification settings - Fork 0
Expand file tree
/
Copy pathhyper_20260615.html
More file actions
761 lines (704 loc) · 212 KB
/
Copy pathhyper_20260615.html
File metadata and controls
761 lines (704 loc) · 212 KB
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725
726
727
728
729
730
731
732
733
734
735
736
737
738
739
740
741
742
743
744
745
746
747
748
749
750
751
752
753
754
755
756
757
758
759
760
761
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width,initial-scale=1">
<title>Code Audit — Interactive</title>
<style>
*,*::before,*::after{box-sizing:border-box;margin:0;padding:0}
body{font-family:-apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,sans-serif;background:#0f172a;color:#e2e8f0;font-size:14px;line-height:1.5}
.app{max-width:1100px;margin:0 auto;padding:1.5rem 1rem}
.top{display:flex;justify-content:space-between;align-items:flex-start;gap:1rem;margin-bottom:1rem;flex-wrap:wrap}
h1{font-size:1.5rem;font-weight:700;color:#f1f5f9}
.meta{font-size:.8rem;color:#64748b;margin-top:.2rem}
.top-right{display:flex;align-items:center;gap:.75rem;flex-wrap:wrap}
.project-info{margin-bottom:1rem;padding:.75rem .9rem;background:#1e293b;border:1px solid #334155;border-radius:8px}
.project-info .pi-name{font-size:.95rem;font-weight:700;color:#f1f5f9}
.project-info .pi-desc{font-size:.82rem;color:#cbd5e1;margin-top:.25rem}
.project-info .pi-row{display:flex;flex-wrap:wrap;gap:.4rem .9rem;margin-top:.45rem;font-size:.78rem;color:#94a3b8}
.project-info .pi-row .pi-k{color:#64748b}
.project-info a{color:#60a5fa;text-decoration:none}
.project-info a:hover{text-decoration:underline}
.project-info code{font-family:"JetBrains Mono","Fira Code",Consolas,monospace;font-size:.74rem;color:#93c5fd}
.pi-repo-btn{display:inline-flex;align-items:center;gap:.3rem;padding:.18rem .5rem;background:#0f172a;border:1px solid #3b82f6;border-radius:6px;color:#60a5fa !important;font-weight:600}
.pi-repo-btn:hover{background:#1d283a;text-decoration:none !important}
#search{background:#1e293b;border:1px solid #334155;color:#e2e8f0;padding:.4rem .75rem;border-radius:6px;font-size:.875rem;width:220px;outline:none}
#search:focus{border-color:#3b82f6}
.group-label{font-size:.8rem;color:#94a3b8;display:flex;align-items:center;gap:.4rem}
select{background:#1e293b;border:1px solid #334155;color:#e2e8f0;padding:.35rem .6rem;border-radius:6px;font-size:.8rem;outline:none;cursor:pointer}
select:focus{border-color:#3b82f6}
.filterbar{display:flex;flex-direction:column;gap:.4rem;margin-bottom:.6rem;padding:.6rem .75rem;background:#1e293b;border:1px solid #334155;border-radius:8px}
.filter-row{display:flex;flex-wrap:wrap;gap:.3rem;align-items:center}
.filter-lbl{font-size:.65rem;font-weight:700;text-transform:uppercase;letter-spacing:.08em;color:#475569;white-space:nowrap;margin-right:.25rem}
.filter-toggle{font-size:.65rem;font-weight:600;padding:.18rem .5rem;border-radius:4px;border:1px solid #334155;background:#0f172a;color:#94a3b8;cursor:pointer;margin-left:auto;white-space:nowrap;letter-spacing:.04em}
.filter-toggle:hover{background:#1e293b;color:#e2e8f0;border-color:#475569}
.badge{font-size:.68rem;font-weight:700;padding:.18rem .5rem;border-radius:4px;border:1px solid transparent;cursor:pointer;letter-spacing:.03em;transition:opacity .12s,filter .12s;user-select:none}
.badge.off{opacity:.28;filter:grayscale(.5)}
.sev-critical{background:#450a0a;color:#fca5a5;border-color:#991b1b}
.sev-high {background:#431407;color:#fdba74;border-color:#9a3412}
.sev-medium {background:#422006;color:#fcd34d;border-color:#854d0e}
.sev-low {background:#0c1a3a;color:#93c5fd;border-color:#1d4ed8}
.sev-negligible{background:#1e293b;color:#64748b;border-color:#334155}
.type-badge{background:#0c1e33;color:#7dd3fc;border-color:#0369a1}
.easy-badge{background:#052e16;color:#86efac;border-color:#16a34a}
.st-open{background:#1e293b;color:#475569;border-color:#334155}
.st-fixed{background:#052e16;color:#4ade80;border-color:#16a34a}
.st-wont{background:#431407;color:#fdba74;border-color:#9a3412}
.st-fp{background:#2e1065;color:#c4b5fd;border-color:#7c3aed}
#progress-wrap{display:flex;align-items:center;gap:.75rem;margin-bottom:.6rem;flex-wrap:wrap}
#progress-bar{flex:1;min-width:120px;height:7px;background:#1e293b;border-radius:4px;overflow:hidden;border:1px solid #334155}
#progress-fill{height:100%;background:#22c55e;border-radius:4px;transition:width .35s ease;width:0}
#progress-text{font-size:.75rem;color:#64748b;white-space:nowrap}
.action-btn{font-size:.72rem;padding:.25rem .6rem;border-radius:5px;border:1px solid #334155;background:#1e293b;color:#94a3b8;cursor:pointer;white-space:nowrap}
.action-btn:hover{background:#334155;color:#e2e8f0}
#hidden-bar{margin-bottom:.6rem}
.hidden-inner{display:flex;flex-wrap:wrap;gap:.35rem;align-items:center}
.hidden-lbl{font-size:.72rem;color:#475569}
.show-btn{font-size:.7rem;padding:.15rem .45rem;border-radius:4px;background:#1e293b;color:#94a3b8;border:1px solid #334155;cursor:pointer}
.show-btn:hover{background:#334155;color:#e2e8f0}
.show-all-btn{font-size:.7rem;padding:.15rem .45rem;border-radius:4px;background:#2563eb;color:#fff;border:none;cursor:pointer}
.show-all-btn:hover{background:#1d4ed8}
.group-section{margin-bottom:1.5rem}
.group-header{display:flex;align-items:center;gap:.5rem;margin-bottom:.5rem;padding-bottom:.35rem;border-bottom:1px solid #334155}
.group-title{font-size:.95rem;font-weight:700;color:#cbd5e1}
.group-title.all-done{color:#4ade80}
.group-count{font-size:.72rem;color:#475569;background:#1e293b;padding:.1rem .4rem;border-radius:10px;white-space:nowrap}
.icon-btn{background:transparent;border:none;color:#334155;cursor:pointer;font-size:.85rem;padding:.05rem .2rem;line-height:1}
.icon-btn:hover{color:#64748b}
.hide-btn{margin-left:auto;font-size:.68rem;padding:.12rem .45rem;border-radius:4px;background:transparent;color:#334155;border:1px solid #1e293b;cursor:pointer}
.hide-btn:hover{background:#1e293b;color:#64748b}
.card{background:#1e293b;border:1px solid #334155;border-radius:8px;margin-bottom:.6rem;overflow:hidden;transition:opacity .15s}
.card.resolved{opacity:.55;border-left:3px solid #16a34a}
.card.resolved:hover{opacity:.85}
.card-header{display:flex;align-items:center;gap:.5rem;padding:.45rem .85rem;background:#162032;border-bottom:1px solid #334155;flex-wrap:wrap;cursor:pointer;user-select:none}
.card-header:hover{background:#1a2840}
.card-arrow{font-size:.65rem;color:#334155;flex-shrink:0}
.card-id{font-size:.75rem;font-weight:700;color:#64748b;font-family:monospace;letter-spacing:.04em}
.card-loc{font-size:.72rem;color:#64748b;font-family:"JetBrains Mono","Fira Code",Consolas,monospace;letter-spacing:.02em;white-space:nowrap;overflow:hidden;text-overflow:ellipsis;min-width:0}
.status-btn{margin-left:auto;font-size:.68rem;font-weight:600;padding:.15rem .5rem;border-radius:4px;border:1px solid transparent;cursor:pointer;white-space:nowrap;transition:filter .1s}
.status-btn:hover{filter:brightness(1.25)}
.copy-btn{font-size:.68rem;font-weight:600;padding:.15rem .5rem;border-radius:4px;border:1px solid #334155;background:#0f172a;color:#94a3b8;cursor:pointer;white-space:nowrap;transition:background .12s,color .12s}
.copy-btn:hover{background:#334155;color:#e2e8f0}
.card-body{padding:.85rem .9rem}
.field-lbl{font-size:.85rem;font-weight:700;text-transform:uppercase;letter-spacing:.1em;color:#7dd3fc;margin:1rem 0 .35rem;padding-bottom:.18rem;border-bottom:1px solid #1e293b}
.field-lbl:first-child{margin-top:0}
.desc,.fix-txt{font-size:.875rem;color:#cbd5e1;line-height:1.55}
.desc p,.fix-txt p{margin:.4rem 0}
.desc p:first-child,.fix-txt p:first-child{margin-top:0}
.desc p:last-child,.fix-txt p:last-child{margin-bottom:0}
.loc-block{margin:.3rem 0 .6rem}
.loc-ref{font-size:.8rem;color:#38bdf8;font-family:"JetBrains Mono","Fira Code",Consolas,monospace;font-weight:600;margin-bottom:.22rem}
.loc-lines{color:#475569;font-weight:400}
pre{background:#0f172a;color:#cdd6f4;border-radius:5px;padding:.6rem .8rem;overflow-x:auto;white-space:pre;font-size:.8rem;line-height:1.55;font-family:"JetBrains Mono","Fira Code",Consolas,monospace;border:1px solid #1e293b;max-height:320px}
.desc pre.md-code,.fix-txt pre.md-code{margin:.5rem 0;font-size:.82rem;color:#e2e8f0;background:#0b1224;border:1px solid #1e293b;max-height:none}
code.md-inline{font-family:"JetBrains Mono","Fira Code",Consolas,monospace;font-size:.86em;background:#0b1224;color:#fca5a5;border:1px solid #1e293b;border-radius:3px;padding:.04rem .32rem;white-space:pre-wrap;word-break:break-word}
.not-found{font-size:.8rem;color:#334155;font-style:italic}
mark{background:#854d0e;color:#fef9c3;border-radius:2px;padding:0 .1rem}
.no-results{text-align:center;color:#334155;padding:3rem 1rem;font-size:.9rem}
</style>
</head>
<body>
<script type="application/json" id="D">[{"id":"PANIC_1","type":"panic","type_label":"Panic","severity":"high","easy":false,"description":"Callback::is_canceled() and Callback::poll_canceled() both hit an `unreachable!()` arm when the inner Option is None. The Option is taken (set to None) by Callback::send(), which is called before these methods in some code paths. If any future caller polls the same Callback again after send() has already consumed the inner sender, execution will panic with 'internal error: entered unreachable code'.","fix":"Replace the `_ => unreachable!()` arms with a sensible fallback: is_canceled should return true, and poll_canceled should return Poll::Ready(()) when the sender is already gone.","locations":[{"ref":"src/client/dispatch.rs:268-282","code":" 268 | pub(crate) fn is_canceled(&self) -> bool {\n 269 | match *self {\n 270 | Callback::Retry(Some(ref tx)) => tx.is_closed(),\n 271 | Callback::NoRetry(Some(ref tx)) => tx.is_closed(),\n 272 | _ => unreachable!(),\n 273 | }\n 274 | }\n 275 | \n 276 | pub(crate) fn poll_canceled(&mut self, cx: &mut Context<'_>) -> Poll<()> {\n 277 | match *self {\n 278 | Callback::Retry(Some(ref mut tx)) => tx.poll_closed(cx),\n 279 | Callback::NoRetry(Some(ref mut tx)) => tx.poll_closed(cx),\n 280 | _ => unreachable!(),\n 281 | }\n 282 | }","lines":15}]},{"id":"PANIC_2","type":"panic","type_label":"Panic","severity":"high","easy":true,"description":"In `is_complete_fast`, when a `\\r` byte is found at position `i`, the code immediately indexes into `bytes[i + 1..]` without checking whether `i + 1` is within bounds. If the buffer ends with `\\r`, the slice `bytes[i + 1..]` is empty and the `chunks(3).next()` call simply returns `None`, but if any subsequent parsing reads beyond the end (e.g., because the check is incorrectly used to assert completeness), it can cause incorrect results. More critically, the pattern `bytes[i + 1..].chunks(3).next() == Some(&b\"\\n\\r\\n\"[..])` only works correctly when the slice has at least 3 bytes remaining; a short buffer (e.g. just `\\r\\n`) causes a spurious `None` but not a panic. However, if in a future refactor this slice were to be indexed directly, it would panic. The current code is technically safe but fragile. A clearer bound check is advisable.","fix":"Use `bytes.get(i+1..i+4)` instead of slicing `bytes[i+1..]` to avoid potential future panics and make the intent explicit.","locations":[{"ref":"src/proto/h1/role.rs:100-115","code":" 100 | fn is_complete_fast(bytes: &[u8], prev_len: usize) -> bool {\n 101 | let start = prev_len.saturating_sub(3);\n 102 | let bytes = &bytes[start..];\n 103 | \n 104 | for (i, b) in bytes.iter().copied().enumerate() {\n 105 | if b == b'\\r' {\n 106 | if bytes[i + 1..].chunks(3).next() == Some(&b\"\\n\\r\\n\"[..]) {\n 107 | return true;\n 108 | }\n 109 | } else if b == b'\\n' && bytes.get(i + 1) == Some(&b'\\n') {\n 110 | return true;\n 111 | }\n 112 | }\n 113 | \n 114 | false\n 115 | }","lines":16}]},{"id":"PANIC_3","type":"panic","type_label":"Panic","severity":"high","easy":true,"description":"In `Conn::poll` (client.rs), `set_initial_window_size` returns a `Result` that is propagated via the `?` operator, but the enclosing `Future::poll` signature returns `Poll<Result<(), h2::Error>>`. Specifically, when `Ponged::SizeUpdate` is received, the code calls `this.conn.set_initial_window_size(wnd)?` inside a `match` arm that has no explicit return type annotation - this compiles correctly. However, the error from `set_initial_window_size` will short-circuit the poll and return `Poll::Ready(Err(...))`, bypassing any further connection polling. The connection would then be abandoned mid-stream if the window size update fails, potentially leaving the client stuck waiting for responses that will never come because `ConnTask` returns `Poll::Ready(())` on conn completion, which signals the dispatch task to shut down.","fix":"After returning `Poll::Ready(Err(...))` from the window size failure, log the error explicitly and consider whether the connection should be cleanly shut down rather than just dropped. This is largely a correctness/robustness issue rather than a panic, but the abrupt drop of pending streams should be documented.","locations":[{"ref":"src/proto/h2/client.rs:237-252","code":" 237 | fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {\n 238 | let mut this = self.project();\n 239 | match this.ponger.poll(cx) {\n 240 | Poll::Ready(ping::Ponged::SizeUpdate(wnd)) => {\n 241 | this.conn.set_target_window_size(wnd);\n 242 | this.conn.set_initial_window_size(wnd)?;\n 243 | }\n 244 | Poll::Ready(ping::Ponged::KeepAliveTimedOut) => {\n 245 | debug!(\"connection keep-alive timed out\");\n 246 | return Poll::Ready(Ok(()));\n 247 | }\n 248 | Poll::Pending => {}\n 249 | }\n 250 | \n 251 | Pin::new(&mut this.conn).poll(cx)\n 252 | }","lines":16}]},{"id":"PANIC_4","type":"panic","type_label":"Panic","severity":"high","easy":false,"description":"In `ConnTask::poll` (client.rs), when `drop_rx` becomes ready (signalling the client was dropped), the code calls `this.cancel_tx.take().expect(\"ConnTask Future polled twice\")`. If `poll_next` on `drop_rx` somehow returns `Poll::Ready` a second time (e.g. due to a buggy waker or executor that re-polls after completion), the `expect` will panic. `drop_rx` is a `Receiver<Infallible>`, which is fused after receiving `None`, but if the executor violates the contract of not polling a completed future, this panics in production.","fix":"Guard with `if let Some(cancel_tx) = this.cancel_tx.take()` and drop it, removing the `.expect()`.","locations":[{"ref":"src/proto/h2/client.rs:353-359","code":" 353 | if !this.drop_rx.is_terminated() && Pin::new(&mut this.drop_rx).poll_next(cx).is_ready() {\n 354 | // mpsc has been dropped, hopefully polling\n 355 | // the connection some more should start shutdown\n 356 | // and then close.\n 357 | trace!(\"send_request dropped, starting conn shutdown\");\n 358 | drop(this.cancel_tx.take().expect(\"ConnTask Future polled twice\"));\n 359 | }","lines":7}]},{"id":"PANIC_5","type":"panic","type_label":"Panic","severity":"high","easy":false,"description":"In `PipeMap::poll` (client.rs), when the cancel receiver fires or the pipe completes, the code calls `this.conn_drop_ref.take().expect(\"Future polled twice\")` and `this.ping.take().expect(\"Future polled twice\")`. If an executor polls a completed future a second time, both `expect` calls will panic with an unhelpful message. While well-behaved executors do not do this, the `#[must_use]` comment on other futures and the general defense-in-depth practice in async Rust suggests these should not panic.","fix":"Replace `.expect(\"Future polled twice\")` with `.take()` (silently no-op on second poll) or use `if let Some(x) = x.take()` guards.","locations":[{"ref":"src/proto/h2/client.rs:491-493","code":" 491 | drop(this.conn_drop_ref.take().expect(\"Future polled twice\"));\n 492 | drop(this.ping.take().expect(\"Future polled twice\"));\n 493 | return Poll::Ready(());","lines":3},{"ref":"src/proto/h2/client.rs:508-509","code":" 508 | drop(this.conn_drop_ref.take().expect(\"Future polled twice\"));\n 509 | drop(this.ping.take().expect(\"Future polled twice\"));","lines":2}]},{"id":"PANIC_6","type":"panic","type_label":"Panic","severity":"medium","easy":false,"description":"In Connection::send_request (HTTP/1), and identically in the HTTP/2 version, the oneshot receiver returning Err (the sender was dropped without sending) causes a panic: `panic!(\"dispatch dropped without returning error\")`. While the comment says this 'shouldn't happen', it is a panic in stable public API code reachable by any user. If the runtime drops the dispatch task due to a shutdown race, this fires.","fix":"Convert the panic to a proper error return, e.g. `Err(crate::Error::new_user_dispatch_gone())`, consistent with what the Callback::drop impl already sends.","locations":[{"ref":"src/client/conn/http1.rs:225","code":" 225 | Err(_canceled) => panic!(\"dispatch dropped without returning error\"),","lines":1},{"ref":"src/client/conn/http2.rs:162","code":" 162 | Err(_canceled) => panic!(\"dispatch dropped without returning error\"),","lines":1}]},{"id":"PANIC_7","type":"panic","type_label":"Panic","severity":"medium","easy":false,"description":"In Connection::try_send_request (HTTP/1 and HTTP/2), the same `panic!(\"dispatch dropped without returning error\")` pattern is repeated.","fix":"Same fix as the send_request panics: return a proper error instead of panicking.","locations":[{"ref":"src/client/conn/http1.rs:254","code":" 254 | Err(_) => panic!(\"dispatch dropped without returning error\"),","lines":1},{"ref":"src/client/conn/http2.rs:192","code":" 192 | Err(_) => panic!(\"dispatch dropped without returning error\"),","lines":1}]},{"id":"PANIC_8","type":"panic","type_label":"Panic","severity":"medium","easy":false,"description":"In SendWhen::poll, `this.call_back.take().expect(\"polled after complete\")` panics if the future is polled again after returning Poll::Ready. The struct stores the callback in an Option that is taken on every poll invocation. Once the future completes and returns Poll::Ready(()), call_back is None; a second poll (which is undefined behavior per the Future contract but still happens with some executors or select! misuse) will panic.","fix":"This is the standard approach for pinned futures, but adding a comment that this future must not be polled after completion would clarify intent. Alternatively, guard with an if-let instead of expect so the second poll is a no-op.","locations":[{"ref":"src/client/dispatch.rs:357","code":" 357 | let mut call_back = this.call_back.take().expect(\"polled after complete\");","lines":1}]},{"id":"PANIC_9","type":"panic","type_label":"Panic","severity":"medium","easy":false,"description":"In common/date.rs, CachedDate::write_str does a direct slice assignment `self.bytes[self.pos..self.pos + len]` with no bounds check. If the formatted HttpDate string is ever longer than DATE_VALUE_LENGTH (29 bytes), this will panic. The debug_assert at line 83 catches this only in debug builds. A non-standard system clock, a date far in the future (year >= 10000, which needs 30 chars), or an httpdate format change would trigger this panic in release builds.","fix":"Add a release-mode bounds check, or use write_to_slice with a proper error path, or assert that the formatted string fits before writing. Document the assumption that HttpDate is always exactly 29 bytes.","locations":[{"ref":"src/common/date.rs:99-103","code":" 99 | let len = s.len();\n 100 | self.bytes[self.pos..self.pos + len].copy_from_slice(s.as_bytes());\n 101 | self.pos += len;\n 102 | Ok(())\n 103 | }","lines":5},{"ref":"src/common/date.rs:83","code":" 83 | debug_assert_eq!(self.pos, DATE_VALUE_LENGTH);","lines":1}]},{"id":"PANIC_10","type":"panic","type_label":"Panic","severity":"medium","easy":false,"description":"In `Server::encode`, if the response `Version` is not HTTP/1.0, HTTP/1.1, or HTTP/2, the code panics with `panic!(\"unexpected response version: {:?}\", other)`. Similarly, in `Client::encode`, any version other than HTTP/1.0, HTTP/1.1, or HTTP/2 causes a panic. These panics can be triggered if a caller constructs a `MessageHead` with an exotic version (e.g., `Version::HTTP_09`) and passes it to the encoder. The panic path is reachable from external code.","fix":"Return `Err(crate::Error::new_user_unsupported_version())` (or similar) instead of panicking, so callers can handle the error gracefully.","locations":[{"ref":"src/proto/h1/role.rs:420-425","code":" 420 | Version::HTTP_2 => {\n 421 | debug!(\"response with HTTP2 version coerced to HTTP/1.1\");\n 422 | extend(dst, b\"HTTP/1.1 \");\n 423 | }\n 424 | other => panic!(\"unexpected response version: {:?}\", other),\n 425 | }","lines":6},{"ref":"src/proto/h1/role.rs:1211-1215","code":" 1211 | Version::HTTP_2 => {\n 1212 | debug!(\"request with HTTP2 version coerced to HTTP/1.1\");\n 1213 | extend(dst, b\"HTTP/1.1\");\n 1214 | }\n 1215 | other => panic!(\"unexpected request version: {:?}\", other),","lines":5}]},{"id":"PANIC_11","type":"panic","type_label":"Panic","severity":"medium","easy":false,"description":"In `ResponseFutMap::poll` (client.rs), after the response future resolves, the code calls `this.ping.take().expect(\"Future polled twice\")` and `this.send_stream.take().expect(\"Future polled twice\")`. Same problem as above - if the future is polled after completion, it panics. `ResponseFutMap` is used inside `SendWhen` which should prevent double-polling, but the panic is still a hard crash rather than a graceful failure.","fix":"Use option-based guards instead of `expect`.","locations":[{"ref":"src/proto/h2/client.rs:618-619","code":" 618 | let ping = this.ping.take().expect(\"Future polled twice\");\n 619 | let send_stream = this.send_stream.take().expect(\"Future polled twice\");","lines":2}]},{"id":"UAF_1","type":"use_after_free","type_label":"Use After Free","severity":"high","easy":false,"description":"In hyper_request_set_body, the body pointer is consumed via Box::from_raw before the req pointer is null-checked. If body is non-null but req is null, the function takes ownership of body (creating a Box), then returns HYPERE_INVALID_ARG, dropping the Box and freeing the body. The C caller receives an error code but the body has already been freed. If the C caller retries with the same body pointer (reasonable on error), it causes use-after-free. The ownership transfer on error is undocumented.","fix":"Check req for null before taking ownership of body. Either reorder the non_null! checks (check req first using &mut *req before Box::from_raw(body)), or document clearly that body is always consumed even on error.","locations":[{"ref":"src/ffi/http_types.rs:248-253","code":" 248 | fn hyper_request_set_body(req: *mut hyper_request, body: *mut hyper_body) -> hyper_code {\n 249 | let body = non_null!(Box::from_raw(body) ?= hyper_code::HYPERE_INVALID_ARG);\n 250 | let req = non_null!(&mut *req ?= hyper_code::HYPERE_INVALID_ARG);\n 251 | *req.0.body_mut() = body.0;\n 252 | hyper_code::HYPERE_OK\n 253 | }","lines":6}]},{"id":"MEM_1","type":"memory_leak","type_label":"Memory Leak","severity":"high","easy":false,"description":"hyper_buf_copy does not null-check the buf pointer before calling slice::from_raw_parts(buf, len). If buf is null and len > 0, this is undefined behavior. The ffi_fn! macro has '?= ptr::null_mut()' for panics but cannot catch UB from a null pointer dereference in from_raw_parts.","fix":"Add a null check at the start of hyper_buf_copy: 'if buf.is_null() { return ptr::null_mut(); }'.","locations":[{"ref":"src/ffi/body.rs:261-266","code":" 261 | fn hyper_buf_copy(buf: *const u8, len: size_t) -> *mut hyper_buf {\n 262 | let slice = unsafe {\n 263 | std::slice::from_raw_parts(buf, len)\n 264 | };\n 265 | Box::into_raw(Box::new(hyper_buf(Bytes::copy_from_slice(slice))))\n 266 | } ?= ptr::null_mut()","lines":6}]},{"id":"MEM_2","type":"memory_leak","type_label":"Memory Leak","severity":"high","easy":false,"description":"hyper_buf_bytes, hyper_buf_len, and hyper_buf_free dereference the buf pointer without checking for null. hyper_buf_bytes and hyper_buf_len use 'unsafe { (*buf).0... }' directly, while hyper_buf_free uses 'unsafe { Box::from_raw(buf) }'. A null pointer passed by a C caller causes undefined behavior (null pointer dereference). The ffi_fn! catch_unwind does not protect against UB - only panics. Compare with hyper_body_free which correctly uses non_null!.","fix":"Use the non_null! macro in hyper_buf_bytes, hyper_buf_len, and hyper_buf_free, just as hyper_body_free does: 'non_null!(&*buf ?= std::ptr::null())' etc.","locations":[{"ref":"src/ffi/body.rs:277-278","code":" 277 | fn hyper_buf_bytes(buf: *const hyper_buf) -> *const u8 {\n 278 | unsafe { (*buf).0.as_ptr() }","lines":2},{"ref":"src/ffi/body.rs:284-285","code":" 284 | fn hyper_buf_len(buf: *const hyper_buf) -> size_t {\n 285 | unsafe { (*buf).0.len() }","lines":2},{"ref":"src/ffi/body.rs:293-295","code":" 293 | fn hyper_buf_free(buf: *mut hyper_buf) {\n 294 | drop(unsafe { Box::from_raw(buf) });\n 295 | }","lines":3}]},{"id":"MEM_3","type":"memory_leak","type_label":"Memory Leak","severity":"high","easy":false,"description":"hyper_error_print does not null-check the dst pointer before calling slice::from_raw_parts_mut(dst, dst_len). If dst is null with dst_len > 0, this is undefined behavior. The err pointer is null-checked (via non_null!) but only after the unsafe slice creation from dst.","fix":"Add a null check for dst before calling slice::from_raw_parts_mut, or check dst.is_null() at the start and return 0.","locations":[{"ref":"src/ffi/error.rs:90-95","code":" 90 | fn hyper_error_print(err: *const hyper_error, dst: *mut u8, dst_len: size_t) -> size_t {\n 91 | let dst = unsafe {\n 92 | std::slice::from_raw_parts_mut(dst, dst_len)\n 93 | };\n 94 | non_null!(&*err ?= 0).print_to(dst)\n 95 | }","lines":6}]},{"id":"MEM_4","type":"memory_leak","type_label":"Memory Leak","severity":"high","easy":false,"description":"In hyper_request_set_method, slice::from_raw_parts(method, method_len) is called before the req null-check. If method is null with method_len > 0, this is UB. The same pattern appears in hyper_request_set_uri (line 141) and hyper_request_set_uri_parts (lines 176, 182, 188). Also, raw_name_value (called from hyper_headers_set and hyper_headers_add) does no null check on name or value before calling slice::from_raw_parts.","fix":"Add null checks for all data pointer arguments before constructing slices from them. For methods like raw_name_value, validate name and value are non-null at the start.","locations":[{"ref":"src/ffi/http_types.rs:107-111","code":" 107 | fn hyper_request_set_method(req: *mut hyper_request, method: *const u8, method_len: size_t) -> hyper_code {\n 108 | let bytes = unsafe {\n 109 | std::slice::from_raw_parts(method, method_len as usize)\n 110 | };\n 111 | let req = non_null!(&mut *req ?= hyper_code::HYPERE_INVALID_ARG);","lines":5},{"ref":"src/ffi/http_types.rs:139-143","code":" 139 | fn hyper_request_set_uri(req: *mut hyper_request, uri: *const u8, uri_len: size_t) -> hyper_code {\n 140 | let bytes = unsafe {\n 141 | std::slice::from_raw_parts(uri, uri_len as usize)\n 142 | };\n 143 | let req = non_null!(&mut *req ?= hyper_code::HYPERE_INVALID_ARG);","lines":5},{"ref":"src/ffi/http_types.rs:173-192","code":" 173 | let mut builder = Uri::builder();\n 174 | if !scheme.is_null() {\n 175 | let scheme_bytes = unsafe {\n 176 | std::slice::from_raw_parts(scheme, scheme_len as usize)\n 177 | };\n 178 | builder = builder.scheme(scheme_bytes);\n 179 | }\n 180 | if !authority.is_null() {\n 181 | let authority_bytes = unsafe {\n 182 | std::slice::from_raw_parts(authority, authority_len as usize)\n 183 | };\n 184 | builder = builder.authority(authority_bytes);\n 185 | }\n 186 | if !path_and_query.is_null() {\n 187 | let path_and_query_bytes = unsafe {\n 188 | std::slice::from_raw_parts(path_and_query, path_and_query_len as usize)\n 189 | };\n 190 | builder = builder.path_and_query(path_and_query_bytes);\n 191 | }\n 192 | let req = non_null!(&mut *req ?= hyper_code::HYPERE_INVALID_ARG);","lines":20},{"ref":"src/ffi/http_types.rs:559-578","code":" 559 | unsafe fn raw_name_value(\n 560 | name: *const u8,\n 561 | name_len: size_t,\n 562 | value: *const u8,\n 563 | value_len: size_t,\n 564 | ) -> Result<(HeaderName, HeaderValue, Bytes), hyper_code> {\n 565 | let name = std::slice::from_raw_parts(name, name_len);\n 566 | let orig_name = Bytes::copy_from_slice(name);\n 567 | let name = match HeaderName::from_bytes(name) {\n 568 | Ok(name) => name,\n 569 | Err(_) => return Err(hyper_code::HYPERE_INVALID_ARG),\n 570 | };\n 571 | let value = std::slice::from_raw_parts(value, value_len);\n 572 | let value = match HeaderValue::from_bytes(value) {\n 573 | Ok(val) => val,\n 574 | Err(_) => return Err(hyper_code::HYPERE_INVALID_ARG),\n 575 | };\n 576 | \n 577 | Ok((name, value, orig_name))\n 578 | }","lines":20}]},{"id":"OVF_1","type":"overflow_underflow","type_label":"Overflow Underflow","severity":"medium","easy":false,"description":"In `DecodedLength::sub_if` (length.rs line 73), the subtraction `*known -= amt` is an unchecked integer subtraction on a `u64`. If the accumulated body data exceeds the declared `Content-Length` (i.e. the server sends more bytes than it claimed), `*known` would underflow and wrap around to a very large value. This would cause `size_hint()` to report a wildly incorrect body size, and subsequent underflow checks that rely on `DecodedLength` comparison with `CHUNKED`/`CLOSE_DELIMITED` sentinels (values near `u64::MAX`) would be corrupted.","fix":"Use `known.saturating_sub(amt)` or add a debug_assert/check that `amt <= *known` before subtracting. At minimum add a `debug_assert!(*known >= amt)` to catch this in tests.","locations":[{"ref":"src/body/length.rs:69-76","code":" 69 | pub(crate) fn sub_if(&mut self, amt: u64) {\n 70 | match *self {\n 71 | DecodedLength::CHUNKED | DecodedLength::CLOSE_DELIMITED => (),\n 72 | DecodedLength(ref mut known) => {\n 73 | *known -= amt;\n 74 | }\n 75 | }\n 76 | }","lines":8}]},{"id":"LOGIC_1","type":"logic_error","type_label":"Logic Error","severity":"high","easy":false,"description":"In hyper_io::poll_read, when the C callback returns ok bytes read, buf.advance(ok) is called with no check that ok <= buf.remaining(). If the C callback lies and returns a count larger than the buffer size, advance() will set buf.filled beyond buf.raw.len(). A subsequent call to as_mut() (which slices raw[filled..]) will then panic with an out-of-bounds index. That panic propagates through FuturesUnordered, poisons the executor's driver Mutex, and permanently breaks the executor (every subsequent poll_next call panics via panic_if_poisoned, which is caught by ffi_fn! returning null_mut()).","fix":"After receiving ok from the C callback, validate that ok <= buf.remaining() before calling buf.advance(ok). Return an error if it is out of range.","locations":[{"ref":"src/ffi/io.rs:159-163","code":" 159 | ok => {\n 160 | // We have to trust that the user's read callback actually\n 161 | // filled in that many bytes... :(\n 162 | unsafe { buf.advance(ok) };\n 163 | Poll::Ready(Ok(()))","lines":5},{"ref":"src/rt/io.rs:341-343","code":" 341 | pub unsafe fn advance(&mut self, n: usize) {\n 342 | self.buf.filled = self.buf.filled.checked_add(n).expect(\"overflow\");\n 343 | self.buf.init = self.buf.filled.max(self.buf.init);","lines":3}]},{"id":"LOGIC_2","type":"logic_error","type_label":"Logic Error","severity":"high","easy":false,"description":"In `ChunkedState::read_body` (decode.rs lines 483-511), after reading bytes, `*rem -= count as u64` is performed. If `count > usize::MAX` as u64 (impossible on current platforms since count is a usize), this would underflow. However, the real issue is that `to_read = usize::try_from(*rem).unwrap_or(usize::MAX)` caps the request at usize::MAX, but if the underlying `read_mem` returns MORE bytes than requested (which is allowed by the `MemRead` trait contract if the implementation is buggy), then `count as u64 > *rem` could be true and the subtraction `*remaining -= num` in the Length decoder (lines 158-166) handles this with a branch. However in `read_body`, the subtraction on line 504 `*rem -= count as u64` has NO guard: if `count as u64 > *rem`, this underflows and wraps (in release mode) or panics (in debug mode). The check `if count == 0` on line 496 only catches the EOF case.","fix":"Add a guard: `if count as u64 > *rem { *rem = 0; } else { *rem -= count as u64; }` mirroring the Length decoder's pattern at lines 158-160. While the `read_mem` contract should prevent count > to_read, defensive coding is warranted here for correctness.","locations":[{"ref":"src/proto/h1/decode.rs:483-511","code":" 483 | fn read_body<R: MemRead>(\n 484 | cx: &mut Context<'_>,\n 485 | rdr: &mut R,\n 486 | rem: &mut u64,\n 487 | buf: &mut Option<Bytes>,\n 488 | ) -> Poll<Result<ChunkedState, io::Error>> {\n 489 | trace!(\"Chunked read, remaining={:?}\", rem);\n 490 | \n 491 | // cap remaining bytes at the max capacity of usize\n 492 | let to_read = usize::try_from(*rem).unwrap_or(usize::MAX);\n 493 | let slice = ready!(rdr.read_mem(cx, to_read))?;\n 494 | let count = slice.len();\n 495 | \n 496 | if count == 0 {\n 497 | *rem = 0;\n 498 | return Poll::Ready(Err(io::Error::new(\n 499 | io::ErrorKind::UnexpectedEof,\n 500 | IncompleteBody,\n 501 | )));\n 502 | }\n 503 | *buf = Some(slice);\n 504 | *rem -= count as u64;\n 505 | \n 506 | if *rem > 0 {\n 507 | Poll::Ready(Ok(ChunkedState::Body))\n 508 | } else {\n 509 | Poll::Ready(Ok(ChunkedState::BodyCr))\n 510 | }\n 511 | }","lines":29}]},{"id":"LOGIC_3","type":"logic_error","type_label":"Logic Error","severity":"high","easy":false,"description":"In `decode_trailers` (decode.rs), when duplicate trailer headers are present, `HeaderMap::insert` is used instead of `HeaderMap::append`. `insert` silently overwrites any previous value for the same header name, so multiple trailer entries with the same name are collapsed to the last one. RFC 7230 permits multiple trailer fields with the same name; the previous values are silently lost, which can cause data loss for multi-valued trailer headers such as `Set-Cookie`.","fix":"Use `trailers.append(name, value)` instead of `trailers.insert(name, value)` to preserve all values for repeated trailer field names.","locations":[{"ref":"src/proto/h1/decode.rs:674","code":" 674 | trailers.insert(name, value);","lines":1}]},{"id":"LOGIC_4","type":"logic_error","type_label":"Logic Error","severity":"high","easy":false,"description":"In `server.rs` `poll_server`, when a CONNECT request has a non-zero Content-Length, the code resets the stream and then returns `Poll::Ready(Ok(()))` (line 293). This causes `poll_server` to return success even though a protocol error was encountered - it terminates the entire server connection accept loop as if it completed gracefully, rather than continuing to accept new streams. All subsequent client requests on this H2 connection are silently dropped.","fix":"Instead of `return Poll::Ready(Ok(()))`, log the error and `continue` the loop (accepting the next stream). The stream was already reset via `respond.send_reset`, so connection is intact. Only the individual stream should be aborted, not the server loop.","locations":[{"ref":"src/proto/h2/server.rs:290-294","code":" 290 | if content_length.map_or(false, |len| len != 0) {\n 291 | warn!(\"h2 connect request with non-zero body not supported\");\n 292 | respond.send_reset(h2::Reason::INTERNAL_ERROR);\n 293 | return Poll::Ready(Ok(()));\n 294 | }","lines":5}]},{"id":"LOGIC_5","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In Sender::can_send (HTTP/1 dispatch), after the channel's receiver has been polled once and set buffered_once=true, additional sends are permanently rejected even after the receiver becomes ready again (giver.give() returns true). Looking at the test 'sender_checks_for_want_on_send' (lines 459-467): after the receiver processes item 1 and then wants again, tx.try_send(Custom(2)) is still rejected with 'expect_err(\"2 still not ready\")'. The intent is correct per the test, but it means once the one buffered slot is used, all subsequent sends require the giver to be in the 'wanting' state. However `can_send` returns `true` if `giver.give()` succeeds regardless of `buffered_once`. The comment says 'one message is always allowed', but the boolean is never reset to false when the giver says it's ready, so it is actually providing no benefit after the first buffered send - the logic degenerates to just using the giver. The `buffered_once` flag is set but never cleared, making the buffering truly a one-time thing for the entire channel lifetime, not per-roundtrip as the comment suggests.","fix":"If the intent is to allow one buffer per roundtrip (i.e., one un-acked message at a time), buffered_once should be reset to false when giver.give() succeeds. Document whether one-time-ever or per-roundtrip is intended.","locations":[{"ref":"src/client/dispatch.rs:93-104","code":" 93 | fn can_send(&mut self) -> bool {\n 94 | if self.giver.give() || !self.buffered_once {\n 95 | // If the receiver is ready *now*, then of course we can send.\n 96 | //\n 97 | // If the receiver isn't ready yet, but we don't have anything\n 98 | // in the channel yet, then allow one message.\n 99 | self.buffered_once = true;\n 100 | true\n 101 | } else {\n 102 | false\n 103 | }\n 104 | }","lines":12}]},{"id":"LOGIC_6","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In common/buf.rs BufList::advance, when cnt equals the remaining bytes of the front buffer exactly, the code calls `front.advance(rem)` followed by `self.bufs.pop_front()`. However, the pop happens after the inner scope releases the borrow of front. This is structurally correct in Rust, but the issue is different: if bufs is empty and advance is called with cnt > 0, `self.bufs[0]` will panic with an index out of bounds. The Buf trait requires callers to not advance past remaining(), but there's no assertion or graceful handling here.","fix":"Add a debug_assert at the top of advance: `debug_assert!(cnt <= self.remaining())` to catch misuse early and document the precondition.","locations":[{"ref":"src/common/buf.rs:41-55","code":" 41 | fn advance(&mut self, mut cnt: usize) {\n 42 | while cnt > 0 {\n 43 | {\n 44 | let front = &mut self.bufs[0];\n 45 | let rem = front.remaining();\n 46 | if rem > cnt {\n 47 | front.advance(cnt);\n 48 | return;\n 49 | } else {\n 50 | front.advance(rem);\n 51 | cnt -= rem;\n 52 | }\n 53 | }\n 54 | self.bufs.pop_front();\n 55 | }","lines":15}]},{"id":"LOGIC_7","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In hyper_headers_foreach when iterating in the ordered path, if headers.headers.get_all(name).iter().nth(*idx) returns None (indicating desync between orig_order and headers), the function silently returns without notifying the caller. The comment says 'something has gone wrong' but provides no error signal. The callback is not invoked for remaining headers, so the C caller receives a silently truncated header list.","fix":"Consider returning an error or calling the callback with whatever data is available. At minimum, document that iteration may stop early without any indication to the caller.","locations":[{"ref":"src/ffi/http_types.rs:473-479","code":" 473 | if let Some(value) = headers.headers.get_all(name).iter().nth(*idx) {\n 474 | val_ptr = value.as_bytes().as_ptr();\n 475 | val_len = value.as_bytes().len();\n 476 | } else {\n 477 | // Stop iterating, something has gone wrong.\n 478 | return;\n 479 | }","lines":7}]},{"id":"LOGIC_8","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"The IO sentinel values HYPER_IO_PENDING (0xFFFFFFFF = 4294967295) and HYPER_IO_ERROR (0xFFFFFFFE = 4294967294) are 32-bit values but size_t is usize (64-bit on 64-bit platforms). A C callback that legitimately reads or writes exactly 4294967295 bytes will be misinterpreted as PENDING. While extremely unlikely in practice, the values should be SIZE_MAX and SIZE_MAX-1 respectively to be safe on all platforms. The C header exposes them as integer literals (not (size_t)~0), making the collision theoretically possible.","fix":"Define HYPER_IO_PENDING as (size_t)(-1) and HYPER_IO_ERROR as (size_t)(-2) in the C header, and update the Rust constants to usize::MAX and usize::MAX - 1.","locations":[{"ref":"src/ffi/io.rs:11-14","code":" 11 | pub const HYPER_IO_PENDING: size_t = 0xFFFFFFFF;\n 12 | /// Sentinel value to return from a read or write callback that the operation\n 13 | /// has errored.\n 14 | pub const HYPER_IO_ERROR: size_t = 0xFFFFFFFE;","lines":4},{"ref":"capi/include/hyper.h:48-53","code":" 48 | #define HYPER_IO_PENDING 4294967295\n 49 | \n 50 | /*\n 51 | Sentinel value to return from a read or write callback that the operation\n 52 | */\n 53 | #define HYPER_IO_ERROR 4294967294","lines":6}]},{"id":"LOGIC_9","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In hyper_io poll_read, when the user's C read callback returns 0 (which happens with the default read_noop), buf.advance(0) is called and Poll::Ready(Ok(())) is returned. Per the Write trait docs, returning Poll::Ready(Ok(())) with no bytes advanced signals EOF to hyper. This means a hyper_io object created with hyper_io_new() but without setting a read callback will immediately signal EOF when hyper attempts to read, causing an unexpected EOF error. Similarly, write_noop returns 0 which means Poll::Ready(Ok(0)) from poll_write, signaling the transport can no longer accept bytes.","fix":"read_noop should return HYPER_IO_PENDING (not 0) to indicate no data yet available, and write_noop should also return HYPER_IO_PENDING. This avoids spurious EOF/closed signals on unconfigured transports.","locations":[{"ref":"src/ffi/io.rs:124-142","code":" 124 | /// cbindgen:ignore\n 125 | extern \"C\" fn read_noop(\n 126 | _userdata: *mut c_void,\n 127 | _: *mut hyper_context<'_>,\n 128 | _buf: *mut u8,\n 129 | _buf_len: size_t,\n 130 | ) -> size_t {\n 131 | 0\n 132 | }\n 133 | \n 134 | /// cbindgen:ignore\n 135 | extern \"C\" fn write_noop(\n 136 | _userdata: *mut c_void,\n 137 | _: *mut hyper_context<'_>,\n 138 | _buf: *const u8,\n 139 | _buf_len: size_t,\n 140 | ) -> size_t {\n 141 | 0\n 142 | }","lines":19},{"ref":"src/ffi/io.rs:159-165","code":" 159 | ok => {\n 160 | // We have to trust that the user's read callback actually\n 161 | // filled in that many bytes... :(\n 162 | unsafe { buf.advance(ok) };\n 163 | Poll::Ready(Ok(()))\n 164 | }\n 165 | }","lines":7}]},{"id":"LOGIC_10","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"HYPER_POLL_ERROR is defined as 3, skipping value 2. The constants are HYPER_POLL_READY=0, HYPER_POLL_PENDING=1, (gap at 2), HYPER_POLL_ERROR=3. In UserBody::poll_data, any return value other than 0, 1, or 3 falls to the 'unexpected' error arm. A C caller accidentally returning 2 (which might seem like a reasonable constant given the sequence) gets a body write error rather than any meaningful diagnostic. This gap is undocumented in the constants.","fix":"Either assign HYPER_POLL_ERROR = 2 to close the gap, or add a comment explaining why value 2 is intentionally skipped.","locations":[{"ref":"src/ffi/task.rs:22-29","code":" 22 | pub const HYPER_POLL_READY: c_int = 0;\n 23 | /// Return in a poll function to indicate it is still pending.\n 24 | ///\n 25 | /// The passed in `hyper_waker` should be registered to wake up the task at\n 26 | /// some later point.\n 27 | pub const HYPER_POLL_PENDING: c_int = 1;\n 28 | /// Return in a poll function indicate an error.\n 29 | pub const HYPER_POLL_ERROR: c_int = 3;","lines":8},{"ref":"src/ffi/body.rs:217-233","code":" 217 | super::task::HYPER_POLL_READY => {\n 218 | if out.is_null() {\n 219 | Poll::Ready(None)\n 220 | } else {\n 221 | let buf = unsafe { Box::from_raw(out) };\n 222 | Poll::Ready(Some(Ok(Frame::data(buf.0))))\n 223 | }\n 224 | }\n 225 | super::task::HYPER_POLL_PENDING => Poll::Pending,\n 226 | super::task::HYPER_POLL_ERROR => {\n 227 | Poll::Ready(Some(Err(crate::Error::new_body_write_aborted())))\n 228 | }\n 229 | unexpected => Poll::Ready(Some(Err(crate::Error::new_body_write(format!(\n 230 | \"unexpected hyper_body_data_func return code {}\",\n 231 | unexpected\n 232 | ))))),\n 233 | }","lines":17}]},{"id":"LOGIC_11","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"hyper_context is defined as 'pub struct hyper_context<'a>(Context<'a>)' without #[repr(transparent)]. The wrap() function performs 'std::mem::transmute::<&mut Context<'_>, &mut hyper_context<'_>>(cx)' relying on the newtype having the same layout as Context. Without repr(transparent), the Rust reference does not guarantee the layout of a single-field struct matches its field. This is technically undefined behavior even though it works in practice with today's compiler.","fix":"Add #[repr(transparent)] to the hyper_context struct definition to formally guarantee layout compatibility.","locations":[{"ref":"src/ffi/task.rs:131","code":" 131 | pub struct hyper_context<'a>(Context<'a>);","lines":1},{"ref":"src/ffi/task.rs:513-517","code":" 513 | impl hyper_context<'_> {\n 514 | pub(crate) fn wrap<'a, 'b>(cx: &'a mut Context<'b>) -> &'a mut hyper_context<'b> {\n 515 | // A struct with only one field has the same layout as that field.\n 516 | unsafe { std::mem::transmute::<&mut Context<'_>, &mut hyper_context<'_>>(cx) }\n 517 | }","lines":5}]},{"id":"LOGIC_12","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `Conn::poll_read_head`, the `allow_trailer_fields` flag is computed after reading the TE header from the *request* headers. The code checks `TE: trailers` to decide whether the *server's response* may include trailers (RFC 7230 section 4.3 - a client advertises trailer support via TE). However, the flag is set unconditionally on every call to `poll_read_head`, so if a subsequent request on the same keep-alive connection does NOT send `TE: trailers`, the flag from the previous request's headers is overwritten. This is correct. But there is a subtle issue: when the header is absent, `get(TE)` returns `None`, and `map_or(false, ...)` returns `false`, so `allow_trailer_fields` is reset to false. This appears correct for the keep-alive case but the logic is not clearly documented and is easy to get wrong if the connection handling changes.","fix":"Add a comment explicitly stating why `allow_trailer_fields` is reset to false on each request parse, to prevent a future developer from changing `map_or(false,...)` to `map_or(true,...)` thinking it is more permissive.","locations":[{"ref":"src/proto/h1/conn.rs:328-333","code":" 328 | self.state.allow_trailer_fields = msg\n 329 | .head\n 330 | .headers\n 331 | .get(TE)\n 332 | .map_or(false, |te_header| te_header == \"trailers\");\n 333 | ","lines":6}]},{"id":"LOGIC_13","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `Conn::write_trailers` (conn.rs lines 729-752), when `encode_trailers` returns `None` (e.g. because the allowed trailer set is empty or trailers are not supported by the encoder), the writing state remains as `Writing::Body` and the encoder is not advanced. This means subsequent calls to `write_body` or `end_body` will still succeed, but the trailers are silently dropped without any error or warning to the caller. The caller has no way to distinguish 'trailers were buffered' from 'trailers were silently discarded'.","fix":"Return a `Result` from `write_trailers` or log a more visible warning when trailers are silently dropped so callers can detect the situation.","locations":[{"ref":"src/proto/h1/conn.rs:729-752","code":" 729 | pub(crate) fn write_trailers(&mut self, trailers: HeaderMap) {\n 730 | if T::is_server() && !self.state.allow_trailer_fields {\n 731 | debug!(\"trailers not allowed to be sent\");\n 732 | return;\n 733 | }\n 734 | debug_assert!(self.can_write_body() && self.can_buffer_body());\n 735 | \n 736 | match self.state.writing {\n 737 | Writing::Body(ref encoder) => {\n 738 | if let Some(enc_buf) =\n 739 | encoder.encode_trailers(trailers, self.state.title_case_headers)\n 740 | {\n 741 | self.io.buffer(enc_buf);\n 742 | \n 743 | self.state.writing = if encoder.is_last() || encoder.is_close_delimited() {\n 744 | Writing::Closed\n 745 | } else {\n 746 | Writing::KeepAlive\n 747 | };\n 748 | }\n 749 | }\n 750 | _ => unreachable!(\"write_trailers invalid state: {:?}\", self.state.writing),\n 751 | }\n 752 | }","lines":24}]},{"id":"LOGIC_14","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `ChunkedState::read_trailer_lf`, the counter check `if *trailers_cnt >= h1_max_headers` fires BEFORE the `\\n` byte is appended and BEFORE `*trailers_cnt += 1`, so the limit enforced is `h1_max_headers - 1` successful trailers, not `h1_max_headers`. Concretely, if `h1_max_headers = 10` the 10th trailer header (index 9, i.e. when `trailers_cnt == 9 >= 10` is false) will be accepted, but the 11th will cause the `trailers_cnt == 10 >= 10` check to fire. However, in `poll` in the Chunked arm, the same check `if *trailers_cnt >= h1_max_headers` is applied again. This means a message with exactly `h1_max_headers` trailers is parsed correctly but is then rejected at the outer check in `poll`. This is an off-by-one: the outer guard in `decode` is redundant and confusingly inverts the intent.","fix":"Either remove the redundant check in `poll` (lines 205-209), or change the check in `read_trailer_lf` to `> h1_max_headers` to make the two consistent and match documented semantics.","locations":[{"ref":"src/proto/h1/decode.rs:569-574","code":" 569 | if *trailers_cnt >= h1_max_headers {\n 570 | return Poll::Ready(Err(io::Error::new(\n 571 | io::ErrorKind::InvalidData,\n 572 | \"chunk trailers count overflow\",\n 573 | )));\n 574 | }","lines":6},{"ref":"src/proto/h1/decode.rs:205-209","code":" 205 | if *trailers_cnt >= h1_max_headers {\n 206 | return Poll::Ready(Err(io::Error::new(\n 207 | io::ErrorKind::InvalidData,\n 208 | \"chunk trailers count overflow\",\n 209 | )));","lines":5}]},{"id":"LOGIC_15","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `ChunkedState::read_end_cr` (decode.rs lines 592-622), when a non-`\\r` byte is encountered in the `EndCr` state, the byte is treated as the start of a new trailer field and the state transitions to `Trailer`. The first byte of the field is appended to `trailers_buf`. However, if `trailers_buf` is `None`, a new `BytesMut` is created with capacity 64. There is no check against `h1_max_header_size` for the initial byte written into the newly created `trailers_buf` at line 611 (`buf.put_u8(byte)`). The `put_u8!` macro (which enforces the limit) is only used in the `Some(ref mut trailers_buf)` branch (line 615) but NOT for the freshly allocated `buf` on line 611. This means the first byte of a trailer in this path bypasses the size limit check.","fix":"Apply the same limit check when writing the first byte into a newly allocated `trailers_buf`. Use the `put_u8!` macro or an equivalent check after line 611.","locations":[{"ref":"src/proto/h1/decode.rs:607-619","code":" 607 | match trailers_buf {\n 608 | None => {\n 609 | // 64 will fit a single Expires header without reallocating\n 610 | let mut buf = BytesMut::with_capacity(64);\n 611 | buf.put_u8(byte);\n 612 | *trailers_buf = Some(buf);\n 613 | }\n 614 | Some(ref mut trailers_buf) => {\n 615 | put_u8!(trailers_buf, byte, h1_max_header_size);\n 616 | }\n 617 | }\n 618 | \n 619 | Poll::Ready(Ok(ChunkedState::Trailer))","lines":13}]},{"id":"LOGIC_16","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `Dispatcher::poll_write` (dispatch.rs lines 353-370), when `body.is_end_stream()` is detected after `poll_frame` returns a data frame, `write_body_and_end` is called. However, `write_body_and_end` calls `encoder.encode_and_end` which, for a `Length` encoder, returns `can_keep_alive = false` if the written bytes are less than the remaining content-length. This is correct behavior. But a subtle issue arises: if `is_end_stream()` returns true but the body's `poll_frame` later yields more frames (which is a contract violation from the body), the dispatcher has already set `*clear_body = true` and will not poll the body again, silently discarding remaining data.","fix":"This is a body contract violation from the user's code, but it may be worth adding a debug assertion or trace log when additional frames are available after `is_end_stream()` returned true.","locations":[{"ref":"src/proto/h1/dispatch.rs:401-411","code":" 401 | if frame.is_data() {\n 402 | let chunk = frame.into_data().unwrap_or_else(|_| unreachable!());\n 403 | let eos = body.is_end_stream();\n 404 | if eos {\n 405 | *clear_body = true;\n 406 | if chunk.remaining() == 0 {\n 407 | trace!(\"discarding empty chunk\");\n 408 | self.conn.end_body()?;\n 409 | } else {\n 410 | self.conn.write_body_and_end(chunk);\n 411 | }","lines":11}]},{"id":"LOGIC_17","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `Server::parse` (role.rs lines 265-280), when multiple `Transfer-Encoding` headers are present, only the LAST one is used to decide `is_te_chunked`. The iteration replaces `is_te_chunked` on each encounter: `is_te_chunked = headers::is_chunked_(&value)`. This means `Transfer-Encoding: chunked, gzip` followed by `Transfer-Encoding: compress` would result in `is_te_chunked = false` (since `compress` is not chunked), causing the server to return a 400 error. But the correct behavior per RFC 7230 section 3.3.3 is to check whether `chunked` is the FINAL encoding in the concatenated list. The current code does check `is_te && !is_te_chunked` at line 338, but only based on the last header encountered, which may miss multi-header sequences where the final encoding is `chunked` but not present in the last header.","fix":"Track the last transfer-encoding value across all Transfer-Encoding headers, not just each header individually. Concatenate all TE values in order and check whether `chunked` is the final element in the combined list.","locations":[{"ref":"src/proto/h1/role.rs:265-280","code":" 265 | header::TRANSFER_ENCODING => {\n 266 | // https://tools.ietf.org/html/rfc7230#section-3.3.3\n 267 | // If Transfer-Encoding header is present, and 'chunked' is\n 268 | // not the final encoding, and this is a Request, then it is\n 269 | // malformed. A server should respond with 400 Bad Request.\n 270 | if !is_http_11 {\n 271 | debug!(\"HTTP/1.0 cannot have Transfer-Encoding header\");\n 272 | return Err(Parse::transfer_encoding_unexpected());\n 273 | }\n 274 | is_te = true;\n 275 | if headers::is_chunked_(&value) {\n 276 | is_te_chunked = true;\n 277 | decoder = DecodedLength::CHUNKED;\n 278 | } else {\n 279 | is_te_chunked = false;\n 280 | }","lines":16},{"ref":"src/proto/h1/role.rs:338-341","code":" 338 | if is_te && !is_te_chunked {\n 339 | debug!(\"request with transfer-encoding header, but not chunked, bad request\");\n 340 | return Err(Parse::transfer_encoding_invalid());\n 341 | }","lines":4}]},{"id":"LOGIC_18","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `Server::encode_headers`, when a `Content-Length: N` header is encountered and `msg.body` is `None` (no body), and `msg.req_method` is NOT `HEAD`, the code emits a warning when value != b\"0\" but then executes `continue 'headers` WITHOUT writing the header to `dst` and WITHOUT setting `wrote_len = true`. This means a non-zero Content-Length on a body-less response is silently dropped. The `wrote_len` flag is also not set, so the encoder will later write `content-length: 0` automatically (via the `can_have_implicit_zero_content_length` path). The caller supplied a non-zero Content-Length but it is silently replaced with 0. Callers expect their headers to be forwarded, or at minimum to receive an error.","fix":"Either return an error when a non-zero Content-Length is provided with no body (other than for HEAD), or document that the header will be replaced.","locations":[{"ref":"src/proto/h1/role.rs:774-793","code":" 774 | None => {\n 775 | // We have no body to actually send,\n 776 | // but the headers claim a content-length.\n 777 | // There's only 2 ways this makes sense:\n 778 | //\n 779 | // - The header says the length is `0`.\n 780 | // - This is a response to a `HEAD` request.\n 781 | if msg.req_method == &Some(Method::HEAD) {\n 782 | debug_assert_eq!(encoder, Encoder::length(0));\n 783 | } else {\n 784 | if value.as_bytes() != b\"0\" {\n 785 | warn!(\n 786 | \"content-length value found, but empty body provided: {:?}\",\n 787 | value\n 788 | );\n 789 | }\n 790 | continue 'headers;\n 791 | }\n 792 | }\n 793 | }","lines":20}]},{"id":"LOGIC_19","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `Server::encode_headers`, the `Trailer` header handling (lines 849-889) checks `Server::can_chunked(...)` to decide whether trailers are allowed. If the check fails, the header is dropped via `continue`. However, if the check passes and the header is written to `dst`, the code still processes the `allowed_trailer_fields` list. If the `Transfer-Encoding` header is subsequently found to be missing or non-chunked (so `encoder` ends up as `Encoder::length(...)` rather than `Encoder::chunked()`), the `allowed_trailer_fields` list is accumulated but then checked only at line 976-979 via `if encoder.is_chunked()`. This means the `Trailer` header itself is written into the response headers even if the response ultimately uses a length-based encoder, advertising trailers that will never be sent. The HTTP response will contain a `Trailer:` header even when the body is not chunked.","fix":"Only write the Trailer header to `dst` after it is confirmed that the encoder will be chunked, or defer writing it to the final encoder selection phase.","locations":[{"ref":"src/proto/h1/role.rs:849-890","code":" 849 | header::TRAILER => {\n 850 | // check that we actually can send a chunked body...\n 851 | if msg.head.version == Version::HTTP_10\n 852 | || !Server::can_chunked(msg.req_method.as_ref(), msg.head.subject)\n 853 | {\n 854 | continue;\n 855 | }\n 856 | \n 857 | if !is_name_written {\n 858 | is_name_written = true;\n 859 | header_name_writer.write_header_name_with_colon(\n 860 | dst,\n 861 | \"trailer: \",\n 862 | header::TRAILER,\n 863 | );\n 864 | extend(dst, value.as_bytes());\n 865 | } else {\n 866 | extend(dst, b\", \");\n 867 | extend(dst, value.as_bytes());\n 868 | }\n 869 | \n 870 | // Parse the Trailer header value into HeaderNames.\n 871 | // The value may contain comma-separated names.\n 872 | // HeaderName normalizes to lowercase for case-insensitive matching.\n 873 | if let Ok(value_str) = value.to_str() {\n 874 | let names: Vec<HeaderName> = value_str\n 875 | .split(',')\n 876 | .filter_map(|s| HeaderName::from_bytes(s.trim().as_bytes()).ok())\n 877 | .collect();\n 878 | \n 879 | match allowed_trailer_fields {\n 880 | Some(ref mut fields) => {\n 881 | fields.extend(names);\n 882 | }\n 883 | None => {\n 884 | allowed_trailer_fields = Some(names);\n 885 | }\n 886 | }\n 887 | }\n 888 | \n 889 | continue 'headers;\n 890 | }","lines":42},{"ref":"src/proto/h1/role.rs:976-979","code":" 976 | if encoder.is_chunked() {\n 977 | if let Some(allowed_trailer_fields) = allowed_trailer_fields {\n 978 | encoder = encoder.into_chunked_with_trailing_fields(allowed_trailer_fields);\n 979 | }","lines":4}]},{"id":"LOGIC_20","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `Server::encode_headers` (role.rs), the `Trailer` header processing at lines 873-887 parses the `Trailer` header VALUE into `HeaderName`s by splitting on commas. If a Trailer value contains an invalid header name (e.g., due to whitespace or invalid characters), `HeaderName::from_bytes(...).ok()` silently drops it via `filter_map`. The trailer name is then never included in `allowed_trailer_fields`, so when the client tries to send a declared trailer, it will be silently dropped by `encode_trailers`. There is no warning or error reported to the caller when a declared trailer name is invalid.","fix":"Log a warning when a Trailer header value entry fails to parse as a valid HeaderName, so operators can detect misconfiguration.","locations":[{"ref":"src/proto/h1/role.rs:873-887","code":" 873 | if let Ok(value_str) = value.to_str() {\n 874 | let names: Vec<HeaderName> = value_str\n 875 | .split(',')\n 876 | .filter_map(|s| HeaderName::from_bytes(s.trim().as_bytes()).ok())\n 877 | .collect();\n 878 | \n 879 | match allowed_trailer_fields {\n 880 | Some(ref mut fields) => {\n 881 | fields.extend(names);\n 882 | }\n 883 | None => {\n 884 | allowed_trailer_fields = Some(names);\n 885 | }\n 886 | }\n 887 | }","lines":15}]},{"id":"LOGIC_21","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `poll_pipe` (client.rs lines 533-563), when `!f.is_connect && !f.eos`, a `PipeToSendStream` is created and eagerly polled once. If the first poll returns `Poll::Ready(_)`, the pipe is dropped without executing `drop(conn_drop_ref)` and `drop(ping)`. The `conn_drop_ref` and `ping` were not moved into `PipeMap` (since we never created one), and were not dropped either - they are still held by `self` (the `ClientTask`). The `conn_drop_ref` is `self.conn_drop_ref.clone()` (line 542), so the clone is just dropped when `pipe` is dropped at the end of the `match` arm. The `ping` clone from line 527 (`let ping = self.ping.clone()`) is also dropped at the end of `poll_pipe`. So both are correctly dropped. However, the `cancel_tx` created at line 531 is dropped without sending, while `cancel_rx` was never moved anywhere (we never entered the `Poll::Pending` branch). This means the cancel channel leaks without being consumed, but since both ends are dropped immediately this is benign.","fix":"This is benign but confusing. Add a comment explaining that when the pipe completes eagerly, the `cancel_rx` is simply dropped along with `cancel_tx` since no `PipeMap` was spawned.","locations":[{"ref":"src/proto/h2/client.rs:526-578","code":" 526 | fn poll_pipe(&mut self, f: FutCtx<B>, cx: &mut Context<'_>) {\n 527 | let ping = self.ping.clone();\n 528 | \n 529 | // A one-shot channel so that send_task can tell pipe_task to\n 530 | // reset the stream when the client cancels the request.\n 531 | let (cancel_tx, cancel_rx) = oneshot::channel::<()>();\n 532 | \n 533 | let send_stream = if !f.is_connect {\n 534 | if !f.eos {\n 535 | let mut pipe = PipeToSendStream::new(f.body, f.body_tx);\n 536 | \n 537 | // eagerly see if the body pipe is ready and\n 538 | // can thus skip allocating in the executor\n 539 | match Pin::new(&mut pipe).poll(cx) {\n 540 | Poll::Ready(_) => (),\n 541 | Poll::Pending => {\n 542 | let conn_drop_ref = self.conn_drop_ref.clone();\n 543 | // keep the ping recorder's knowledge of an\n 544 | // \"open stream\" alive while this body is\n 545 | // still sending...\n 546 | let ping = ping.clone();\n 547 | \n 548 | let pipe = PipeMap {\n 549 | pipe,\n 550 | conn_drop_ref: Some(conn_drop_ref),\n 551 | ping: Some(ping),\n 552 | cancel_rx: Some(cancel_rx),\n 553 | };\n 554 | // Clear send task\n 555 | self.executor\n 556 | .execute_h2_future(H2ClientFuture::Pipe { pipe });\n 557 | }\n 558 | }\n 559 | }\n 560 | \n 561 | None\n 562 | } else {\n 563 | Some(f.body_tx)\n 564 | };\n 565 | \n 566 | self.executor.execute_h2_future(H2ClientFuture::Send {\n 567 | send_when: SendWhen {\n 568 | when: ResponseFutMap {\n 569 | fut: f.fut,\n 570 | ping: Some(ping),\n 571 | send_stream: Some(send_stream),\n 572 | exec: self.executor.clone(),\n 573 | cancel_tx: Some(cancel_tx),\n 574 | },\n 575 | call_back: Some(f.cb),\n 576 | },\n 577 | });\n 578 | }","lines":53}]},{"id":"LOGIC_22","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `PipeToSendStream::poll` (mod.rs), the code polls `me.body_tx.poll_reset(cx)` on every iteration of the outer loop to detect RST_STREAM. When `buffered_data` is `Some` and we are in the inner capacity-waiting loop, the outer RST_STREAM check is only done at the top of the outer loop. After `break`ing out of the inner capacity loop, the code immediately sends the buffered data without re-checking for RST_STREAM. If RST_STREAM arrives while waiting for capacity, it will be missed until the next outer loop iteration (after the send). This means one extra `send_data` call may be made to a reset stream, which the h2 library should handle gracefully with an error, but the RST_STREAM handling is delayed.","fix":"After breaking from the capacity loop (line 175), re-poll `poll_reset` before calling `send_data` on the buffered chunk.","locations":[{"ref":"src/proto/h2/mod.rs:160-187","code":" 160 | if me.buffered_data.is_some() {\n 161 | while me.body_tx.capacity() == 0 {\n 162 | match ready!(me.body_tx.poll_capacity(cx)) {\n 163 | Some(Ok(0)) => {}\n 164 | Some(Ok(_)) => break,\n 165 | Some(Err(e)) => return Poll::Ready(Err(crate::Error::new_body_write(e))),\n 166 | None => {\n 167 | // None means the stream is no longer in a\n 168 | // streaming state, we either finished it\n 169 | // somehow, or the remote reset us.\n 170 | return Poll::Ready(Err(crate::Error::new_body_write(\n 171 | \"send stream capacity unexpectedly closed\",\n 172 | )));\n 173 | }\n 174 | }\n 175 | }\n 176 | \n 177 | let peeked = me.buffered_data.take().expect(\"checked is_some above\");\n 178 | let buf = SendBuf::Buf(peeked.data);\n 179 | me.body_tx\n 180 | .send_data(buf, peeked.is_eos)\n 181 | .map_err(crate::Error::new_body_write)?;\n 182 | \n 183 | if peeked.is_eos {\n 184 | return Poll::Ready(Ok(()));\n 185 | }\n 186 | continue;\n 187 | }","lines":28}]},{"id":"LOGIC_23","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `Ponger::poll` (ping.rs), when `poll_pong` returns `Poll::Ready(Ok(_pong))` for a BDP ping, the keep-alive state is updated by calling `ka.maybe_schedule(is_idle, &locked)` and `ka.maybe_ping(cx, is_idle, &mut locked)` (lines 291-293). However, `is_idle` was computed at the start of `poll()` before the pong was received. If receiving the pong indicates that a stream just closed (reducing strong_count), the `is_idle` value used for rescheduling the keep-alive is stale. This is a minor timing issue - the keep-alive may be (re)scheduled with a stale idle state.","fix":"Recompute `is_idle` before calling `maybe_schedule`/`maybe_ping` after receiving the pong, or document that a one-poll lag is acceptable.","locations":[{"ref":"src/proto/h2/ping.rs:266-269","code":" 266 | pub(super) fn poll(&mut self, cx: &mut task::Context<'_>) -> Poll<Ponged> {\n 267 | let mut locked = self.shared.lock().panic_if_poisoned();\n 268 | let now = locked.timer.now(); // hoping this is fine to move within the lock\n 269 | let is_idle = self.is_idle();","lines":4},{"ref":"src/proto/h2/ping.rs:290-293","code":" 290 | if let Some(ref mut ka) = self.keep_alive {\n 291 | locked.update_last_read_at();\n 292 | ka.maybe_schedule(is_idle, &locked);\n 293 | ka.maybe_ping(cx, is_idle, &mut locked);","lines":4}]},{"id":"LOGIC_24","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `Ponger::is_idle` (ping.rs), the connection is considered idle when `Arc::strong_count(&self.shared) <= 2`. The count of 2 represents `Ponger` itself plus one `Recorder`. However, if ping is disabled for the body stream (e.g. `Recorder::for_stream` returns `disabled()` when stream is already EOS), the count can drop to 1 (only `Ponger`), which is still `<= 2` and treated as idle - that is correct. But if multiple streams are in flight, each has its own cloned `Recorder` (strong_count increases), so the idle check does work correctly. The concern is: the comment says 'Arc::strong_count <= 2' implies idle, but if somehow there were 2 real recorders plus Ponger (count=3), it would incorrectly be considered not-idle even with 0 open streams, since recorders can outlive streams. This is a minor over-counting scenario but the heuristic is fragile.","fix":"Document the idle-detection heuristic's limitations. Consider tracking open stream count explicitly for more accurate idle detection.","locations":[{"ref":"src/proto/h2/ping.rs:326-328","code":" 326 | fn is_idle(&self) -> bool {\n 327 | Arc::strong_count(&self.shared) <= 2\n 328 | }","lines":3}]},{"id":"LOGIC_25","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `KeepAlive::maybe_ping` (ping.rs), when the sleep timer fires in `Scheduled` state, there is a check `if !self.while_idle && is_idle` that causes the function to return without sending a ping and without transitioning state back to `Init`. The state remains `KeepAliveState::Scheduled(at)` with an expired timer, but no new scheduling occurs. On the next poll, `maybe_schedule` sees `Scheduled(..)` state and does nothing (line 447: `KeepAliveState::Scheduled(..) => ()`). This means that after a while-idle=false connection goes idle after the timer fires, keep-alive never resumes even when a new stream starts (making the connection no longer idle).","fix":"When `!self.while_idle && is_idle` and the timer has fired, transition to `KeepAliveState::Init` so that `maybe_schedule` will reschedule when activity resumes.","locations":[{"ref":"src/proto/h2/ping.rs:469-472","code":" 469 | if !self.while_idle && is_idle {\n 470 | trace!(\"keep-alive no need to ping when idle and while_idle=false\");\n 471 | return;\n 472 | }","lines":4},{"ref":"src/proto/h2/ping.rs:447","code":" 447 | KeepAliveState::Scheduled(..) => (),","lines":1}]},{"id":"LOGIC_26","type":"logic_error","type_label":"Logic Error","severity":"medium","easy":false,"description":"In `H2Stream::poll2` (server.rs), the CONNECT success handling checks for a non-zero Content-Length header and resets the stream. But then after the `if res.status().is_success()` block, execution falls through to the general body-sending code (line 518: `if !body.is_end_stream()`). For a CONNECT response that fails (non-success status), the response is sent through the general path. However, `connect_parts` was taken from the Option at line 488, so if the service returned a non-success response to CONNECT, `connect_parts` is gone and the upgrade never happens - this is correct. But the CONNECT case does not strip the body from the response: a non-success CONNECT response's body is piped through as a normal response body. This is technically valid but could lead to unexpected behavior if the upgrade machinery expects a clean close.","fix":"Document that non-success CONNECT responses have their bodies streamed normally. Consider whether the body should be forcibly empty for non-success CONNECT responses per the HTTP/2 spec.","locations":[{"ref":"src/proto/h2/server.rs:488-531","code":" 488 | if let Some(connect_parts) = connect_parts.take() {\n 489 | if res.status().is_success() {\n 490 | if headers::content_length_parse_all(res.headers())\n 491 | .map_or(false, |len| len != 0)\n 492 | {\n 493 | warn!(\"h2 successful response to CONNECT request with body not supported\");\n 494 | me.reply.send_reset(h2::Reason::INTERNAL_ERROR);\n 495 | return Poll::Ready(Err(crate::Error::new_user_header()));\n 496 | }\n 497 | if res\n 498 | .headers_mut()\n 499 | .remove(::http::header::CONTENT_LENGTH)\n 500 | .is_some()\n 501 | {\n 502 | warn!(\"successful response to CONNECT request disallows content-length header\");\n 503 | }\n 504 | let send_stream = reply!(me, res, false);\n 505 | let (h2_up, up_task) = super::upgrade::pair(\n 506 | send_stream,\n 507 | connect_parts.recv_stream,\n 508 | connect_parts.ping,\n 509 | );\n 510 | connect_parts\n 511 | .pending\n 512 | .fulfill(Upgraded::new(h2_up, Bytes::new()));\n 513 | self.exec.execute_upgrade(up_task);\n 514 | return Poll::Ready(Ok(()));\n 515 | }\n 516 | }\n 517 | \n 518 | if !body.is_end_stream() {\n 519 | // automatically set Content-Length from body...\n 520 | if let Some(len) = body.size_hint().exact() {\n 521 | headers::set_content_length_if_missing(res.headers_mut(), len);\n 522 | }\n 523 | \n 524 | let body_tx = reply!(me, res, false);\n 525 | H2StreamState::Body {\n 526 | pipe: PipeToSendStream::new(body, body_tx),\n 527 | }\n 528 | } else {\n 529 | reply!(me, res, true);\n 530 | return Poll::Ready(Ok(()));\n 531 | }","lines":44}]},{"id":"LOGIC_27","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"In common/time.rs, Time::now() returns Instant::now() when Time::Empty, but Time::reset() panics when Time::Empty. This asymmetry means that code which calls now() to compute a deadline and then reset() to set that deadline will panic at reset() if no timer was configured, even though now() silently succeeded. The panic message 'You must supply a timer.' is helpful, but the inconsistency makes it easy to configure partial timer usage incorrectly.","fix":"Either panic in now() as well when Time::Empty (with a clear message), or make reset() work with a no-op when Time::Empty. Document the contract for each method clearly.","locations":[{"ref":"src/common/time.rs:53-58","code":" 53 | pub(crate) fn now(&self) -> Instant {\n 54 | match *self {\n 55 | Time::Empty => Instant::now(),\n 56 | Time::Timer(ref t) => t.now(),\n 57 | }\n 58 | }","lines":6},{"ref":"src/common/time.rs:60-67","code":" 60 | pub(crate) fn reset(&self, sleep: &mut Pin<Box<dyn Sleep>>, new_deadline: Instant) {\n 61 | match *self {\n 62 | Time::Empty => {\n 63 | panic!(\"You must supply a timer.\")\n 64 | }\n 65 | Time::Timer(ref t) => t.reset(sleep, new_deadline),\n 66 | }\n 67 | }","lines":8}]},{"id":"LOGIC_28","type":"logic_error","type_label":"Logic Error","severity":"low","easy":true,"description":"In common/watch.rs, Sender::send only wakes the receiver waker if the value actually changed (swap returns a different value). This is correct for deduplication. However, when Sender is dropped, Drop calls self.send(CLOSED). If the previous value was already CLOSED (e.g., send(CLOSED) was called manually before drop), the waker is NOT notified. Any pending Receiver::load would never wake up. This is a silent livelock if something manually closes the channel before the Sender is dropped.","fix":"In Drop::drop, unconditionally wake the waker after setting CLOSED, or ensure that CLOSED is never set twice in normal usage paths and document that constraint.","locations":[{"ref":"src/common/watch.rs:51-55","code":" 51 | pub(crate) fn send(&mut self, value: Value) {\n 52 | if self.shared.value.swap(value, Ordering::SeqCst) != value {\n 53 | self.shared.waker.wake();\n 54 | }\n 55 | }","lines":5},{"ref":"src/common/watch.rs:58-61","code":" 58 | impl Drop for Sender {\n 59 | fn drop(&mut self) {\n 60 | self.send(CLOSED);\n 61 | }","lines":4}]},{"id":"LOGIC_29","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"In ext/mod.rs, OriginalHeaderOrder::insert only records a header name the first time it is seen, discarding subsequent inserts of the same name (the early return comment says 'Replacing an already existing element does not change ordering'). However, HeaderCaseMap::append always appends, creating a mismatch in semantics. If a header appears multiple times and the first insertion is via `insert` and subsequent ones via `insert`, the count in num_entries stays at 1 and entry_order only has one entry, but the underlying HeaderMap (used via HeaderCaseMap) may have multiple values. This can produce incorrect index lookups when iterating get_in_order.","fix":"Clarify whether insert is semantically 'upsert first occurrence only' or 'always append'. If it's the former, document it clearly. If callers need to track all occurrences, use append instead of insert, consistent with HeaderCaseMap.","locations":[{"ref":"src/ext/mod.rs:225-234","code":" 225 | pub(crate) fn insert(&mut self, name: HeaderName) {\n 226 | if !self.num_entries.contains_key(&name) {\n 227 | let idx = 0;\n 228 | self.num_entries.insert(name.clone(), 1);\n 229 | self.entry_order.push((name, idx));\n 230 | }\n 231 | // Replacing an already existing element does not\n 232 | // change ordering, so we only care if its the first\n 233 | // header name encountered\n 234 | }","lines":10}]},{"id":"LOGIC_30","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"In ext/mod.rs, OriginalHeaderOrder::append computes an idx that represents the zero-based occurrence count among previously seen headers with the same name, but the value inserted in the Vacant arm is 1 (the new count after insertion), while the idx used is 0 (correct for the first occurrence). However for the Occupied arm: idx = *entry.get() gives the current count before incrementing, then increments. So for the second occurrence, idx = 1, meaning 'use the second value (index 1) from get_all()'. This is off by one: for the first occurrence, idx = 0 (correct); for the second, idx = 1 (correct); but the semantics depend on the order in which get_all() returns values. The code appears correct but is fragile and the Vacant arm sets the initial count to 1 instead of 0, which works only because the first occurrence uses idx = 0 before inserting.","fix":"Add a comment explaining the indexing scheme explicitly. The Vacant arm inserts 1 (correct - it's the count after this append), and idx = 0 is the first index into get_all() results. This works but is non-obvious.","locations":[{"ref":"src/ext/mod.rs:241-252","code":" 241 | let idx = match self.num_entries.entry(name.clone()) {\n 242 | std::collections::hash_map::Entry::Occupied(mut entry) => {\n 243 | let idx = *entry.get();\n 244 | *entry.get_mut() += 1;\n 245 | idx\n 246 | }\n 247 | std::collections::hash_map::Entry::Vacant(entry) => {\n 248 | entry.insert(1);\n 249 | 0\n 250 | }\n 251 | };\n 252 | self.entry_order.push((name, idx));","lines":12}]},{"id":"LOGIC_31","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"hyper_task_value uses 'p == std::ptr::NonNull::<c_void>::dangling().as_ptr()' to detect a ZST output (the () type). NonNull::<c_void>::dangling() returns an address equal to align_of::<c_void>() which is 1. This works for the standard allocator (Box::into_raw for ZSTs produces dangling()) but could fail for custom allocators or if a real allocation happens to land at address 1. The type information should be used instead.","fix":"Check 'val.as_task_type() == hyper_task_return_type::HYPER_TASK_EMPTY' before Box::into_raw, and return null_mut() in that case instead of relying on the dangling pointer address.","locations":[{"ref":"src/ffi/task.rs:417-425","code":" 417 | if let Some(val) = task.output.take() {\n 418 | let p = Box::into_raw(val).cast();\n 419 | // protect from returning fake pointers to empty types\n 420 | if p == std::ptr::NonNull::<c_void>::dangling().as_ptr() {\n 421 | ptr::null_mut()\n 422 | } else {\n 423 | p\n 424 | }\n 425 | } else {","lines":9}]},{"id":"LOGIC_32","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"In `Conn::poll_read_head` (conn.rs lines 311-325), when `msg.expect_continue` is true but `msg.head.version` is exactly HTTP/1.0 (not `> HTTP_10`), the `else` branch is taken and `Reading::Body` is set directly without sending a 100 Continue. The condition `msg.expect_continue && msg.head.version.gt(&Version::HTTP_10)` means HTTP/1.0 requests with `Expect: 100-continue` silently get their body decoded without sending 100 Continue. RFC 2616 says servers SHOULD NOT send 100 Continue to HTTP/1.0 clients, so this behavior is correct. However, the code comment at line 311 does not explain this edge case, making it appear to be a bug when reviewing.","fix":"Add a comment stating that for HTTP/1.0 clients, `Expect: 100-continue` is intentionally ignored per RFC 2616 and the body decoder is started directly.","locations":[{"ref":"src/proto/h1/conn.rs:311-326","code":" 311 | } else if msg.expect_continue && msg.head.version.gt(&Version::HTTP_10) {\n 312 | let h1_max_header_size = None; // TODO: remove this when we land h1_max_header_size support\n 313 | self.state.reading = Reading::Continue(Decoder::new(\n 314 | msg.decode,\n 315 | self.state.h1_max_headers,\n 316 | h1_max_header_size,\n 317 | ));\n 318 | wants = wants.add(Wants::EXPECT);\n 319 | } else {\n 320 | let h1_max_header_size = None; // TODO: remove this when we land h1_max_header_size support\n 321 | self.state.reading = Reading::Body(Decoder::new(\n 322 | msg.decode,\n 323 | self.state.h1_max_headers,\n 324 | h1_max_header_size,\n 325 | ));\n 326 | }","lines":16}]},{"id":"LOGIC_33","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"In `Conn::try_keep_alive` / `State::try_keep_alive` (conn.rs lines 1072-1091), when both reading and writing are `KeepAlive`, the code calls `self.idle::<T>()` only if `keep_alive == KA::Busy`. If it's `KA::Disabled`, it calls `self.close()`. However, `KA::Idle` at this point would be unexpected (it means we're already idle but somehow have KeepAlive states on both sides), and the match arm `_ => ()` silently ignores it. If `KA::Idle` is somehow reached here, the connection would be stuck with both sides in `KeepAlive` state indefinitely.","fix":"Add a debug assertion `debug_assert!(!matches!(self.keep_alive.status(), KA::Idle), ...)` in the `(&Reading::KeepAlive, &Writing::KeepAlive)` arm to catch this impossible state during development.","locations":[{"ref":"src/proto/h1/conn.rs:1072-1091","code":" 1072 | fn try_keep_alive<T: Http1Transaction>(&mut self) {\n 1073 | match (&self.reading, &self.writing) {\n 1074 | (&Reading::KeepAlive, &Writing::KeepAlive) => {\n 1075 | if let KA::Busy = self.keep_alive.status() {\n 1076 | self.idle::<T>();\n 1077 | } else {\n 1078 | trace!(\n 1079 | \"try_keep_alive({}): could keep-alive, but status = {:?}\",\n 1080 | T::LOG,\n 1081 | self.keep_alive\n 1082 | );\n 1083 | self.close();\n 1084 | }\n 1085 | }\n 1086 | (&Reading::Closed, &Writing::KeepAlive) | (&Reading::KeepAlive, &Writing::Closed) => {\n 1087 | self.close()\n 1088 | }\n 1089 | _ => (),\n 1090 | }\n 1091 | }","lines":20}]},{"id":"LOGIC_34","type":"logic_error","type_label":"Logic Error","severity":"low","easy":true,"description":"In `Dispatcher::poll_read_head` (dispatch.rs line 317), the code has `debug_assert!(!upgrade.is_none(), \"empty upgrade\")`. The method `is_none()` on `OnUpgrade` returns `true` when there is no pending upgrade. The assertion intent is 'upgrade should not be empty/none', but the expression `!upgrade.is_none()` means 'upgrade is Some'. This is correct logically but the double-negative is confusing; `debug_assert!(upgrade.is_some())` or `debug_assert!(!upgrade.is_none())` are equivalent, but the latter is idiomatic in Rust and should be rewritten for clarity.","fix":"Replace `debug_assert!(!upgrade.is_none(), \"empty upgrade\")` with `debug_assert!(!upgrade.is_none(), \"expected non-empty upgrade\")` - or check what `OnUpgrade::is_none()` actually means since it is a custom type; if it has a different meaning than `Option::is_none`, document it.","locations":[{"ref":"src/proto/h1/dispatch.rs:317","code":" 317 | debug_assert!(!upgrade.is_none(), \"empty upgrade\");","lines":1}]},{"id":"LOGIC_35","type":"logic_error","type_label":"Logic Error","severity":"low","easy":true,"description":"The `Http1Transaction` trait has a mutual-recursion between `is_client()` and `is_server()`: `is_client()` returns `!Self::is_server()` and `is_server()` returns `!Self::is_client()`. Neither provides a base case in the trait itself. Any type that implements the trait and does NOT override at least one of these methods will cause infinite mutual recursion at runtime and stack overflow. The concrete `Client` and `Server` types each override one of these methods, but the default implementations are dangerous.","fix":"Change the default implementations so one of them returns a concrete value (e.g. `is_server()` defaults to `false`) or mark the pair as requiring at least one to be overridden via documentation. Alternatively, provide only one method in the trait and derive the other.","locations":[{"ref":"src/proto/h1/mod.rs:40-46","code":" 40 | fn is_client() -> bool {\n 41 | !Self::is_server()\n 42 | }\n 43 | \n 44 | fn is_server() -> bool {\n 45 | !Self::is_client()\n 46 | }","lines":7}]},{"id":"LOGIC_36","type":"logic_error","type_label":"Logic Error","severity":"low","easy":true,"description":"In `Server::encode` (role.rs line 392-399), when `msg.head.subject.is_informational()` (1xx other than 101), the code sets `msg.body = None` and then returns `Err(...)` at line 469. But before returning the error, the status line has already been partially written to `dst` (lines 415-445). The `rewind` closure is NOT called, so `dst` contains a partial 500 Internal Server Error status line (since `msg.head.subject` was set to INTERNAL_SERVER_ERROR at line 394 before the write loop). Actually looking more carefully: the truncation at `dst.truncate(orig_len)` only happens inside `rewind`, which is only called from inside `encode_headers_with_lower_case` / `encode_headers_with_original_case` when they error. But the 1xx error path sets up the status 500, writes the status line, and eventually calls `encode_headers_with_*`. Inside `encode_headers`, there is no early return before the headers are written, so the status line IS written. When `ret` (which is `Err`) is returned at the end via `ret.map(|()| encoder)`, the caller sees an error but `dst` contains a valid 500 response. This may be intentional (send the 500 and also return an error to the caller), but is confusing and undocumented.","fix":"Add a comment clarifying that a 500 response is intentionally written to `dst` before returning the error, or add a `rewind(dst); return Err(...)` path to suppress the write when that is the desired behavior.","locations":[{"ref":"src/proto/h1/role.rs:391-399","code":" 391 | } else if msg.head.subject.is_informational() {\n 392 | warn!(\"response with 1xx status code not supported\");\n 393 | *msg.head = MessageHead::default();\n 394 | msg.head.subject = StatusCode::INTERNAL_SERVER_ERROR;\n 395 | msg.body = None;\n 396 | (Err(crate::Error::new_user_unsupported_status_code()), true)\n 397 | } else {\n 398 | (Ok(()), !msg.keep_alive)\n 399 | };","lines":9},{"ref":"src/proto/h1/role.rs:469","code":" 469 | ret.map(|()| encoder)","lines":1}]},{"id":"LOGIC_37","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"In `Client::decoder` (role.rs lines 1266-1314), the method number 200-299 range check for CONNECT at line 1282 uses a range pattern `200..=299`, which is correct. However, the decoder for `Method::CONNECT` only returns `DecodedLength::ZERO` for 2xx responses. For CONNECT + non-2xx, the code falls through to the Transfer-Encoding/Content-Length checks. But RFC 7230 does not define body semantics for non-2xx CONNECT responses at all; the fallthrough behavior (read until EOF for responses without C-L or TE) may be reasonable but is not explicitly documented.","fix":"Add a comment explaining that non-2xx CONNECT responses fall through to normal body decoding rules, as this is intentional.","locations":[{"ref":"src/proto/h1/role.rs:1280-1285","code":" 1280 | }\n 1281 | Some(Method::CONNECT) => {\n 1282 | if let 200..=299 = inc.subject.as_u16() {\n 1283 | return Ok(Some((DecodedLength::ZERO, true)));\n 1284 | }\n 1285 | }","lines":6}]},{"id":"LOGIC_38","type":"logic_error","type_label":"Logic Error","severity":"low","easy":true,"description":"In `Client::set_length` (role.rs lines 1388-1403), for HTTP/1.1 requests with `BodyLength::Unknown` and no existing Transfer-Encoding header, the code returns `Encoder::length(0)` for GET, HEAD, and CONNECT methods. This is documented behavior but is subtly incorrect for POST/PUT/DELETE/etc. when `BodyLength::Unknown` is returned by a body that actually has no data - the chunked encoder will send `0\\r\\n\\r\\n` unnecessarily. This is a minor performance issue, not a bug per se, but worth noting.","fix":"Document explicitly that `BodyLength::Unknown` always forces chunked for non-GET/HEAD/CONNECT methods, even if the body ends up being empty.","locations":[{"ref":"src/proto/h1/role.rs:1388-1403","code":" 1388 | } else if let BodyLength::Unknown = body {\n 1389 | // GET, HEAD, and CONNECT almost never have bodies.\n 1390 | //\n 1391 | // So instead of sending a \"chunked\" body with a 0-chunk,\n 1392 | // assume no body here. If you *must* send a body,\n 1393 | // set the headers explicitly.\n 1394 | match head.subject.0 {\n 1395 | Method::GET | Method::HEAD | Method::CONNECT => Some(Encoder::length(0)),\n 1396 | _ => {\n 1397 | te.insert(HeaderValue::from_static(\"chunked\"));\n 1398 | Some(Encoder::chunked())\n 1399 | }\n 1400 | }\n 1401 | } else {\n 1402 | None\n 1403 | }","lines":16}]},{"id":"LOGIC_39","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"In `Bdp::calculate` (ping.rs), the BDP window update formula uses `(bytes * 2).min(BDP_LIMIT) as WindowSize` where `WindowSize = u32` and `BDP_LIMIT = 16 * 1024 * 1024 = 16_777_216`. Since `bytes` is a `usize`, if `bytes > usize::MAX / 2` (which would require receiving >4GB in a single BDP measurement window - extremely unlikely but theoretically possible on 32-bit targets), `bytes * 2` would overflow. On 64-bit targets the `.min(BDP_LIMIT)` clamps it safely, but on 32-bit targets `usize` is 32-bit so overflow is possible with >2GB received.","fix":"Use `bytes.saturating_mul(2).min(BDP_LIMIT) as WindowSize` to prevent potential overflow on 32-bit targets.","locations":[{"ref":"src/proto/h2/ping.rs:398-399","code":" 398 | if bytes >= self.bdp as usize * 2 / 3 {\n 399 | self.bdp = (bytes * 2).min(BDP_LIMIT) as WindowSize;","lines":2}]},{"id":"LOGIC_40","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"In `KeepAlive::schedule` (ping.rs line 452), the scheduled time is `shared.last_read_at() + self.interval`. This schedules the next ping for `interval` time after the last frame was received. However, `last_read_at` is always updated to `now` when any frame is received. If the keep-alive interval is, say, 30 seconds, and the last frame arrived 5 seconds ago, the timer is set for 25 seconds from now (correct). But `schedule` is also called from `maybe_schedule` when in `PingSent` state and `!shared.is_ping_sent()` (meaning the pong was received) - at that point `last_read_at` was already updated by `update_last_read_at()` in the pong handler (line 291). So this is correct. However, `schedule` is also called from `Init` state during `maybe_schedule` even when `last_read_at` is stale (never received any frame since connection). In this case `last_read_at` is `Some(now_at_creation)`, which may cause the first ping to fire later than expected if the connection was idle for a long time before first use.","fix":"This is a minor timing issue. Document that the initial keep-alive timer is based on connection creation time, so the first ping after a dormant period may be delayed by up to `interval` extra time.","locations":[{"ref":"src/proto/h2/ping.rs:451-455","code":" 451 | fn schedule(&mut self, shared: &Shared) {\n 452 | let interval = shared.last_read_at() + self.interval;\n 453 | self.state = KeepAliveState::Scheduled(interval);\n 454 | self.timer.reset(&mut self.sleep, interval);\n 455 | }","lines":5},{"ref":"src/proto/h2/ping.rs:72-73","code":" 72 | let last_read_at = keep_alive.as_ref().map(|_| now);\n 73 | ","lines":2}]},{"id":"LOGIC_41","type":"logic_error","type_label":"Logic Error","severity":"low","easy":false,"description":"In `UpgradedSendStreamTask::tick` (upgrade.rs), the code calls `me.h2_tx.reserve_capacity(1)` at the top of every loop iteration before checking if capacity is zero. If capacity is already nonzero from a previous iteration, `reserve_capacity(1)` is a no-op (since the requested amount is less than what's available). But if the prior send consumed some capacity and there's still leftover capacity, calling `reserve_capacity(1)` when we already have capacity >0 skips the capacity check block. The comment says 'reserve 1 byte to make sure there is some capacity available', but after sending data the capacity tracking in h2 is decremented. The concern is: `reserve_capacity(1)` is called even when we have pending capacity from a prior `reserve_capacity` call that wasn't fully consumed, potentially leading to unnecessary h2 WINDOW_UPDATE frames being triggered for trivial 1-byte reservations.","fix":"Only call `reserve_capacity(1)` if current capacity is 0, or document why it's safe to call unconditionally.","locations":[{"ref":"src/proto/h2/upgrade.rs:79-101","code":" 79 | me.h2_tx.reserve_capacity(1);\n 80 | \n 81 | if me.h2_tx.capacity() == 0 {\n 82 | // poll_capacity oddly needs a loop\n 83 | 'capacity: loop {\n 84 | match me.h2_tx.poll_capacity(cx) {\n 85 | Poll::Ready(Some(Ok(0))) => {}\n 86 | Poll::Ready(Some(Ok(_))) => break,\n 87 | Poll::Ready(Some(Err(e))) => {\n 88 | return Poll::Ready(Err(crate::Error::new_body_write(e)))\n 89 | }\n 90 | Poll::Ready(None) => {\n 91 | // None means the stream is no longer in a\n 92 | // streaming state, we either finished it\n 93 | // somehow, or the remote reset us.\n 94 | return Poll::Ready(Err(crate::Error::new_body_write(\n 95 | \"send stream capacity unexpectedly closed\",\n 96 | )));\n 97 | }\n 98 | Poll::Pending => break 'capacity,\n 99 | }\n 100 | }\n 101 | }","lines":23}]},{"id":"ERR_1","type":"incorrect_error_handling","type_label":"Incorrect Error Handling","severity":"medium","easy":false,"description":"In `Conn::on_read_head_error` (conn.rs lines 337-361), when `on_parse_error` returns `Ok(())`, the function returns `Poll::Pending` with the comment `// XXX: wat?`. This is semantically incorrect: the connection has encountered a parse error, written an error response, but returns `Poll::Pending` to the caller, which will cause the driver to wait for more I/O events before progressing. The error response that was written to the write buffer will only be flushed when the write side is next polled. However, the connection is in an inconsistent state (read closed, write has pending error response) and returning `Pending` may cause the caller to miss flushing the response if it never polls again. The comment admits this is suspicious behavior.","fix":"After writing an error response, immediately trigger a flush and return an appropriate `Poll::Ready` or ensure the caller will re-poll the write side. The `Poll::Pending` here may leave the error response undelivered if the future is dropped.","locations":[{"ref":"src/proto/h1/conn.rs:337-361","code":" 337 | fn on_read_head_error<Z>(&mut self, e: crate::Error) -> Poll<Option<crate::Result<Z>>> {\n 338 | // If we are currently waiting on a message, then an empty\n 339 | // message should be reported as an error. If not, it is just\n 340 | // the connection closing gracefully.\n 341 | let must_error = self.should_error_on_eof();\n 342 | self.close_read();\n 343 | self.io.consume_leading_lines();\n 344 | let was_mid_parse = e.is_parse() || !self.io.read_buf().is_empty();\n 345 | if was_mid_parse || must_error {\n 346 | // We check if the buf contains the h2 Preface\n 347 | debug!(\n 348 | \"parse error ({}) with {} bytes\",\n 349 | e,\n 350 | self.io.read_buf().len()\n 351 | );\n 352 | match self.on_parse_error(e) {\n 353 | Ok(()) => Poll::Pending, // XXX: wat?\n 354 | Err(e) => Poll::Ready(Some(Err(e))),\n 355 | }\n 356 | } else {\n 357 | debug!(\"read eof\");\n 358 | self.close_write();\n 359 | Poll::Ready(None)\n 360 | }\n 361 | }","lines":25}]},{"id":"ERR_2","type":"incorrect_error_handling","type_label":"Incorrect Error Handling","severity":"medium","easy":false,"description":"In `server.rs` `poll_server`, when `poll_accept` returns `Some(Err(e))`, the function returns `Poll::Ready(Err(crate::Error::new_h2(e)))`, which exits `poll_server` with an error. In `Server::poll`, this error propagates through `ready!()` and returns from the top-level future as an error. However, the `closing` field is never set in this path - `srv.closing` is only set when... it is never actually set in the code shown. The `closing` field exists and `poll_closed` is called only in the `self.closing.is_some()` branch (line 338-345), but there is no code path that sets `self.closing`. This means `poll_closed` is dead code and `self.closing.take().expect(\"polled after error\")` at line 345 could never be reached (it's unreachable code, though harmless).","fix":"Either remove the dead `closing`/`poll_closed` code path, or add the missing code that sets `srv.closing` when a service error occurs. This appears to be a leftover from refactoring.","locations":[{"ref":"src/proto/h2/server.rs:338-345","code":" 338 | debug_assert!(\n 339 | self.closing.is_some(),\n 340 | \"poll_server broke loop without closing\"\n 341 | );\n 342 | \n 343 | ready!(self.conn.poll_closed(cx).map_err(crate::Error::new_h2))?;\n 344 | \n 345 | Poll::Ready(Err(self.closing.take().expect(\"polled after error\")))","lines":8}]},{"id":"ERR_3","type":"incorrect_error_handling","type_label":"Incorrect Error Handling","severity":"medium","easy":false,"description":"In `H2Upgraded::poll_write` (upgrade.rs), when `send_stream.tx.start_send` fails (because the task dropped), the code polls `error_rx` to check for a task error. However, `error_rx` is polled without any prior `poll_ready`, and importantly the `cx` waker used here was registered by the caller for write readiness, not for error channel readiness. If `error_rx` returns `Poll::Pending`, the function returns `Poll::Pending`, but the waker may never be woken for the error channel specifically - the write caller will be woken only when write capacity is available, potentially causing a deadlock where the write is stuck pending forever because the task dropped without sending an error.","fix":"When `start_send` fails and `error_rx` returns `Poll::Pending`, consider returning `BrokenPipe` immediately rather than `Pending`, since the channel is already closed (task dropped).","locations":[{"ref":"src/proto/h2/upgrade.rs:222-235","code":" 222 | match self.send_stream.tx.start_send(Cursor::new(buf.into())) {\n 223 | Ok(()) => Poll::Ready(Ok(n)),\n 224 | Err(_task_dropped) => {\n 225 | // if the task dropped, check if there was an error\n 226 | // otherwise i guess its a broken pipe\n 227 | match Pin::new(&mut self.send_stream.error_rx).poll(cx) {\n 228 | Poll::Ready(Ok(reason)) => Poll::Ready(Err(io_error(reason))),\n 229 | Poll::Ready(Err(_task_dropped)) => {\n 230 | Poll::Ready(Err(std::io::ErrorKind::BrokenPipe.into()))\n 231 | }\n 232 | Poll::Pending => Poll::Pending,\n 233 | }\n 234 | }\n 235 | }","lines":14}]},{"id":"ERR_4","type":"incorrect_error_handling","type_label":"Incorrect Error Handling","severity":"low","easy":false,"description":"In `H2Upgraded::poll_read` (upgrade.rs), when `recv_stream.poll_data` returns `Some(Err(e))` with reason `CANCEL`, the function returns `Poll::Ready(Ok(()))`, treating it as a clean EOF. RFC 9113 section 8.1 says CANCEL means the stream was reset by the sender. Returning `Ok(())` (EOF) to the reader when the stream was abnormally cancelled may cause the application to treat a truncated response as complete, potentially leading to data corruption if the reader doesn't verify the full content was received.","fix":"Consider returning a broken-pipe or connection-reset error for `CANCEL` reason rather than treating it as a clean EOF, or at minimum document why CANCEL is treated as clean EOF in the upgrade context.","locations":[{"ref":"src/proto/h2/upgrade.rs:176-178","code":" 176 | return Poll::Ready(match e.reason() {\n 177 | Some(Reason::NO_ERROR) | Some(Reason::CANCEL) => Ok(()),\n 178 | Some(Reason::STREAM_CLOSED) => {","lines":3}]},{"id":"PERF_1","type":"performance","type_label":"Performance","severity":"medium","easy":false,"description":"In `Client::encode` (role.rs line 1233), `msg.head.headers.clear()` is called with the comment `//TODO: remove when switching to drain()`. The Server's `encode_headers` uses `drain()` on the headers, which avoids cloning and is more efficient. The client path still uses `clear()` after writing headers with an immutable reference, which is less efficient as it requires an extra pass over the header map.","fix":"Switch the client encoder to use `drain()` like the server encoder, which would also simplify the code and eliminate the need for the `//TODO` comment.","locations":[{"ref":"src/proto/h1/role.rs:1233","code":" 1233 | msg.head.headers.clear(); //TODO: remove when switching to drain()","lines":1}]},{"id":"PERF_2","type":"performance","type_label":"Performance","severity":"low","easy":false,"description":"In common/buf.rs, BufList::remaining() calls iter().map(|buf| buf.remaining()).sum() which is O(n) in the number of buffers. This is called by the Buf trait implementation and may be called frequently by callers (e.g., in the copy_to_bytes fallback path at line 85 which calls self.remaining() for an assertion after already consuming data). Caching the total remaining count and updating it incrementally in push() and advance() would make this O(1).","fix":"Add a cached `total: usize` field to BufList, increment in push(), decrement in advance(), and return it from remaining(). This is especially important if BufList is used in hot write paths.","locations":[{"ref":"src/common/buf.rs:31-33","code":" 31 | fn remaining(&self) -> usize {\n 32 | self.bufs.iter().map(|buf| buf.remaining()).sum()\n 33 | }","lines":3}]},{"id":"PERF_3","type":"performance","type_label":"Performance","severity":"low","easy":true,"description":"In common/date.rs, the buffer() method returns the full 29-byte slice regardless of how many bytes were actually written (tracked by self.pos). If render() writes fewer than 29 bytes (e.g. due to a bug or early return from fmt::Write), stale bytes from a previous render will be included in the HTTP Date header silently.","fix":"Change buffer() to return &self.bytes[..self.pos], or assert self.pos == DATE_VALUE_LENGTH before returning the full slice.","locations":[{"ref":"src/common/date.rs:59-61","code":" 59 | fn buffer(&self) -> &[u8] {\n 60 | &self.bytes[..]\n 61 | }","lines":3}]},{"id":"PERF_4","type":"performance","type_label":"Performance","severity":"low","easy":false,"description":"In hyper_headers_foreach, the ordered path uses headers.headers.get_all(name).iter().nth(*idx) for every header, which is O(idx) for each call (linear scan through a GetAll iterator). For a response with N headers of the same name, this makes the iteration O(N^2) total. The non-ordered path iterates linearly but uses two separate iterators. The ordered path's O(N^2) behavior can be significant for responses with many repeated header names.","fix":"For the ordered path, consider building a mapping from (name, occurrence-index) to value once, then iterate in order. Alternatively, use a vec of pre-fetched (name_ptr, val_ptr) pairs to avoid repeated nth() calls.","locations":[{"ref":"src/ffi/http_types.rs:459-508","code":" 459 | let mut ordered_iter = headers.orig_order.get_in_order().peekable();\n 460 | if ordered_iter.peek().is_some() {\n 461 | for (name, idx) in ordered_iter {\n 462 | let (name_ptr, name_len) = if let Some(orig_name) = headers.orig_casing.get_all(name).nth(*idx) {\n 463 | (orig_name.as_ref().as_ptr(), orig_name.as_ref().len())\n 464 | } else {\n 465 | (\n 466 | name.as_str().as_bytes().as_ptr(),\n 467 | name.as_str().as_bytes().len(),\n 468 | )\n 469 | };\n 470 | \n 471 | let val_ptr;\n 472 | let val_len;\n 473 | if let Some(value) = headers.headers.get_all(name).iter().nth(*idx) {\n 474 | val_ptr = value.as_bytes().as_ptr();\n 475 | val_len = value.as_bytes().len();\n 476 | } else {\n 477 | // Stop iterating, something has gone wrong.\n 478 | return;\n 479 | }\n 480 | \n 481 | if HYPER_ITER_CONTINUE != func(userdata, name_ptr, name_len, val_ptr, val_len) {\n 482 | return;\n 483 | }\n 484 | }\n 485 | } else {\n 486 | for name in headers.headers.keys() {\n 487 | let mut names = headers.orig_casing.get_all(name);\n 488 | \n 489 | for value in headers.headers.get_all(name) {\n 490 | let (name_ptr, name_len) = if let Some(orig_name) = names.next() {\n 491 | (orig_name.as_ref().as_ptr(), orig_name.as_ref().len())\n 492 | } else {\n 493 | (\n 494 | name.as_str().as_bytes().as_ptr(),\n 495 | name.as_str().as_bytes().len(),\n 496 | )\n 497 | };\n 498 | \n 499 | let val_ptr = value.as_bytes().as_ptr();\n 500 | let val_len = value.as_bytes().len();\n 501 | \n 502 | if HYPER_ITER_CONTINUE != func(userdata, name_ptr, name_len, val_ptr, val_len) {\n 503 | return;\n 504 | }\n 505 | }\n 506 | }\n 507 | }\n 508 | }","lines":50}]},{"id":"PERF_5","type":"performance","type_label":"Performance","severity":"low","easy":false,"description":"In headers.rs, content_length_parse_all_values() iterates over all Content-Length header values and for each one splits by comma to handle comma-separated values within a single header. RFC 7230 does not allow multiple Content-Length values to be comma-separated in a single header field; each must be a separate header line. The comma-splitting adds unnecessary complexity and a minor performance overhead. More importantly, it may silently accept malformed Content-Length headers like '100,100' as valid (if both values are equal), which is technically incorrect.","fix":"Remove the inner comma-splitting loop and treat each header line as a single integer value. This is more RFC-conformant and simpler.","locations":[{"ref":"src/headers.rs:45-70","code":" 45 | pub(super) fn content_length_parse_all_values(values: ValueIter<'_, HeaderValue>) -> Option<u64> {\n 46 | // If multiple Content-Length headers were sent, everything can still\n 47 | // be alright if they all contain the same value, and all parse\n 48 | // correctly. If not, then it's an error.\n 49 | \n 50 | let mut content_length: Option<u64> = None;\n 51 | for h in values {\n 52 | if let Ok(line) = h.to_str() {\n 53 | for v in line.split(',') {\n 54 | if let Some(n) = from_digits(v.trim().as_bytes()) {\n 55 | if content_length.is_none() {\n 56 | content_length = Some(n)\n 57 | } else if content_length != Some(n) {\n 58 | return None;\n 59 | }\n 60 | } else {\n 61 | return None;\n 62 | }\n 63 | }\n 64 | } else {\n 65 | return None;\n 66 | }\n 67 | }\n 68 | \n 69 | content_length\n 70 | }","lines":26}]},{"id":"PERF_6","type":"performance","type_label":"Performance","severity":"low","easy":false,"description":"In `Dispatcher::poll_loop` (dispatch.rs lines 166-213), `poll_write` is called twice in some iterations: once at line 174 and potentially again at line 207. This is intentional per the comments but causes redundant work. When `wants_write_again && !wants_read_again`, the second `poll_write` call re-polls the write side. If the body is a slow producer, this second poll immediately returns `Pending` and is wasted. The loop limit of 16 means this doubles the write polls in the worst case.","fix":"Low priority; this is an intentional optimization to avoid starvation. The double-poll is bounded by the 16-iteration limit.","locations":[{"ref":"src/proto/h1/dispatch.rs:166-213","code":" 166 | fn poll_loop(&mut self, cx: &mut Context<'_>) -> Poll<crate::Result<()>> {\n 167 | // Limit the looping on this connection, in case it is ready far too\n 168 | // often, so that other futures don't starve.\n 169 | //\n 170 | // 16 was chosen arbitrarily, as that is number of pipelined requests\n 171 | // benchmarks often use. Perhaps it should be a config option instead.\n 172 | for _ in 0..16 {\n 173 | let _ = self.poll_read(cx)?;\n 174 | let write_ready = self.poll_write(cx)?.is_ready();\n 175 | let flush_ready = self.poll_flush(cx)?.is_ready();\n 176 | \n 177 | // If we can write more body and the connection is ready, we should\n 178 | // write again. If we return `Ready(Ok(())` here, we will yield\n 179 | // without a guaranteed wake-up from the write side of the connection.\n 180 | // This would lead to a deadlock if we also don't expect reads.\n 181 | let wants_write_again = self.can_write_again() && (write_ready || flush_ready);\n 182 | \n 183 | // This could happen if reading paused before blocking on IO,\n 184 | // such as getting to the end of a framed message, but then\n 185 | // writing/flushing set the state back to Init. In that case,\n 186 | // if the read buffer still had bytes, we'd want to try poll_read\n 187 | // again, or else we wouldn't ever be woken up again.\n 188 | //\n 189 | // Using this instead of task::current() and notify() inside\n 190 | // the Conn is noticeably faster in pipelined benchmarks.\n 191 | let wants_read_again = self.conn.wants_read_again();\n 192 | \n 193 | // If we cannot write or read again, we yield and rely on the\n 194 | // wake-up from the connection futures.\n 195 | if !(wants_write_again || wants_read_again) {\n 196 | return Poll::Ready(Ok(()));\n 197 | }\n 198 | \n 199 | // If we are continuing only because \"wants_write_again\", re-check whether a second\n 200 | // write poll can make progress. `poll_flush` can be ready even when there is no\n 201 | // buffered data and the request body is still pending, so relying on the previous\n 202 | // readiness can hot-loop.\n 203 | if !wants_read_again && wants_write_again {\n 204 | // Write was previously pending, but may have become ready since polling flush, so\n 205 | // we need to check it again. If it is still pending, it is safe to yield and rely\n 206 | // on wake-up from the connection futures.\n 207 | if self.poll_write(cx)?.is_pending() {\n 208 | return Poll::Ready(Ok(()));\n 209 | }\n 210 | }\n 211 | }\n 212 | trace!(\"poll_loop yielding (self = {:p})\", self);\n 213 | task::yield_now(cx).map(|never| match never {})","lines":48}]},{"id":"PERF_7","type":"performance","type_label":"Performance","severity":"low","easy":false,"description":"In `encode_trailers` (encode.rs lines 163-217), a `HashSet<&HeaderName>` is built from `allowed_trailer_fields` on every call, even though the allowed list is set once at encoder construction and never changes. This causes repeated heap allocation and hashing on every trailer send.","fix":"Pre-compute the `HashSet` when `into_chunked_with_trailing_fields` is called (i.e. store it alongside the `Vec<HeaderName>`) so that `encode_trailers` can use it directly without re-allocating.","locations":[{"ref":"src/proto/h1/encode.rs:163-217","code":" 163 | pub(crate) fn encode_trailers<B>(\n 164 | &self,\n 165 | trailers: HeaderMap,\n 166 | title_case_headers: bool,\n 167 | ) -> Option<EncodedBuf<B>> {\n 168 | trace!(\"encoding trailers\");\n 169 | match &self.kind {\n 170 | Kind::Chunked(Some(allowed_trailer_fields)) => {\n 171 | let allowed_set: HashSet<&HeaderName> = allowed_trailer_fields.iter().collect();\n 172 | \n 173 | let mut cur_name = None;\n 174 | let mut allowed_trailers = HeaderMap::new();\n 175 | \n 176 | for (opt_name, value) in trailers {\n 177 | if let Some(n) = opt_name {\n 178 | cur_name = Some(n);\n 179 | }\n 180 | let name = cur_name.as_ref().expect(\"current header name\");\n 181 | \n 182 | if allowed_set.contains(name) {\n 183 | if is_valid_trailer_field(name) {\n 184 | allowed_trailers.insert(name, value);\n 185 | } else {\n 186 | debug!(\"trailer field is not valid: {}\", &name);\n 187 | }\n 188 | } else {\n 189 | debug!(\"trailer header name not found in trailer header: {}\", &name);\n 190 | }\n 191 | }\n 192 | \n 193 | let mut buf = Vec::new();\n 194 | if title_case_headers {\n 195 | write_headers_title_case(&allowed_trailers, &mut buf);\n 196 | } else {\n 197 | write_headers(&allowed_trailers, &mut buf);\n 198 | }\n 199 | \n 200 | if buf.is_empty() {\n 201 | return None;\n 202 | }\n 203 | \n 204 | Some(EncodedBuf {\n 205 | kind: BufKind::Trailers(b\"0\\r\\n\".chain(Bytes::from(buf)).chain(b\"\\r\\n\")),\n 206 | })\n 207 | }\n 208 | Kind::Chunked(None) => {\n 209 | debug!(\"attempted to encode trailers, but the trailer header is not set\");\n 210 | None\n 211 | }\n 212 | _ => {\n 213 | debug!(\"attempted to encode trailers for non-chunked response\");\n 214 | None\n 215 | }\n 216 | }\n 217 | }","lines":55}]},{"id":"PERF_8","type":"performance","type_label":"Performance","severity":"low","easy":false,"description":"In `WriteBuf::buffer` with `WriteStrategy::Flatten` (io.rs lines 549-570), the copy loop manually calls `buf.chunk()` and `buf.advance()` in a hand-written loop. The `bytes` crate's `Buf::copy_to_slice` or `bytes::BufMut::put` would be faster because they can take advantage of the concrete type, but the comment says this is already faster than `<Vec as BufMut>::put`. However the inner loop calls `extend_from_slice` with each chunk individually, which may cause repeated bounds checks. This is minor but noted.","fix":"Low priority; the existing loop is already an optimization. No action required unless profiling shows it as a hotspot.","locations":[{"ref":"src/proto/h1/io.rs:549-570","code":" 549 | WriteStrategy::Flatten => {\n 550 | let head = self.headers_mut();\n 551 | \n 552 | head.maybe_unshift(buf.remaining());\n 553 | trace!(\n 554 | self.len = head.remaining(),\n 555 | buf.len = buf.remaining(),\n 556 | \"buffer.flatten\"\n 557 | );\n 558 | //perf: This is a little faster than <Vec as BufMut>>::put,\n 559 | //but accomplishes the same result.\n 560 | loop {\n 561 | let adv = {\n 562 | let slice = buf.chunk();\n 563 | if slice.is_empty() {\n 564 | return;\n 565 | }\n 566 | head.bytes.extend_from_slice(slice);\n 567 | slice.len()\n 568 | };\n 569 | buf.advance(adv);\n 570 | }","lines":22}]},{"id":"PERF_9","type":"performance","type_label":"Performance","severity":"low","easy":false,"description":"In `ClientTask::poll` (client.rs), after calling `self.h2_tx.send_request(req, eos)` at line 735, there is an immediate second call to `self.h2_tx.poll_ready(cx)` at line 759 to check if the channel is still available. This means `poll_ready` is called twice in the same poll cycle for every request: once at the top of the loop (line 680) and once after `send_request`. The `h2` library may not cache this result, so this is a redundant roundtrip through the h2 state machine. The comment at line 756 explains this is intentional to handle the case where `send_request` causes the stream to be pending-open, but the overhead affects every request even when not needed.","fix":"This is by design per the comment. The performance cost is acceptable. Consider adding a comment explaining this is a necessary double-check, not redundant polling.","locations":[{"ref":"src/proto/h2/client.rs:679-691","code":" 679 | loop {\n 680 | match ready!(self.h2_tx.poll_ready(cx)) {\n 681 | Ok(()) => (),\n 682 | Err(err) => {\n 683 | self.ping.ensure_not_timed_out()?;\n 684 | return if err.reason() == Some(::h2::Reason::NO_ERROR) {\n 685 | trace!(\"connection gracefully shutdown\");\n 686 | Poll::Ready(Ok(Dispatched::Shutdown))\n 687 | } else {\n 688 | Poll::Ready(Err(crate::Error::new_h2(err)))\n 689 | };\n 690 | }\n 691 | };","lines":13},{"ref":"src/proto/h2/client.rs:759-773","code":" 759 | match self.h2_tx.poll_ready(cx) {\n 760 | Poll::Pending => {\n 761 | // Save Context\n 762 | self.fut_ctx = Some(f);\n 763 | return Poll::Pending;\n 764 | }\n 765 | Poll::Ready(Ok(())) => (),\n 766 | Poll::Ready(Err(err)) => {\n 767 | f.cb.send(Err(TrySendError {\n 768 | error: crate::Error::new_h2(err),\n 769 | message: None,\n 770 | }));\n 771 | continue;\n 772 | }\n 773 | }","lines":15}]},{"id":"PERF_10","type":"performance","type_label":"Performance","severity":"low","easy":false,"description":"In `strip_connection_headers` (mod.rs), the function iterates over `CONNECTION_HEADERS` (4 items: keep-alive, proxy-connection, transfer-encoding, upgrade) and removes each from the `HeaderMap` using `headers.remove(header)`. This is up to 4 separate O(1) hash lookups. Additionally, after processing those, it removes the `CONNECTION` header itself and then parses its value to remove any additional headers it references. In practice most HTTP/2 requests won't have these headers, so the removals are no-ops. The overhead is minor, but this function is called for every request and response.","fix":"No change needed for correctness. For high-throughput scenarios, consider checking if any connection-type headers exist before iterating, though this optimization is likely not significant.","locations":[{"ref":"src/proto/h2/mod.rs:50-89","code":" 50 | fn strip_connection_headers(headers: &mut HeaderMap, kind: MessageKind) {\n 51 | for header in &CONNECTION_HEADERS {\n 52 | if headers.remove(header).is_some() {\n 53 | warn!(\"Connection header illegal in HTTP/2: {}\", header.as_str());\n 54 | }\n 55 | }\n 56 | \n 57 | #[cfg(not(feature = \"client\"))]\n 58 | let _ = kind;\n 59 | #[cfg(feature = \"client\")]\n 60 | if matches!(kind, MessageKind::Request) {\n 61 | if headers\n 62 | .get(http::header::TE)\n 63 | .map_or(false, |te_header| te_header != \"trailers\")\n 64 | {\n 65 | warn!(\"TE headers not set to \\\"trailers\\\" are illegal in HTTP/2 requests\");\n 66 | headers.remove(http::header::TE);\n 67 | }\n 68 | } else if headers.remove(http::header::TE).is_some() {\n 69 | warn!(\"TE headers illegal in HTTP/2 responses\");\n 70 | }\n 71 | \n 72 | if let Some(header) = headers.remove(CONNECTION) {\n 73 | warn!(\n 74 | \"Connection header illegal in HTTP/2: {}\",\n 75 | CONNECTION.as_str()\n 76 | );\n 77 | // A `Connection` header may have a comma-separated list of names of other headers that\n 78 | // are meant for only this specific connection.\n 79 | //\n 80 | // Iterate these names and remove them as headers. Connection-specific headers are\n 81 | // forbidden in HTTP2, as that information has been moved into frame types of the h2\n 82 | // protocol.\n 83 | if let Ok(header_contents) = header.to_str() {\n 84 | for name in header_contents.split(',') {\n 85 | let name = name.trim();\n 86 | headers.remove(name);\n 87 | }\n 88 | }\n 89 | }","lines":40}]},{"id":"DEAD_1","type":"dead_code","type_label":"Dead Code","severity":"low","easy":true,"description":"In hyper_headers_foreach, the peekable() check 'if ordered_iter.peek().is_some()' followed by iterating with 'for (name, idx) in ordered_iter' has a double-space on the assignment line (line 459: 'let mut ordered_iter = headers.orig_order...'). Minor style issue.","fix":"Remove the extra space: 'let mut ordered_iter = headers.orig_order.get_in_order().peekable();'.","locations":[{"ref":"src/ffi/http_types.rs:459","code":" 459 | let mut ordered_iter = headers.orig_order.get_in_order().peekable();","lines":1}]},{"id":"DEAD_2","type":"dead_code","type_label":"Dead Code","severity":"low","easy":true,"description":"The `Serving::closing` field (server.rs) is declared and checked (`self.closing.is_none()`, `self.closing.is_some()`), and `self.closing.take().expect(...)` is called at line 345, but `self.closing` is never actually set to `Some(...)` anywhere in the codebase. The entire `closing` branch in `poll_server` (lines 338-345, the block that calls `poll_closed` and returns `Err(self.closing.take()...)`) is dead code.","fix":"Remove the `closing: Option<crate::Error>` field and its associated dead code, or implement the intended logic that sets `closing` when a graceful shutdown with a pending error is needed.","locations":[{"ref":"src/proto/h2/server.rs:110-118","code":" 110 | struct Serving<T, B>\n 111 | where\n 112 | B: Body,\n 113 | {\n 114 | ping: Option<(ping::Recorder, ping::Ponger)>,\n 115 | conn: Connection<Compat<T>, SendBuf<B::Data>>,\n 116 | closing: Option<crate::Error>,\n 117 | date_header: bool,\n 118 | }","lines":9},{"ref":"src/proto/h2/server.rs:338-345","code":" 338 | debug_assert!(\n 339 | self.closing.is_some(),\n 340 | \"poll_server broke loop without closing\"\n 341 | );\n 342 | \n 343 | ready!(self.conn.poll_closed(cx).map_err(crate::Error::new_h2))?;\n 344 | \n 345 | Poll::Ready(Err(self.closing.take().expect(\"polled after error\")))","lines":8}]},{"id":"DEAD_3","type":"dead_code","type_label":"Dead Code","severity":"negligible","easy":true,"description":"src/client/tests.rs and src/mock.rs are entirely commented out with block comments (/* ... */). They contain no active code. The test code references outdated pre-async APIs (futures 0.1 style). These files add noise and maintenance burden without providing any actual test coverage.","fix":"Either remove the files entirely, or rewrite the tests using the current async/await API as the FIXME comments suggest.","locations":[{"ref":"src/client/tests.rs:1-261","code":" 1 | /*\n 2 | // FIXME: re-implement tests with `async/await`\n 3 | #[test]\n 4 | fn retryable_request() {\n 5 | let _ = pretty_env_logger::try_init();\n 6 | \n 7 | let mut rt = Runtime::new().expect(\"new rt\");\n 8 | let mut connector = MockConnector::new();\n 9 | \n 10 | let sock1 = connector.mock(\"http://mock.local\");\n 11 | let sock2 = connector.mock(\"http://mock.local\");\n 12 | \n 13 | let client = Client::builder()\n 14 | .build::<_, crate::Body>(connector);\n 15 | \n 16 | client.pool.no_timer();\n 17 | \n 18 | {\n 19 | \n 20 | let req = Request::builder()\n 21 | .uri(\"http://mock.local/a\")\n 22 | .body(Default::default())\n 23 | .unwrap();\n 24 | let res1 = client.request(req);\n 25 | let srv1 = poll_fn(|| {\n 26 | try_ready!(sock1.read(&mut [0u8; 512]));\n 27 | try_ready!(sock1.write(b\"HTTP/1.1 200 OK\\r\\nContent-Length: 0\\r\\n\\r\\n\"));\n 28 | Ok(Async::Ready(()))\n 29 | }).map_err(|e: std::io::Error| panic!(\"srv1 poll_fn error: {}\", e));\n 30 | rt.block_on(res1.join(srv1)).expect(\"res1\");\n 31 | }\n 32 | drop(sock1);\n 33 | \n 34 | let req = Request::builder()\n 35 | .uri(\"http://mock.local/b\")\n 36 | .body(Default::default())\n 37 | .unwrap();\n 38 | let res2 = client.request(req)\n 39 | .map(|res| {\n 40 | assert_eq!(res.status().as_u16(), 222);\n 41 | });\n 42 | let srv2 = poll_fn(|| {\n 43 | try_ready!(sock2.read(&mut [0u8; 512]));\n 44 | try_ready!(sock2.write(b\"HTTP/1.1 222 OK\\r\\nContent-Length: 0\\r\\n\\r\\n\"));\n 45 | Ok(Async::Ready(()))\n 46 | }).map_err(|e: std::io::Error| panic!(\"srv2 poll_fn error: {}\", e));\n 47 | \n 48 | rt.block_on(res2.join(srv2)).expect(\"res2\");\n 49 | }\n 50 | \n 51 | #[test]\n 52 | fn conn_reset_after_write() {\n 53 | let _ = pretty_env_logger::try_init();\n 54 | \n 55 | let mut rt = Runtime::new().expect(\"new rt\");\n 56 | let mut connector = MockConnector::new();\n 57 | \n 58 | let sock1 = connector.mock(\"http://mock.local\");\n 59 | \n 60 | let client = Client::builder()\n 61 | .build::<_, crate::Body>(connector);\n 62 | \n 63 | client.pool.no_timer();\n 64 | \n 65 | {\n 66 | let req = Request::builder()\n 67 | .uri(\"http://mock.local/a\")\n 68 | .body(Default::default())\n 69 | .unwrap();\n 70 | let res1 = client.request(req);\n 71 | let srv1 = poll_fn(|| {\n 72 | try_ready!(sock1.read(&mut [0u8; 512]));\n 73 | try_ready!(sock1.write(b\"HTTP/1.1 200 OK\\r\\nContent-Length: 0\\r\\n\\r\\n\"));\n 74 | Ok(Async::Ready(()))\n 75 | }).map_err(|e: std::io::Error| panic!(\"srv1 poll_fn error: {}\", e));\n 76 | rt.block_on(res1.join(srv1)).expect(\"res1\");\n 77 | }\n 78 | \n 79 | let req = Request::builder()\n 80 | .uri(\"http://mock.local/a\")\n 81 | .body(Default::default())\n 82 | .unwrap();\n 83 | let res2 = client.request(req);\n 84 | let mut sock1 = Some(sock1);\n 85 | let srv2 = poll_fn(|| {\n 86 | // We purposefully keep the socket open until the client\n 87 | // has written the second request, and THEN disconnect.\n 88 | //\n 89 | // Not because we expect servers to be jerks, but to trigger\n 90 | // state where we write on an assumedly good connection, and\n 91 | // only reset the close AFTER we wrote bytes.\n 92 | try_ready!(sock1.as_mut().unwrap().read(&mut [0u8; 512]));\n 93 | sock1.take();\n 94 | Ok(Async::Ready(()))\n 95 | }).map_err(|e: std::io::Error| panic!(\"srv2 poll_fn error: {}\", e));\n 96 | let err = rt.block_on(res2.join(srv2)).expect_err(\"res2\");\n 97 | assert!(err.is_incomplete_message(), \"{:?}\", err);\n 98 | }\n 99 | \n 100 | #[test]\n 101 | fn checkout_win_allows_connect_future_to_be_pooled() {\n 102 | let _ = pretty_env_logger::try_init();\n 103 | \n 104 | let mut rt = Runtime::new().expect(\"new rt\");\n 105 | let mut connector = MockConnector::new();\n 106 | \n 107 | \n 108 | let (tx, rx) = oneshot::channel::<()>();\n 109 | let sock1 = connector.mock(\"http://mock.local\");\n 110 | let sock2 = connector.mock_fut(\"http://mock.local\", rx);\n 111 | \n 112 | let client = Client::builder()\n 113 | .build::<_, crate::Body>(connector);\n 114 | \n 115 | client.pool.no_timer();\n 116 | \n 117 | let uri = \"http://mock.local/a\".parse::<crate::Uri>().expect(\"uri parse\");\n 118 | \n 119 | // First request just sets us up to have a connection able to be put\n 120 | // back in the pool. *However*, it doesn't insert immediately. The\n 121 | // body has 1 pending byte, and we will only drain in request 2, once\n 122 | // the connect future has been started.\n 123 | let mut body = {\n 124 | let res1 = client.get(uri.clone())\n 125 | .map(|res| res.into_body().concat2());\n 126 | let srv1 = poll_fn(|| {\n 127 | try_ready!(sock1.read(&mut [0u8; 512]));\n 128 | // Chunked is used so as to force 2 body reads.\n 129 | try_ready!(sock1.write(b\"\\\n 130 | HTTP/1.1 200 OK\\r\\n\\\n 131 | transfer-encoding: chunked\\r\\n\\\n 132 | \\r\\n\\\n 133 | 1\\r\\nx\\r\\n\\\n 134 | 0\\r\\n\\r\\n\\\n 135 | \"));\n 136 | Ok(Async::Ready(()))\n 137 | }).map_err(|e: std::io::Error| panic!(\"srv1 poll_fn error: {}\", e));\n 138 | \n 139 | rt.block_on(res1.join(srv1)).expect(\"res1\").0\n 140 | };\n 141 | \n 142 | \n 143 | // The second request triggers the only mocked connect future, but then\n 144 | // the drained body allows the first socket to go back to the pool,\n 145 | // \"winning\" the checkout race.\n 146 | {\n 147 | let res2 = client.get(uri.clone());\n 148 | let drain = poll_fn(move || {\n 149 | body.poll()\n 150 | });\n 151 | let srv2 = poll_fn(|| {\n 152 | try_ready!(sock1.read(&mut [0u8; 512]));\n 153 | try_ready!(sock1.write(b\"HTTP/1.1 200 OK\\r\\nConnection: close\\r\\n\\r\\nx\"));\n 154 | Ok(Async::Ready(()))\n 155 | }).map_err(|e: std::io::Error| panic!(\"srv2 poll_fn error: {}\", e));\n 156 | \n 157 | rt.block_on(res2.join(drain).join(srv2)).expect(\"res2\");\n 158 | }\n 159 | \n 160 | // \"Release\" the mocked connect future, and let the runtime spin once so\n 161 | // it's all setup...\n 162 | {\n 163 | let mut tx = Some(tx);\n 164 | let client = &client;\n 165 | let key = client.pool.h1_key(\"http://mock.local\");\n 166 | let mut tick_cnt = 0;\n 167 | let fut = poll_fn(move || {\n 168 | tx.take();\n 169 | \n 170 | if client.pool.idle_count(&key) == 0 {\n 171 | tick_cnt += 1;\n 172 | assert!(tick_cnt < 10, \"ticked too many times waiting for idle\");\n 173 | trace!(\"no idle yet; tick count: {}\", tick_cnt);\n 174 | ::futures::task::current().notify();\n 175 | Ok(Async::NotReady)\n 176 | } else {\n 177 | Ok::<_, ()>(Async::Ready(()))\n 178 | }\n 179 | });\n 180 | rt.block_on(fut).unwrap();\n 181 | }\n 182 | \n 183 | // Third request just tests out that the \"loser\" connection was pooled. If\n 184 | // it isn't, this will panic since the MockConnector doesn't have any more\n 185 | // mocks to give out.\n 186 | {\n 187 | let res3 = client.get(uri);\n 188 | let srv3 = poll_fn(|| {\n 189 | try_ready!(sock2.read(&mut [0u8; 512]));\n 190 | try_ready!(sock2.write(b\"HTTP/1.1 200 OK\\r\\nContent-Length: 0\\r\\n\\r\\n\"));\n 191 | Ok(Async::Ready(()))\n 192 | }).map_err(|e: std::io::Error| panic!(\"srv3 poll_fn error: {}\", e));\n 193 | \n 194 | rt.block_on(res3.join(srv3)).expect(\"res3\");\n 195 | }\n 196 | }\n 197 | \n 198 | #[cfg(feature = \"nightly\")]\n 199 | #[bench]\n 200 | fn bench_http1_get_0b(b: &mut test::Bencher) {\n 201 | let _ = pretty_env_logger::try_init();\n 202 | \n 203 | let mut rt = Runtime::new().expect(\"new rt\");\n 204 | let mut connector = MockConnector::new();\n 205 | \n 206 | \n 207 | let client = Client::builder()\n 208 | .build::<_, crate::Body>(connector.clone());\n 209 | \n 210 | client.pool.no_timer();\n 211 | \n 212 | let uri = Uri::from_static(\"http://mock.local/a\");\n 213 | \n 214 | b.iter(move || {\n 215 | let sock1 = connector.mock(\"http://mock.local\");\n 216 | let res1 = client\n 217 | .get(uri.clone())\n 218 | .and_then(|res| {\n 219 | res.into_body().for_each(|_| Ok(()))\n 220 | });\n 221 | let srv1 = poll_fn(|| {\n 222 | try_ready!(sock1.read(&mut [0u8; 512]));\n 223 | try_ready!(sock1.write(b\"HTTP/1.1 200 OK\\r\\nContent-Length: 0\\r\\n\\r\\n\"));\n 224 | Ok(Async::Ready(()))\n 225 | }).map_err(|e: std::io::Error| panic!(\"srv1 poll_fn error: {}\", e));\n 226 | rt.block_on(res1.join(srv1)).expect(\"res1\");\n 227 | });\n 228 | }\n 229 | \n 230 | #[cfg(feature = \"nightly\")]\n 231 | #[bench]\n 232 | fn bench_http1_get_10b(b: &mut test::Bencher) {\n 233 | let _ = pretty_env_logger::try_init();\n 234 | \n 235 | let mut rt = Runtime::new().expect(\"new rt\");\n 236 | let mut connector = MockConnector::new();\n 237 | \n 238 | \n 239 | let client = Client::builder()\n 240 | .build::<_, crate::Body>(connector.clone());\n 241 | \n 242 | client.pool.no_timer();\n 243 | \n 244 | let uri = Uri::from_static(\"http://mock.local/a\");\n 245 | \n 246 | b.iter(move || {\n 247 | let sock1 = connector.mock(\"http://mock.local\");\n 248 | let res1 = client\n 249 | .get(uri.clone())\n 250 | .and_then(|res| {\n 251 | res.into_body().for_each(|_| Ok(()))\n 252 | });\n 253 | let srv1 = poll_fn(|| {\n 254 | try_ready!(sock1.read(&mut [0u8; 512]));\n 255 | try_ready!(sock1.write(b\"HTTP/1.1 200 OK\\r\\nContent-Length: 10\\r\\n\\r\\n0123456789\"));\n 256 | Ok(Async::Ready(()))\n 257 | }).map_err(|e: std::io::Error| panic!(\"srv1 poll_fn error: {}\", e));\n 258 | rt.block_on(res1.join(srv1)).expect(\"res1\");\n 259 | });\n 260 | }\n 261 | */","lines":261},{"ref":"src/mock.rs:1-237","code":" 1 | // FIXME: re-implement tests with `async/await`\n 2 | /*\n 3 | #[cfg(feature = \"runtime\")]\n 4 | use std::collections::HashMap;\n 5 | use std::cmp;\n 6 | use std::io::{self, Read, Write};\n 7 | #[cfg(feature = \"runtime\")]\n 8 | use std::sync::{Arc, Mutex};\n 9 | \n 10 | use bytes::Buf;\n 11 | use futures::{Async, Poll};\n 12 | #[cfg(feature = \"runtime\")]\n 13 | use futures::Future;\n 14 | use futures::task::{self, Task};\n 15 | use tokio_io::{AsyncRead, AsyncWrite};\n 16 | \n 17 | #[cfg(feature = \"runtime\")]\n 18 | use crate::client::connect::{Connect, Connected, Destination};\n 19 | #[cfg(feature = \"runtime\")]\n 20 | use crate::common::lock::LockResultExt;\n 21 | \n 22 | \n 23 | \n 24 | #[cfg(feature = \"runtime\")]\n 25 | pub struct Duplex {\n 26 | inner: Arc<Mutex<DuplexInner>>,\n 27 | }\n 28 | \n 29 | #[cfg(feature = \"runtime\")]\n 30 | struct DuplexInner {\n 31 | handle_read_task: Option<Task>,\n 32 | read: AsyncIo<MockCursor>,\n 33 | write: AsyncIo<MockCursor>,\n 34 | }\n 35 | \n 36 | #[cfg(feature = \"runtime\")]\n 37 | impl Duplex {\n 38 | pub(crate) fn channel() -> (Duplex, DuplexHandle) {\n 39 | let mut inner = DuplexInner {\n 40 | handle_read_task: None,\n 41 | read: AsyncIo::new_buf(Vec::new(), 0),\n 42 | write: AsyncIo::new_buf(Vec::new(), std::usize::MAX),\n 43 | };\n 44 | \n 45 | inner.read.park_tasks(true);\n 46 | inner.write.park_tasks(true);\n 47 | \n 48 | let inner = Arc::new(Mutex::new(inner));\n 49 | \n 50 | let duplex = Duplex {\n 51 | inner: inner.clone(),\n 52 | };\n 53 | let handle = DuplexHandle {\n 54 | inner: inner,\n 55 | };\n 56 | \n 57 | (duplex, handle)\n 58 | }\n 59 | }\n 60 | \n 61 | #[cfg(feature = \"runtime\")]\n 62 | impl Read for Duplex {\n 63 | fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {\n 64 | self.inner.lock().panic_if_poisoned().read.read(buf)\n 65 | }\n 66 | }\n 67 | \n 68 | #[cfg(feature = \"runtime\")]\n 69 | impl Write for Duplex {\n 70 | fn write(&mut self, buf: &[u8]) -> io::Result<usize> {\n 71 | let mut inner = self.inner.lock().panic_if_poisoned();\n 72 | let ret = inner.write.write(buf);\n 73 | if let Some(task) = inner.handle_read_task.take() {\n 74 | trace!(\"waking DuplexHandle read\");\n 75 | task.notify();\n 76 | }\n 77 | ret\n 78 | }\n 79 | \n 80 | fn flush(&mut self) -> io::Result<()> {\n 81 | self.inner.lock().panic_if_poisoned().write.flush()\n 82 | }\n 83 | }\n 84 | \n 85 | #[cfg(feature = \"runtime\")]\n 86 | impl AsyncRead for Duplex {\n 87 | }\n 88 | \n 89 | #[cfg(feature = \"runtime\")]\n 90 | impl AsyncWrite for Duplex {\n 91 | fn shutdown(&mut self) -> Poll<(), io::Error> {\n 92 | Ok(().into())\n 93 | }\n 94 | \n 95 | fn write_buf<B: Buf>(&mut self, buf: &mut B) -> Poll<usize, io::Error> {\n 96 | let mut inner = self.inner.lock().panic_if_poisoned();\n 97 | if let Some(task) = inner.handle_read_task.take() {\n 98 | task.notify();\n 99 | }\n 100 | inner.write.write_buf(buf)\n 101 | }\n 102 | }\n 103 | \n 104 | #[cfg(feature = \"runtime\")]\n 105 | pub struct DuplexHandle {\n 106 | inner: Arc<Mutex<DuplexInner>>,\n 107 | }\n 108 | \n 109 | #[cfg(feature = \"runtime\")]\n 110 | impl DuplexHandle {\n 111 | pub fn read(&self, buf: &mut [u8]) -> Poll<usize, io::Error> {\n 112 | let mut inner = self.inner.lock().panic_if_poisoned();\n 113 | assert!(buf.len() >= inner.write.inner.len());\n 114 | if inner.write.inner.is_empty() {\n 115 | trace!(\"DuplexHandle read parking\");\n 116 | inner.handle_read_task = Some(task::current());\n 117 | return Ok(Async::NotReady);\n 118 | }\n 119 | inner.write.read(buf).map(Async::Ready)\n 120 | }\n 121 | \n 122 | pub fn write(&self, bytes: &[u8]) -> Poll<usize, io::Error> {\n 123 | let mut inner = self.inner.lock().panic_if_poisoned();\n 124 | assert_eq!(inner.read.inner.pos, 0);\n 125 | assert_eq!(inner.read.inner.vec.len(), 0, \"write but read isn't empty\");\n 126 | inner\n 127 | .read\n 128 | .inner\n 129 | .vec\n 130 | .extend(bytes);\n 131 | inner.read.block_in(bytes.len());\n 132 | Ok(Async::Ready(bytes.len()))\n 133 | }\n 134 | }\n 135 | \n 136 | #[cfg(feature = \"runtime\")]\n 137 | impl Drop for DuplexHandle {\n 138 | fn drop(&mut self) {\n 139 | trace!(\"mock duplex handle drop\");\n 140 | if !::std::thread::panicking() {\n 141 | let mut inner = self.inner.lock().panic_if_poisoned();\n 142 | inner.read.close();\n 143 | inner.write.close();\n 144 | }\n 145 | }\n 146 | }\n 147 | \n 148 | #[cfg(feature = \"runtime\")]\n 149 | type BoxedConnectFut = Box<dyn Future<Item=(Duplex, Connected), Error=io::Error> + Send>;\n 150 | \n 151 | #[cfg(feature = \"runtime\")]\n 152 | #[derive(Clone)]\n 153 | pub struct MockConnector {\n 154 | mocks: Arc<Mutex<MockedConnections>>,\n 155 | }\n 156 | \n 157 | #[cfg(feature = \"runtime\")]\n 158 | struct MockedConnections(HashMap<String, Vec<BoxedConnectFut>>);\n 159 | \n 160 | #[cfg(feature = \"runtime\")]\n 161 | impl MockConnector {\n 162 | pub fn new() -> MockConnector {\n 163 | MockConnector {\n 164 | mocks: Arc::new(Mutex::new(MockedConnections(HashMap::new()))),\n 165 | }\n 166 | }\n 167 | \n 168 | pub fn mock(&mut self, key: &str) -> DuplexHandle {\n 169 | use futures::future;\n 170 | self.mock_fut(key, future::ok::<_, ()>(()))\n 171 | }\n 172 | \n 173 | pub fn mock_fut<F>(&mut self, key: &str, fut: F) -> DuplexHandle\n 174 | where\n 175 | F: Future + Send + 'static,\n 176 | {\n 177 | self.mock_opts(key, Connected::new(), fut)\n 178 | }\n 179 | \n 180 | pub fn mock_opts<F>(&mut self, key: &str, connected: Connected, fut: F) -> DuplexHandle\n 181 | where\n 182 | F: Future + Send + 'static,\n 183 | {\n 184 | let key = key.to_owned();\n 185 | \n 186 | let (duplex, handle) = Duplex::channel();\n 187 | \n 188 | let fut = Box::new(fut.then(move |_| {\n 189 | trace!(\"MockConnector mocked fut ready\");\n 190 | Ok((duplex, connected))\n 191 | }));\n 192 | self.mocks.lock().panic_if_poisoned().0.entry(key)\n 193 | .or_insert(Vec::new())\n 194 | .push(fut);\n 195 | \n 196 | handle\n 197 | }\n 198 | }\n 199 | \n 200 | #[cfg(feature = \"runtime\")]\n 201 | impl Connect for MockConnector {\n 202 | type Transport = Duplex;\n 203 | type Error = io::Error;\n 204 | type Future = BoxedConnectFut;\n 205 | \n 206 | fn connect(&self, dst: Destination) -> Self::Future {\n 207 | trace!(\"mock connect: {:?}\", dst);\n 208 | let key = format!(\"{}://{}{}\", dst.scheme(), dst.host(), if let Some(port) = dst.port() {\n 209 | format!(\":{}\", port)\n 210 | } else {\n 211 | \"\".to_owned()\n 212 | });\n 213 | let mut mocks = self.mocks.lock().panic_if_poisoned();\n 214 | let mocks = mocks.0.get_mut(&key)\n 215 | .expect(&format!(\"unknown mocks uri: {}\", key));\n 216 | assert!(!mocks.is_empty(), \"no additional mocks for {}\", key);\n 217 | mocks.remove(0)\n 218 | }\n 219 | }\n 220 | \n 221 | \n 222 | #[cfg(feature = \"runtime\")]\n 223 | impl Drop for MockedConnections {\n 224 | fn drop(&mut self) {\n 225 | if !::std::thread::panicking() {\n 226 | for (key, mocks) in self.0.iter() {\n 227 | assert_eq!(\n 228 | mocks.len(),\n 229 | 0,\n 230 | \"not all mocked connects for {:?} were used\",\n 231 | key,\n 232 | );\n 233 | }\n 234 | }\n 235 | }\n 236 | }\n 237 | */","lines":237}]},{"id":"CMT_1","type":"invalid_comment","type_label":"Invalid Comment","severity":"low","easy":true,"description":"In the C header (capi/include/hyper.h), both HYPER_IO_PENDING and HYPER_IO_ERROR have truncated comments: 'Sentinel value to return from a read or write callback that the operation' - the sentence is cut off and never completed. The Rust source has 'is pending' and 'has errored' as the missing endings.","fix":"Fix the gen_header.sh script or cbindgen configuration to emit the full doc comment for these constants.","locations":[{"ref":"capi/include/hyper.h:45-53","code":" 45 | /*\n 46 | Sentinel value to return from a read or write callback that the operation\n 47 | */\n 48 | #define HYPER_IO_PENDING 4294967295\n 49 | \n 50 | /*\n 51 | Sentinel value to return from a read or write callback that the operation\n 52 | */\n 53 | #define HYPER_IO_ERROR 4294967294","lines":9}]},{"id":"CMT_2","type":"invalid_comment","type_label":"Invalid Comment","severity":"low","easy":true,"description":"In common/io/rewind.rs, the rewind() method has `debug_assert!(self.pre.is_none())` which asserts no pre-buffer exists before rewinding. However the comment in the method is absent; the assertion will silently be dropped in release builds if someone calls rewind() twice, leading to the first rewind data being silently discarded (replaced by the second). There is no production-mode protection against this misuse.","fix":"Add a release-mode check or at least a doc comment explaining that calling rewind() when pre is already Some will silently discard the existing buffered bytes.","locations":[{"ref":"src/common/io/rewind.rs:41-44","code":" 41 | pub(crate) fn rewind(&mut self, bs: Bytes) {\n 42 | debug_assert!(self.pre.is_none());\n 43 | self.pre = Some(bs);\n 44 | }","lines":4}]},{"id":"CMT_3","type":"invalid_comment","type_label":"Invalid Comment","severity":"low","easy":true,"description":"The doc comment for hyper_response (struct) contains a typo on line 36: 'polling the executor unntil you get a hyper_task' - 'unntil' should be 'until'.","fix":"Change 'unntil' to 'until'.","locations":[{"ref":"src/ffi/http_types.rs:36","code":" 36 | /// polling the executor unntil you get a `hyper_task` of type","lines":1}]},{"id":"CMT_4","type":"invalid_comment","type_label":"Invalid Comment","severity":"low","easy":true,"description":"The documentation for hyper_io_set_write (line 117) says 'If there is an irrecoverable error reading data, then HYPER_IO_ERROR should be the return value.' This is a copy-paste error from hyper_io_set_read - the write callback has nothing to do with reading. It should say 'writing data'.","fix":"Change 'reading data' to 'writing data' in the write callback documentation.","locations":[{"ref":"src/ffi/io.rs:115-118","code":" 115 | /// for `hyper_waker`.\n 116 | ///\n 117 | /// If there is an irrecoverable error reading data, then `HYPER_IO_ERROR`\n 118 | /// should be the return value.","lines":4}]},{"id":"CMT_5","type":"invalid_comment","type_label":"Invalid Comment","severity":"low","easy":true,"description":"The module-level comment in src/ffi/mod.rs line 25 says 'staring with 1.64.0' - should be 'starting with 1.64.0'.","fix":"Change 'staring' to 'starting'.","locations":[{"ref":"src/ffi/mod.rs:25","code":" 25 | //! `cargo`, staring with `1.64.0`, it can be compiled with the following command:","lines":1}]},{"id":"CMT_6","type":"invalid_comment","type_label":"Invalid Comment","severity":"low","easy":true,"description":"In `decode.rs` line 19, the comment says `/// This limit is currentlty applied for the entire body, not per chunk.` - the word 'currentlty' is a typo for 'currently'. Minor but indicates the comment was not carefully reviewed.","fix":"Fix the typo: change 'currentlty' to 'currently'.","locations":[{"ref":"src/proto/h1/decode.rs:19","code":" 19 | /// This limit is currentlty applied for the entire body, not per chunk.","lines":1}]},{"id":"CMT_7","type":"invalid_comment","type_label":"Invalid Comment","severity":"low","easy":true,"description":"In `ping.rs` at lines 277 and 323, there are comments `// XXX: this doesn't register a waker...?` suggesting the author was uncertain whether wakers are properly registered when returning `Poll::Pending`. In `Ponger::poll`, when there is no ping sent (line 276-279) or after processing a pong result without a BDP update (line 322-323), `Poll::Pending` is returned without registering a waker. This means the `Ponger` task will not be woken up by itself - it relies on the parent task (the connection future) to drive it. If the connection has no other pending I/O, the ponger will never be polled again. This could delay keep-alive pings and BDP measurements.","fix":"If the Ponger is meant to wake itself (e.g. when a keep-alive timer fires), ensure the waker is registered. If it relies on the parent connection task's poll loop, document this design dependency explicitly and remove the `XXX` comments.","locations":[{"ref":"src/proto/h2/ping.rs:276-279","code":" 276 | if !locked.is_ping_sent() {\n 277 | // XXX: this doesn't register a waker...?\n 278 | return Poll::Pending;\n 279 | }","lines":4},{"ref":"src/proto/h2/ping.rs:321-323","code":" 321 | \n 322 | // XXX: this doesn't register a waker...?\n 323 | Poll::Pending","lines":3}]},{"id":"ARCH_1","type":"architecture","type_label":"Architecture","severity":"low","easy":false,"description":"The Callback::send() method takes self by value, which means the Callback is consumed. But Callback also has a custom Drop impl that tries to send an error if the inner Option is still Some. Since send() takes the inner Option value out via take(), the Drop impl will find None and do nothing. This is correct but the double-path (send via method or send via drop) is fragile: if send() panics between take() and the actual send, the error is silently lost (neither the success message is sent, nor the drop fallback fires). The expect() calls on lines 289 and 295 can panic if the sender was already consumed, though this should not happen in practice.","fix":"Document the invariant that send() must only be called once on a Callback, and add a test that verifies the drop fallback fires when send() is not called.","locations":[{"ref":"src/client/dispatch.rs:284-299","code":" 284 | pub(crate) fn send(mut self, val: Result<U, TrySendError<T>>) {\n 285 | match self {\n 286 | Callback::Retry(ref mut tx) => {\n 287 | let _ = tx\n 288 | .take()\n 289 | .expect(\"callback sender not dropped before send\")\n 290 | .send(val);\n 291 | }\n 292 | Callback::NoRetry(ref mut tx) => {\n 293 | let _ = tx\n 294 | .take()\n 295 | .expect(\"callback sender not dropped before send\")\n 296 | .send(val.map_err(|e| e.error));\n 297 | }\n 298 | }\n 299 | }","lines":16},{"ref":"src/client/dispatch.rs:236-254","code":" 236 | impl<T, U> Drop for Callback<T, U> {\n 237 | fn drop(&mut self) {\n 238 | match self {\n 239 | Callback::Retry(tx) => {\n 240 | if let Some(tx) = tx.take() {\n 241 | let _ = tx.send(Err(TrySendError {\n 242 | error: dispatch_gone(),\n 243 | message: None,\n 244 | }));\n 245 | }\n 246 | }\n 247 | Callback::NoRetry(tx) => {\n 248 | if let Some(tx) = tx.take() {\n 249 | let _ = tx.send(Err(dispatch_gone()));\n 250 | }\n 251 | }\n 252 | }\n 253 | }\n 254 | }","lines":19}]},{"id":"ARCH_2","type":"architecture","type_label":"Architecture","severity":"low","easy":false,"description":"In hyper_clientconn_send, a borrow of conn (via non_null!(&mut *conn)) is used to create 'fut' (a SendRequest future), but conn is a raw pointer whose lifetime is not tracked. The fut captures a reference to conn's internals (the Tx field), while conn itself is a raw pointer managed by C code. If the C caller frees conn via hyper_clientconn_free before the returned task completes, the future will access freed memory. The API documentation does not explicitly state that conn must remain alive until the task completes.","fix":"Document clearly that the hyper_clientconn must not be freed (via hyper_clientconn_free) until all outstanding tasks from hyper_clientconn_send have been polled to completion. Alternatively, restructure so the task takes ownership of the conn (but that prevents multiple in-flight requests).","locations":[{"ref":"src/ffi/client.rs:144-160","code":" 144 | fn hyper_clientconn_send(conn: *mut hyper_clientconn, req: *mut hyper_request) -> *mut hyper_task {\n 145 | let mut req = non_null! { Box::from_raw(req) ?= ptr::null_mut() };\n 146 | \n 147 | // Update request with original-case map of headers\n 148 | req.finalize_request();\n 149 | \n 150 | let fut = match non_null! { &mut *conn ?= ptr::null_mut() }.tx {\n 151 | Tx::Http1(ref mut tx) => futures_util::future::Either::Left(tx.send_request(req.0)),\n 152 | Tx::Http2(ref mut tx) => futures_util::future::Either::Right(tx.send_request(req.0)),\n 153 | };\n 154 | \n 155 | let fut = async move {\n 156 | fut.await.map(hyper_response::wrap)\n 157 | };\n 158 | \n 159 | Box::into_raw(hyper_task::boxed(fut))\n 160 | } ?= std::ptr::null_mut()","lines":17}]},{"id":"ARCH_3","type":"architecture","type_label":"Architecture","severity":"low","easy":false,"description":"The `Server::encode_headers` function in role.rs (lines 640-983) is approximately 340 lines long and handles many distinct concerns: writing the status line extension, handling Content-Length, Transfer-Encoding, Connection, Date, and Trailer headers, determining encoder type, handling implicit content-length, and applying the final `set_last` call. This function is complex enough that subtle bugs (like the Trailer header issue above) are easy to miss. It is also hard to unit-test individual steps in isolation.","fix":"Consider splitting into smaller functions: one for determining the encoder type, one for writing the body-length headers, and one for writing the remaining headers. This would improve testability and readability.","locations":[{"ref":"src/proto/h1/role.rs:640-983","code":" 640 | #[inline]\n 641 | fn encode_headers<W>(\n 642 | msg: Encode<'_, StatusCode>,\n 643 | dst: &mut Vec<u8>,\n 644 | mut is_last: bool,\n 645 | orig_len: usize,\n 646 | mut wrote_len: bool,\n 647 | mut header_name_writer: W,\n 648 | ) -> crate::Result<Encoder>\n 649 | where\n 650 | W: HeaderNameWriter,\n 651 | {\n 652 | // In some error cases, we don't know about the invalid message until already\n 653 | // pushing some bytes onto the `dst`. In those cases, we don't want to send\n 654 | // the half-pushed message, so rewind to before.\n 655 | let rewind = |dst: &mut Vec<u8>| {\n 656 | dst.truncate(orig_len);\n 657 | };\n 658 | \n 659 | let mut encoder = Encoder::length(0);\n 660 | let mut allowed_trailer_fields: Option<Vec<HeaderName>> = None;\n 661 | let mut wrote_date = false;\n 662 | let mut cur_name = None;\n 663 | let mut is_name_written = false;\n 664 | let mut must_write_chunked = false;\n 665 | let mut prev_con_len = None;\n 666 | \n 667 | macro_rules! handle_is_name_written {\n 668 | () => {{\n 669 | if is_name_written {\n 670 | // we need to clean up and write the newline\n 671 | debug_assert_ne!(\n 672 | &dst[dst.len() - 2..],\n 673 | b\"\\r\\n\",\n 674 | \"previous header wrote newline but set is_name_written\"\n 675 | );\n 676 | \n 677 | if must_write_chunked {\n 678 | extend(dst, b\", chunked\\r\\n\");\n 679 | } else {\n 680 | extend(dst, b\"\\r\\n\");\n 681 | }\n 682 | }\n 683 | }};\n 684 | }\n 685 | \n 686 | 'headers: for (opt_name, value) in msg.head.headers.drain() {\n 687 | if let Some(n) = opt_name {\n 688 | cur_name = Some(n);\n 689 | handle_is_name_written!();\n 690 | is_name_written = false;\n 691 | }\n 692 | let name = cur_name.as_ref().expect(\"current header name\");\n 693 | match *name {\n 694 | header::CONTENT_LENGTH => {\n 695 | if wrote_len && !is_name_written {\n 696 | warn!(\"unexpected content-length found, canceling\");\n 697 | rewind(dst);\n 698 | return Err(crate::Error::new_user_header());\n 699 | }\n 700 | match msg.body {\n 701 | Some(BodyLength::Known(known_len)) => {\n 702 | // The Body claims to know a length, and\n 703 | // the headers are already set. For performance\n 704 | // reasons, we are just going to trust that\n 705 | // the values match.\n 706 | //\n 707 | // In debug builds, we'll assert they are the\n 708 | // same to help developers find bugs.\n 709 | #[cfg(debug_assertions)]\n 710 | {\n 711 | if let Some(len) = headers::content_length_parse(&value) {\n 712 | if msg.req_method != &Some(Method::HEAD) || known_len != 0 {\n 713 | assert!(\n 714 | len == known_len,\n 715 | \"payload claims content-length of {}, custom content-length header claims {}\",\n 716 | known_len,\n 717 | len,\n 718 | );\n 719 | }\n 720 | }\n 721 | }\n 722 | \n 723 | if !is_name_written {\n 724 | encoder = Encoder::length(known_len);\n 725 | header_name_writer.write_header_name_with_colon(\n 726 | dst,\n 727 | \"content-length: \",\n 728 | header::CONTENT_LENGTH,\n 729 | );\n 730 | extend(dst, value.as_bytes());\n 731 | wrote_len = true;\n 732 | is_name_written = true;\n 733 | }\n 734 | continue 'headers;\n 735 | }\n 736 | Some(BodyLength::Unknown) => {\n 737 | // The Body impl didn't know how long the\n 738 | // body is, but a length header was included.\n 739 | // We have to parse the value to return our\n 740 | // Encoder...\n 741 | \n 742 | if let Some(len) = headers::content_length_parse(&value) {\n 743 | if let Some(prev) = prev_con_len {\n 744 | if prev != len {\n 745 | warn!(\n 746 | \"multiple Content-Length values found: [{}, {}]\",\n 747 | prev, len\n 748 | );\n 749 | rewind(dst);\n 750 | return Err(crate::Error::new_user_header());\n 751 | }\n 752 | debug_assert!(is_name_written);\n 753 | continue 'headers;\n 754 | } else {\n 755 | // we haven't written content-length yet!\n 756 | encoder = Encoder::length(len);\n 757 | header_name_writer.write_header_name_with_colon(\n 758 | dst,\n 759 | \"content-length: \",\n 760 | header::CONTENT_LENGTH,\n 761 | );\n 762 | extend(dst, value.as_bytes());\n 763 | wrote_len = true;\n 764 | is_name_written = true;\n 765 | prev_con_len = Some(len);\n 766 | continue 'headers;\n 767 | }\n 768 | } else {\n 769 | warn!(\"illegal Content-Length value: {:?}\", value);\n 770 | rewind(dst);\n 771 | return Err(crate::Error::new_user_header());\n 772 | }\n 773 | }\n 774 | None => {\n 775 | // We have no body to actually send,\n 776 | // but the headers claim a content-length.\n 777 | // There's only 2 ways this makes sense:\n 778 | //\n 779 | // - The header says the length is `0`.\n 780 | // - This is a response to a `HEAD` request.\n 781 | if msg.req_method == &Some(Method::HEAD) {\n 782 | debug_assert_eq!(encoder, Encoder::length(0));\n 783 | } else {\n 784 | if value.as_bytes() != b\"0\" {\n 785 | warn!(\n 786 | \"content-length value found, but empty body provided: {:?}\",\n 787 | value\n 788 | );\n 789 | }\n 790 | continue 'headers;\n 791 | }\n 792 | }\n 793 | }\n 794 | wrote_len = true;\n 795 | }\n 796 | header::TRANSFER_ENCODING => {\n 797 | if wrote_len && !is_name_written {\n 798 | warn!(\"unexpected transfer-encoding found, canceling\");\n 799 | rewind(dst);\n 800 | return Err(crate::Error::new_user_header());\n 801 | }\n 802 | // check that we actually can send a chunked body...\n 803 | if msg.head.version == Version::HTTP_10\n 804 | || !Server::can_chunked(msg.req_method.as_ref(), msg.head.subject)\n 805 | {\n 806 | continue;\n 807 | }\n 808 | wrote_len = true;\n 809 | // Must check each value, because `chunked` needs to be the\n 810 | // last encoding, or else we add it.\n 811 | must_write_chunked = !headers::is_chunked_(&value);\n 812 | \n 813 | if !is_name_written {\n 814 | encoder = Encoder::chunked();\n 815 | is_name_written = true;\n 816 | header_name_writer.write_header_name_with_colon(\n 817 | dst,\n 818 | \"transfer-encoding: \",\n 819 | header::TRANSFER_ENCODING,\n 820 | );\n 821 | extend(dst, value.as_bytes());\n 822 | } else {\n 823 | extend(dst, b\", \");\n 824 | extend(dst, value.as_bytes());\n 825 | }\n 826 | continue 'headers;\n 827 | }\n 828 | header::CONNECTION => {\n 829 | if !is_last && headers::connection_close(&value) {\n 830 | is_last = true;\n 831 | }\n 832 | if !is_name_written {\n 833 | is_name_written = true;\n 834 | header_name_writer.write_header_name_with_colon(\n 835 | dst,\n 836 | \"connection: \",\n 837 | header::CONNECTION,\n 838 | );\n 839 | extend(dst, value.as_bytes());\n 840 | } else {\n 841 | extend(dst, b\", \");\n 842 | extend(dst, value.as_bytes());\n 843 | }\n 844 | continue 'headers;\n 845 | }\n 846 | header::DATE => {\n 847 | wrote_date = true;\n 848 | }\n 849 | header::TRAILER => {\n 850 | // check that we actually can send a chunked body...\n 851 | if msg.head.version == Version::HTTP_10\n 852 | || !Server::can_chunked(msg.req_method.as_ref(), msg.head.subject)\n 853 | {\n 854 | continue;\n 855 | }\n 856 | \n 857 | if !is_name_written {\n 858 | is_name_written = true;\n 859 | header_name_writer.write_header_name_with_colon(\n 860 | dst,\n 861 | \"trailer: \",\n 862 | header::TRAILER,\n 863 | );\n 864 | extend(dst, value.as_bytes());\n 865 | } else {\n 866 | extend(dst, b\", \");\n 867 | extend(dst, value.as_bytes());\n 868 | }\n 869 | \n 870 | // Parse the Trailer header value into HeaderNames.\n 871 | // The value may contain comma-separated names.\n 872 | // HeaderName normalizes to lowercase for case-insensitive matching.\n 873 | if let Ok(value_str) = value.to_str() {\n 874 | let names: Vec<HeaderName> = value_str\n 875 | .split(',')\n 876 | .filter_map(|s| HeaderName::from_bytes(s.trim().as_bytes()).ok())\n 877 | .collect();\n 878 | \n 879 | match allowed_trailer_fields {\n 880 | Some(ref mut fields) => {\n 881 | fields.extend(names);\n 882 | }\n 883 | None => {\n 884 | allowed_trailer_fields = Some(names);\n 885 | }\n 886 | }\n 887 | }\n 888 | \n 889 | continue 'headers;\n 890 | }\n 891 | _ => (),\n 892 | }\n 893 | //TODO: this should perhaps instead combine them into\n 894 | //single lines, as RFC7230 suggests is preferable.\n 895 | \n 896 | // non-special write Name and Value\n 897 | debug_assert!(\n 898 | !is_name_written,\n 899 | \"{:?} set is_name_written and didn't continue loop\",\n 900 | name,\n 901 | );\n 902 | header_name_writer.write_header_name(dst, name);\n 903 | extend(dst, b\": \");\n 904 | extend(dst, value.as_bytes());\n 905 | extend(dst, b\"\\r\\n\");\n 906 | }\n 907 | \n 908 | handle_is_name_written!();\n 909 | \n 910 | if !wrote_len {\n 911 | encoder = match msg.body {\n 912 | Some(BodyLength::Unknown) => {\n 913 | if msg.head.version == Version::HTTP_10\n 914 | || !Server::can_chunked(msg.req_method.as_ref(), msg.head.subject)\n 915 | {\n 916 | Encoder::close_delimited()\n 917 | } else {\n 918 | header_name_writer.write_full_header_line(\n 919 | dst,\n 920 | \"transfer-encoding: chunked\\r\\n\",\n 921 | (header::TRANSFER_ENCODING, \": chunked\\r\\n\"),\n 922 | );\n 923 | Encoder::chunked()\n 924 | }\n 925 | }\n 926 | None | Some(BodyLength::Known(0)) => {\n 927 | if Server::can_have_implicit_zero_content_length(\n 928 | msg.req_method.as_ref(),\n 929 | msg.head.subject,\n 930 | ) {\n 931 | header_name_writer.write_full_header_line(\n 932 | dst,\n 933 | \"content-length: 0\\r\\n\",\n 934 | (header::CONTENT_LENGTH, \": 0\\r\\n\"),\n 935 | )\n 936 | }\n 937 | Encoder::length(0)\n 938 | }\n 939 | Some(BodyLength::Known(len)) => {\n 940 | if !Server::can_have_content_length(msg.req_method.as_ref(), msg.head.subject) {\n 941 | Encoder::length(0)\n 942 | } else {\n 943 | header_name_writer.write_header_name_with_colon(\n 944 | dst,\n 945 | \"content-length: \",\n 946 | header::CONTENT_LENGTH,\n 947 | );\n 948 | extend(dst, ::itoa::Buffer::new().format(len).as_bytes());\n 949 | extend(dst, b\"\\r\\n\");\n 950 | Encoder::length(len)\n 951 | }\n 952 | }\n 953 | };\n 954 | }\n 955 | \n 956 | if !Server::can_have_body(msg.req_method.as_ref(), msg.head.subject) {\n 957 | trace!(\n 958 | \"server body forced to 0; method={:?}, status={:?}\",\n 959 | msg.req_method,\n 960 | msg.head.subject\n 961 | );\n 962 | encoder = Encoder::length(0);\n 963 | }\n 964 | \n 965 | // cached date is much faster than formatting every request\n 966 | // don't force the write if disabled\n 967 | if !wrote_date && msg.date_header {\n 968 | dst.reserve(date::DATE_VALUE_LENGTH + 8);\n 969 | header_name_writer.write_header_name_with_colon(dst, \"date: \", header::DATE);\n 970 | date::extend(dst);\n 971 | extend(dst, b\"\\r\\n\\r\\n\");\n 972 | } else {\n 973 | extend(dst, b\"\\r\\n\");\n 974 | }\n 975 | \n 976 | if encoder.is_chunked() {\n 977 | if let Some(allowed_trailer_fields) = allowed_trailer_fields {\n 978 | encoder = encoder.into_chunked_with_trailing_fields(allowed_trailer_fields);\n 979 | }\n 980 | }\n 981 | \n 982 | Ok(encoder.set_last(is_last))\n 983 | }","lines":344}]},{"id":"ARCH_4","type":"architecture","type_label":"Architecture","severity":"low","easy":false,"description":"In `client.rs`, `ConnMapErr` has a field `is_terminated: bool` annotated with `#[pin]` via pin_project (line 265-267). Booleans are `Unpin` by default and don't need to be pinned - pinning a bool is meaningless and generates unnecessary complexity in the projected types. This is misleading to future maintainers who may wonder why a primitive bool needs to be structurally pinned.","fix":"Remove the `#[pin]` annotation from `is_terminated: bool` - booleans are always `Unpin` and do not need pinning.","locations":[{"ref":"src/proto/h2/client.rs:255-268","code":" 255 | pin_project! {\n 256 | struct ConnMapErr<T, B>\n 257 | where\n 258 | B: Body,\n 259 | T: Read,\n 260 | T: Write,\n 261 | T: Unpin,\n 262 | {\n 263 | #[pin]\n 264 | conn: Either<Conn<T, B>, Connection<Compat<T>, SendBuf<<B as Body>::Data>>>,\n 265 | #[pin]\n 266 | is_terminated: bool,\n 267 | }\n 268 | }","lines":14}]},{"id":"ARCH_5","type":"architecture","type_label":"Architecture","severity":"low","easy":false,"description":"The OnUpgrade future in upgrade.rs stores its receiver behind Arc<Mutex<oneshot::Receiver<...>>>. The Clone derive on OnUpgrade means multiple clones can all poll the same oneshot::Receiver concurrently after locking. While the Mutex prevents data races, two concurrent polls on the same oneshot::Receiver are semantically problematic: only one will receive the value. The second poll after the first succeeds will return Poll::Pending forever (waker replaced) or get a spurious cancellation error. Since OnUpgrade is Clone and Future, this is a footgun.","fix":"Document clearly that only one clone of OnUpgrade should be awaited, and that cloning is provided for storage/sharing purposes only. Consider making the Future impl take &mut self and not being Clone, or returning an error if the mutex is already being polled by another task.","locations":[{"ref":"src/upgrade.rs:74-76","code":" 74 | pub struct OnUpgrade {\n 75 | rx: Option<Arc<Mutex<oneshot::Receiver<crate::Result<Upgraded>>>>>,\n 76 | }","lines":3},{"ref":"src/upgrade.rs:228-242","code":" 228 | fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {\n 229 | match self.rx {\n 230 | Some(ref rx) => Pin::new(&mut *rx.lock().panic_if_poisoned())\n 231 | .poll(cx)\n 232 | .map(|res| match res {\n 233 | Ok(Ok(upgraded)) => Ok(upgraded),\n 234 | Ok(Err(err)) => Err(err),\n 235 | Err(_oneshot_canceled) => {\n 236 | Err(crate::Error::new_canceled().with(UpgradeExpected))\n 237 | }\n 238 | }),\n 239 | None => Poll::Ready(Err(crate::Error::new_user_no_upgrade())),\n 240 | }\n 241 | }\n 242 | }","lines":15}]}]</script>
<script type="application/json" id="PI">{"name":"hyper","description":"","git_url":"https://github.com/hyperium/hyper.git","repo_url":"https://github.com/hyperium/hyper","commit":"e64ad1e1b37f2d60d963ac9c0259b46fae3084d5","commit_short":"e64ad1e1b3","commit_url":"https://github.com/hyperium/hyper/commit/e64ad1e1b37f2d60d963ac9c0259b46fae3084d5","branch":"master","generated":"2026-06-15"}</script>
<div class="app">
<div class="top">
<div>
<h1>Code Audit Report — hyper</h1>
<div class="meta">Generated 2026-06-15 — <span id="stats"></span></div>
</div>
<div class="top-right">
<input type="search" id="search" placeholder="Search findings…" autocomplete="off">
<label class="group-label">Group by
<select id="groupby">
<option value="type">Type</option>
<option value="severity">Severity</option>
<option value="file">File</option>
<option value="none">None (flat)</option>
</select>
</label>
</div>
</div>
<div class="project-info"><div class="pi-name">hyper</div><div class="pi-row"><a class="pi-repo-btn" href="https://github.com/hyperium/hyper" target="_blank" rel="noopener">🔗 https://github.com/hyperium/hyper</a><span><span class="pi-k">branch</span> <code>master</code></span><span><span class="pi-k">commit</span> <a href="https://github.com/hyperium/hyper/commit/e64ad1e1b37f2d60d963ac9c0259b46fae3084d5" target="_blank" rel="noopener"><code>e64ad1e1b3</code></a></span></div></div>
<div class="filterbar" id="filterbar"></div>
<div id="progress-wrap">
<div id="progress-bar"><div id="progress-fill"></div></div>
<span id="progress-text"></span>
<button class="action-btn" data-action="collapse-all">⊟ Collapse all</button>
<button class="action-btn" data-action="expand-all">⊞ Expand all</button>
<button class="action-btn" data-action="download-md">⬇ MD</button>
<button class="action-btn" data-action="download-md-short">⬇ MD Short</button>
<button class="action-btn" data-action="download-toml">⬇ TOML</button>
</div>
<div id="hidden-bar"></div>
<div id="content"></div>
</div>
<script>
(function(){
'use strict';
// Data
const FINDINGS = JSON.parse(document.getElementById('D').textContent);
const PROJECT_INFO = JSON.parse(document.getElementById('PI').textContent);
const SEVS = ['critical','high','medium','low','negligible'];
const STATUSES = ['open','fixed','wont-fix','false-positive'];
const ALL_TYPES = [...new Set(FINDINGS.map(f => f.type))].sort();
// Build lookup map for O(1) access
const FINDING_BY_ID = new Map(FINDINGS.map(f => [f.id, f]));
// Only severities actually present in the report — the filter bar hides the rest.
const PRESENT_SEVS = SEVS.filter(s => FINDINGS.some(f => (f.severity || 'medium') === s));
// Whether any finding is flagged easy — the "Quick wins" filter row is hidden otherwise.
const HAS_EASY = FINDINGS.some(f => f.easy);
const STORE_KEY = 'audit-status:' + location.href;
const STATUS_CLS = {open:'st-open', fixed:'st-fixed', 'wont-fix':'st-wont', 'false-positive':'st-fp'};
const STATUS_LBL = {open:'● open', fixed:'✓ fixed', 'wont-fix':"— won't fix", 'false-positive':'✗ false pos.'};
const STATUS_NEXT = {open:'fixed', fixed:'wont-fix', 'wont-fix':'false-positive', 'false-positive':'open'};
// State
const S = {
search: '', groupBy: 'type',
hiddenSev: new Set(), hiddenType: new Set(),
hiddenStatus: new Set(), hiddenGroup: new Set(),
collapsed: new Set(), easyOnly: false,
};
const statuses = {};
// Persistence
function loadStatuses() {
try { Object.assign(statuses, JSON.parse(localStorage.getItem(STORE_KEY) || '{}')); } catch {}
}
function saveStatuses() {
try { localStorage.setItem(STORE_KEY, JSON.stringify(statuses)); } catch {}
}
function statusOf(id) { return statuses[id] || 'open'; }
// URL hash — debounced so rapid filter changes don't thrash history
let _hashTimer = 0;
function syncHash() {
clearTimeout(_hashTimer);
_hashTimer = setTimeout(() => {
const p = new URLSearchParams();
if (S.groupBy !== 'type') p.set('g', S.groupBy);
if (S.hiddenSev.size) p.set('hs', [...S.hiddenSev].join(','));
if (S.hiddenType.size) p.set('ht', [...S.hiddenType].join(','));
if (S.hiddenStatus.size) p.set('hst', [...S.hiddenStatus].join(','));
if (S.easyOnly) p.set('e', '1');
if (S.search) p.set('q', S.search);
const str = p.toString();
history.replaceState(null, '', str ? '#' + str : location.pathname + location.search);
}, 300);
}
function loadHash() {
const p = new URLSearchParams(location.hash.slice(1));
if (p.has('g')) { S.groupBy = p.get('g'); document.getElementById('groupby').value = S.groupBy; }
if (p.has('q')) { S.search = p.get('q'); document.getElementById('search').value = S.search; }
if (p.has('hs')) p.get('hs') .split(',').filter(Boolean).forEach(v => S.hiddenSev.add(v));
if (p.has('ht')) p.get('ht') .split(',').filter(Boolean).forEach(v => S.hiddenType.add(v));
if (p.has('hst')) p.get('hst').split(',').filter(Boolean).forEach(v => S.hiddenStatus.add(v));
if (p.has('e')) S.easyOnly = p.get('e') === '1';
}
// Helpers
function esc(s) {
return String(s).replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>').replace(/"/g,'"');
}
function highlight(text, q) {
if (!q) return esc(text);
const re = new RegExp('(' + q.replace(/[.*+?^${}()|[\]\\]/g,'\\$&') + ')', 'gi');
return String(text).split(re).map((p, i) => i % 2 ? '<mark>' + esc(p) + '</mark>' : esc(p)).join('');
}
// Render text with ```fenced``` code blocks and `inline` backticks,
// also paragraph-splitting on blank lines. Output is HTML-safe.
function renderMd(text, q) {
const src = String(text == null ? '' : text);
const FENCE = /```[ \t]*([A-Za-z0-9_+-]*)[ \t]*\r?\n([\s\S]*?)```/g;
const parts = [];
let last = 0, m;
while ((m = FENCE.exec(src)) !== null) {
parts.push({kind: 'text', value: src.slice(last, m.index)});
parts.push({kind: 'code', value: m[2].replace(/\r?\n$/, '')});
last = m.index + m[0].length;
}
parts.push({kind: 'text', value: src.slice(last)});
function renderInline(s) {
const re = /`([^`\n]+)`/g;
let out = '', i = 0, mm;
while ((mm = re.exec(s)) !== null) {
out += highlight(s.slice(i, mm.index), q);
out += '<code class="md-inline">' + highlight(mm[1], q) + '</code>';
i = mm.index + mm[0].length;
}
out += highlight(s.slice(i), q);
return out;
}
function renderTextChunk(s) {
const paragraphs = s.split(/\r?\n[ \t]*\r?\n/);
const out = [];
for (const p of paragraphs) {
const trimmed = p.replace(/^\s+|\s+$/g, '');
if (!trimmed) continue;
out.push('<p>' + renderInline(trimmed).replace(/\r?\n/g, '<br>') + '</p>');
}
return out.join('');
}
let html = '';
for (const p of parts) {
if (p.kind === 'text') html += renderTextChunk(p.value);
else html += '<pre class="md-code">' + highlight(p.value, q) + '</pre>';
}
return html;
}
function sevCls(s) { return SEVS.includes(s) ? 'sev-'+s : 'sev-negligible'; }
function sevLbl(s) { return s ? s[0].toUpperCase()+s.slice(1) : 'Medium'; }
function typeLabel(t) { return t.replace(/_/g,' ').replace(/\b\w/g, c => c.toUpperCase()); }
// First sentence of text (up to and including the first period).
function firstSentence(text) {
const m = String(text).match(/\.(\s|$)/);
return m ? text.slice(0, text.indexOf(m[0]) + 1) : text;
}
// Build full MD report (mirrors to_markdown in Python).
function buildMdFull() {
const SORD = ['critical','high','medium','low','negligible'];
const today = new Date().toISOString().slice(0, 10);
let out = '# Code Audit Report\n\n_Generated ' + today + '_\n\n';
const typeMap = new Map();
for (const f of FINDINGS) {
if (!typeMap.has(f.type)) typeMap.set(f.type, []);
typeMap.get(f.type).push(f);
}
const sortedTypes = [...typeMap.keys()].sort();
out += '## Summary\n\n| Type | Severity | Count |\n|------|----------|-------|\n';
for (const t of sortedTypes) {
const items = typeMap.get(t);
const topSev = items.map(f => f.severity || 'medium').reduce((best, s) => {
const bi = SORD.indexOf(best), si = SORD.indexOf(s);
return (si >= 0 && (bi < 0 || si < bi)) ? s : best;
}, 'negligible');
out += '| ' + typeLabel(t) + ' | ' + topSev + ' | ' + items.length + ' |\n';
}
for (const t of sortedTypes) {
out += '\n## ' + typeLabel(t) + '\n\n';
for (const f of typeMap.get(t)) {
const sevStr = f.severity ? ' `' + f.severity.toUpperCase() + '`' : '';
const easyStr = f.easy ? ' ⚡ easy' : '';
out += '### ' + f.id + sevStr + easyStr + '\n\n';
out += '**Description:** ' + (f.description || '') + '\n\n';
if (f.locations.length) {
out += '**Locations:**\n\n';
for (const l of f.locations) {
if (l.code != null) out += '```\n' + l.ref + '\n' + l.code + '\n```\n\n';
else out += '- `' + l.ref + '` _(file not found)_\n\n';
}
}
out += '**Fix:** ' + (f.fix || '') + '\n\n---\n\n';
}
}
return out;
}
// Build MD short checklist (mirrors to_md_short in Python).
function buildMdShort() {
const lines = [];
const ids = [];
const sorted = [...FINDINGS].sort((a, b) => a.type.localeCompare(b.type));
for (const f of sorted) {
ids.push(f.id);
const desc = firstSentence(f.description || '');
const locs = f.locations.map(l => l.ref);
const locStr = locs.length ? ' - [' + locs.join(', ') + ']' : '';
const sevStr = f.severity ? ' [' + f.severity.toUpperCase() + ']' : '';
const easyStr = f.easy ? ' ⚡' : '';
lines.push('- [ ] - ' + f.id + sevStr + easyStr + ' - ' + desc + locStr);
}
lines.push('', '## IDs only', '');
ids.forEach(id => lines.push('- [ ] - ' + id));
return lines.join('\n') + '\n';
}
function downloadText(text, filename, mime) {
const a = document.createElement('a');
a.href = URL.createObjectURL(new Blob([text], {type: mime || 'text/plain'}));
a.download = filename;
a.click();
setTimeout(() => URL.revokeObjectURL(a.href), 1000);
}
function tomlStr(s) {
return '"' + String(s == null ? '' : s)
.replace(/\\/g, '\\\\')
.replace(/"/g, '\\"')
.replace(/\n/g, '\\n')
.replace(/\r/g, '\\r')
.replace(/\t/g, '\\t') + '"';
}
// Build TOML matching the original findings.toml format (re-parseable by toml_to_report.py).
function buildFindingsToml() {
function pad(key) { return (key + ' ').slice(0, 12); }
const chunks = [];
for (const f of FINDINGS) {
const lns = ['[[finding]]'];
lns.push(pad('type') + '= ' + tomlStr(f.type));
lns.push(pad('severity') + '= ' + tomlStr(f.severity || 'medium'));
if (f.easy) lns.push(pad('easy') + '= true');
lns.push(pad('description') + '= ' + tomlStr(f.description || ''));
const refs = f.locations.map(l => tomlStr(l.ref));
lns.push(pad('locations') + '= [' + refs.join(', ') + ']');
lns.push(pad('fix') + '= ' + tomlStr(f.fix || ''));
chunks.push(lns.join('\n'));
}
return chunks.join('\n\n') + '\n';
}
// Build project-info.toml from embedded PROJECT_INFO (same format as gen_project_info.py).
function buildProjectInfoToml() {
const ORDER = ['name','description','git_url','repo_url','commit','commit_short','commit_url','branch','generated'];
const pi = Object.assign({}, PROJECT_INFO);
if (!('description' in pi)) pi.description = '';
const lines = [
'# Project metadata for the code audit report (all fields optional).',
'# Hand-edit freely (e.g. fill in description); re-run the audit without --force to keep edits.',
'', '[project]'
];
for (const k of ORDER) {
if (k in pi) lines.push(k + ' = ' + tomlStr(pi[k]));
}
return lines.join('\n') + '\n';
}
// Build the GitHub-ready Markdown for one finding (mirrors findings.md).
function findingToMd(f) {
let out = '### ' + f.id + (f.severity ? ' `' + f.severity.toUpperCase() + '`' : '') + (f.easy ? ' ⚡ easy' : '') + '\n\n';
out += '**Description:** ' + (f.description || '') + '\n\n';
if (f.locations.length) {
out += '**Locations:**\n\n';
for (const l of f.locations) {
if (l.code != null) out += '```\n' + l.ref + '\n' + l.code + '\n```\n\n';
else out += '- `' + l.ref + '` _(file not found)_\n\n';
}
}
out += '**Fix:** ' + (f.fix || '') + '\n';
return out;
}
function flashBtn(btn, msg) {
btn.textContent = msg;
setTimeout(() => { btn.textContent = '⧉ Copy'; }, 1200);
}
function copyText(text, btn) {
const ok = () => flashBtn(btn, '✓ Copied');
const no = () => flashBtn(btn, '✗ Failed');
const fallback = () => {
try {
const ta = document.createElement('textarea');
ta.value = text; ta.style.position = 'fixed'; ta.style.top = '-9999px';
document.body.appendChild(ta); ta.select();
const r = document.execCommand('copy');
document.body.removeChild(ta);
r ? ok() : no();
} catch { no(); }
};
if (navigator.clipboard && navigator.clipboard.writeText)
navigator.clipboard.writeText(text).then(ok, fallback);
else fallback();
}
// Filter logic
function getKey(f) {
if (S.groupBy === 'severity') return f.severity || 'medium';
if (S.groupBy === 'file') return f.locations.length ? f.locations[0].ref.replace(/:.*/,'') : '(no location)';
if (S.groupBy === 'none') return '';
return f.type_label;
}
// Pre-compute search haystack per finding (cleared on full re-render)
const _hay = new Map();
function hayOf(f) {
if (!_hay.has(f.id)) {
_hay.set(f.id, [f.id, f.type_label, f.severity, f.description, f.fix,
f.easy ? 'easy quick win' : '',
...f.locations.map(l => l.ref)].join(' ').toLowerCase());
}
return _hay.get(f.id);
}
function matches(f) {
if (S.easyOnly && !f.easy) return false;
if (S.hiddenSev.has(f.severity || 'medium')) return false;
if (S.hiddenType.has(f.type)) return false;
const st = statusOf(f.id);
if (S.hiddenStatus.has(st)) return false;
if (S.search) {
const q = S.search.toLowerCase();
if (!hayOf(f).includes(q) && !st.includes(q)) return false;
}
return true;
}
function sortKeys(keys) {
if (S.groupBy === 'severity') {
return [...keys].sort((a, b) => {
const ai = SEVS.indexOf(a), bi = SEVS.indexOf(b);
return (ai < 0 ? 99 : ai) - (bi < 0 ? 99 : bi);
});
}
return [...keys].sort();
}
// Build body HTML for an expanded card (called lazily on first expand).
function renderCardBody(f, q) {
let locs = '';
for (const l of f.locations) {
const lineInfo = l.lines != null
? ' <span class="loc-lines">(' + l.lines + ' line' + (l.lines === 1 ? '' : 's') + ')</span>'
: '';
locs += '<div class="loc-block"><div class="loc-ref">' + highlight(l.ref, q) + lineInfo + '</div>';
locs += l.code != null ? '<pre>' + esc(l.code) + '</pre>' : '<div class="not-found">(file not found)</div>';
locs += '</div>';
}
return (
'<div class="field-lbl">Description</div>' +
'<div class="desc">' + renderMd(f.description, q) + '</div>' +
(locs ? '<div class="field-lbl">Locations</div>' + locs : '') +
'<div class="field-lbl">Fix</div>' +
'<div class="fix-txt">' + renderMd(f.fix, q) + '</div>'
);
}
// Render card shell only — header + empty or populated body.
// When collapsed the body is empty (display:none) — avoids renderMd for 1800+ cards.
function renderCard(f, q) {
const sc = sevCls(f.severity || 'medium');
const sl = sevLbl(f.severity || 'medium');
const st = statusOf(f.id);
const sc2 = STATUS_CLS[st] || 'st-open';
const sl2 = STATUS_LBL[st] || '● open';
const isCollapsed = S.collapsed.has(f.id);
const isResolved = st !== 'open';
const locSummary = f.locations.length
? f.locations[0].ref + (f.locations.length > 1 ? ' \u2026' : '')
: '';
const bodyInner = isCollapsed ? '' : renderCardBody(f, q);
return (
'<div class="card' + (isResolved ? ' resolved' : '') + '" data-card-id="' + esc(f.id) + '">' +
'<div class="card-header" data-action="toggle-card" data-id="' + esc(f.id) + '">' +
'<span class="card-arrow">' + (isCollapsed ? '\u25b6' : '\u25bc') + '</span>' +
'<span class="card-id">' + esc(f.id) + '</span>' +
'<span class="badge ' + sc + '" style="cursor:default">' + esc(sl) + '</span>' +
'<span class="badge type-badge" style="cursor:default">' + esc(f.type_label) + '</span>' +
(f.easy ? '<span class="badge easy-badge" style="cursor:default" title="Easy to verify & fix - no deep knowledge of the code, SDK or technology required">\u26a1 Easy</span>' : '') +
(locSummary ? '<span class="card-loc" title="' + esc(locSummary) + '">' + esc(locSummary) + '</span>' : '') +
'<button class="status-btn ' + sc2 + '" data-action="cycle-status" data-id="' + esc(f.id) + '">' + sl2 + '</button>' +
'<button class="copy-btn" data-action="copy-md" data-id="' + esc(f.id) + '" title="Copy as Markdown for GitHub">\u29c9 Copy</button>' +
'</div>' +
'<div class="card-body" data-body-id="' + esc(f.id) + '"' + (isCollapsed ? ' style="display:none"' : '') + '>' +
bodyInner +
'</div></div>'
);
}
function initFilterBar() {
const fb = document.getElementById('filterbar');
let h = '<div class="filter-row"><span class="filter-lbl">Severity:</span>';
for (const s of PRESENT_SEVS)
h += '<button class="badge ' + sevCls(s) + '" data-sev="' + s + '">' + sevLbl(s) + '</button>';
h += '<button class="filter-toggle" data-toggle-all="sev"></button></div>';
h += '<div class="filter-row"><span class="filter-lbl">Type:</span>';
for (const t of ALL_TYPES)
h += '<button class="badge type-badge" data-type="' + esc(t) + '">' + esc(typeLabel(t)) + '</button>';
h += '<button class="filter-toggle" data-toggle-all="type"></button></div>';
h += '<div class="filter-row"><span class="filter-lbl">Status:</span>';
for (const st of STATUSES)
h += '<button class="badge ' + (STATUS_CLS[st]||'st-open') + '" data-status="' + st + '">' + (STATUS_LBL[st]||st) + '</button>';
h += '<button class="filter-toggle" data-toggle-all="status"></button></div>';
if (HAS_EASY)
h += '<div class="filter-row"><span class="filter-lbl">Quick wins:</span>'
+ '<button class="badge easy-badge" data-easy="1">\u26a1 Easy only</button></div>';
fb.innerHTML = h;
}
function updateToggles() {
for (const b of document.querySelectorAll('[data-sev]')) b.classList.toggle('off', S.hiddenSev.has(b.dataset.sev));
for (const b of document.querySelectorAll('[data-type]')) b.classList.toggle('off', S.hiddenType.has(b.dataset.type));
for (const b of document.querySelectorAll('[data-status]')) b.classList.toggle('off', S.hiddenStatus.has(b.dataset.status));
for (const b of document.querySelectorAll('[data-easy]')) b.classList.toggle('off', !S.easyOnly);
for (const b of document.querySelectorAll('[data-toggle-all]')) {
const k = b.dataset.toggleAll;
const set = k === 'sev' ? S.hiddenSev : k === 'type' ? S.hiddenType : S.hiddenStatus;
b.textContent = set.size === 0 ? 'Hide all' : 'Show all';
}
}
function updateProgress() {
const total = FINDINGS.length;
const done = FINDINGS.filter(f => statusOf(f.id) !== 'open').length;
const pct = total ? Math.round(done / total * 100) : 0;
document.getElementById('progress-fill').style.width = pct + '%';
document.getElementById('progress-text').textContent = done + ' / ' + total + ' resolved (' + pct + '%)';
}
// Surgical toggle: flip one card without touching the rest of the DOM.
function toggleCard(id) {
const wasCollapsed = S.collapsed.has(id);
if (wasCollapsed) S.collapsed.delete(id); else S.collapsed.add(id);
const body = document.querySelector('[data-body-id="' + id + '"]');
const arrow = document.querySelector('[data-card-id="' + id + '"] .card-arrow');
if (!body) { render(); return; }
if (wasCollapsed) {
if (!body.firstChild) {
const f = FINDING_BY_ID.get(id);
if (f) body.innerHTML = renderCardBody(f, S.search);
}
body.style.display = '';
if (arrow) arrow.textContent = '\u25bc';
} else {
body.style.display = 'none';
if (arrow) arrow.textContent = '\u25b6';
}
}
// Surgical status update: update one card's button + resolved class.
function cycleStatus(id) {
statuses[id] = STATUS_NEXT[statusOf(id)] || 'open';
saveStatuses();
const st = statusOf(id);
const btn = document.querySelector('.status-btn[data-id="' + id + '"]');
if (!btn) { render(); return; }
btn.className = 'status-btn ' + (STATUS_CLS[st] || 'st-open');
btn.textContent = STATUS_LBL[st] || '\u25cf open';
const card = document.querySelector('[data-card-id="' + id + '"]');
if (card) card.classList.toggle('resolved', st !== 'open');
updateProgress();
}
// Surgical collapse/expand for a group: toggle all bodies in the group without full re-render.
function collapseGroup(key, collapse) {
FINDINGS.forEach(f => {
if (getKey(f) !== key) return;
if (collapse) S.collapsed.add(f.id); else S.collapsed.delete(f.id);
const body = document.querySelector('[data-body-id="' + f.id + '"]');
const arrow = document.querySelector('[data-card-id="' + f.id + '"] .card-arrow');
if (!body) return;
if (collapse) {
body.style.display = 'none';
if (arrow) arrow.textContent = '\u25b6';
} else {
if (!body.firstChild) {
const ff = FINDING_BY_ID.get(f.id);
if (ff) body.innerHTML = renderCardBody(ff, S.search);
}
body.style.display = '';
if (arrow) arrow.textContent = '\u25bc';
}
});
}
// Full re-render: used for filter/group-by/search changes.
// Batched via requestAnimationFrame.
let _rafPending = false;
function scheduleRender() {
if (_rafPending) return;
_rafPending = true;
requestAnimationFrame(() => { _rafPending = false; render(); });
}
function render() {
_hay.clear();
updateToggles();
updateProgress();
const q = S.search;
const visible = FINDINGS.filter(matches);
document.getElementById('stats').textContent = visible.length + ' / ' + FINDINGS.length + ' findings';
// Build groups
const map = new Map();
for (const f of visible) {
const k = getKey(f);
if (!map.has(k)) map.set(k, []);
map.get(k).push(f);
}
// Hidden-groups bar
const hbar = document.getElementById('hidden-bar');
if (S.hiddenGroup.size > 0) {
let hh = '<div class="hidden-inner"><span class="hidden-lbl">Hidden:</span>';
for (const g of [...S.hiddenGroup].sort())
hh += '<button class="show-btn" data-action="show-group" data-key="' + esc(g) + '">' + esc(g || '(all findings)') + '</button>';
hh += '<button class="show-all-btn" data-action="show-all">Show all</button></div>';
hbar.innerHTML = hh;
} else {
hbar.innerHTML = '';
}
// Content
const content = document.getElementById('content');
if (!visible.length) {
content.innerHTML = '<div class="no-results">No findings match the current filters.</div>';
syncHash();
return;
}
let hh = '';
for (const k of sortKeys([...map.keys()])) {
if (S.hiddenGroup.has(k)) continue;
const items = map.get(k);
const title = k || 'All Findings';
const done = items.filter(f => statusOf(f.id) !== 'open').length;
const allDone = done === items.length;
hh += '<div class="group-section">' +
'<div class="group-header">' +
'<span class="group-title' + (allDone ? ' all-done' : '') + '">' + esc(title) + '</span>' +
'<span class="group-count">' + done + '/' + items.length + '</span>' +
'<button class="icon-btn" data-action="collapse-group" data-key="' + esc(k) + '" title="Collapse all">\u229f</button>' +
'<button class="icon-btn" data-action="expand-group" data-key="' + esc(k) + '" title="Expand all">\u229e</button>' +
'<button class="hide-btn" data-action="hide-group" data-key="' + esc(k) + '">Hide</button>' +
'</div>';
for (const f of items) hh += renderCard(f, q);
hh += '</div>';
}
content.innerHTML = hh;
syncHash();
}
// Event delegation
document.getElementById('filterbar').addEventListener('click', e => {
const b = e.target.closest('[data-sev],[data-type],[data-status],[data-toggle-all],[data-easy]');
if (!b) return;
if (b.dataset.easy) {
S.easyOnly = !S.easyOnly;
}
else if (b.dataset.toggleAll) {
const k = b.dataset.toggleAll;
const set = k === 'sev' ? S.hiddenSev : k === 'type' ? S.hiddenType : S.hiddenStatus;
const full = k === 'sev' ? PRESENT_SEVS : k === 'type' ? ALL_TYPES : STATUSES;
if (set.size === 0) full.forEach(v => set.add(v));
else set.clear();
}
else if (b.dataset.sev) { const s = S.hiddenSev; s.has(b.dataset.sev) ? s.delete(b.dataset.sev) : s.add(b.dataset.sev); }
else if (b.dataset.type) { const s = S.hiddenType; s.has(b.dataset.type) ? s.delete(b.dataset.type) : s.add(b.dataset.type); }
else if (b.dataset.status) { const s = S.hiddenStatus; s.has(b.dataset.status) ? s.delete(b.dataset.status) : s.add(b.dataset.status); }
scheduleRender();
});
document.addEventListener('click', e => {
const b = e.target.closest('[data-action]');
if (!b) return;
const { action, key, id } = b.dataset;
if (action === 'hide-group') { S.hiddenGroup.add(key); scheduleRender(); }
else if (action === 'show-group') { S.hiddenGroup.delete(key); scheduleRender(); }
else if (action === 'show-all') { S.hiddenGroup.clear(); scheduleRender(); }
else if (action === 'collapse-all') { FINDINGS.forEach(f => S.collapsed.add(f.id)); scheduleRender(); }
else if (action === 'expand-all') { S.collapsed.clear(); scheduleRender(); }
else if (action === 'copy-md') {
e.stopPropagation();
const f = FINDING_BY_ID.get(id);
if (f) copyText(findingToMd(f), b);
}
else if (action === 'download-md') { downloadText(buildMdFull(), 'findings.md'); }
else if (action === 'download-md-short') { downloadText(buildMdShort(), 'findings-short.md'); }
else if (action === 'download-toml') {
downloadText(buildFindingsToml(), 'findings.toml', 'application/toml');
downloadText(buildProjectInfoToml(), 'project-info.toml', 'application/toml');
}
else if (action === 'cycle-status') {
e.stopPropagation();
cycleStatus(id);
}
else if (action === 'toggle-card') {
toggleCard(id);
}
else if (action === 'collapse-group') {
collapseGroup(key, true);
}
else if (action === 'expand-group') {
collapseGroup(key, false);
}
});
// Debounced search — 180 ms after last keystroke
let _searchTimer = 0;
document.getElementById('search').addEventListener('input', e => {
S.search = e.target.value;
clearTimeout(_searchTimer);
_searchTimer = setTimeout(scheduleRender, 180);
});
document.getElementById('groupby').addEventListener('change', e => {
S.groupBy = e.target.value;
S.hiddenGroup.clear();
scheduleRender();
});
// Init
loadStatuses();
initFilterBar();
loadHash();
FINDINGS.forEach(f => S.collapsed.add(f.id));
render();
})();
</script>
</body></html>