Skip to content

Commit

Permalink
BUG/MEDIUM: quic: Possible crash during retransmissions and heavy load
Browse files Browse the repository at this point in the history 
This bug could be reproduced with -dMfail and dectected by libasan as follows:

$ ASAN_OPTIONS=disable_coredump=0:unmap_shadow_on_exit=1:abort_on_error=f quic-freeze.cfg -dMfail -dMno-cache -dM0x55
=================================================================
==82989==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffc 0x560790cc4749 bp 0x7fff8e0e8e30 sp 0x7fff8e0e8e28
WRITE of size 8 at 0x7fff8e0ea338 thread T0
    #0 0x560790cc4748 in qc_frm_free src/quic_frame.c:1222
    joyent#1 0x560790cc5260 in qc_release_frm src/quic_frame.c:1261
    joyent#2 0x560790d1de99 in qc_treat_acked_tx_frm src/quic_rx.c:312
    haproxy#3 0x560790d1e708 in qc_ackrng_pkts src/quic_rx.c:370
    haproxy#4 0x560790d22a1d in qc_parse_ack_frm src/quic_rx.c:694
    haproxy#5 0x560790d25daa in qc_parse_pkt_frms src/quic_rx.c:988
    haproxy#6 0x560790d2a509 in qc_treat_rx_pkts src/quic_rx.c:1373
    haproxy#7 0x560790c72d45 in quic_conn_io_cb src/quic_conn.c:906
    haproxy#8 0x560791207847 in run_tasks_from_lists src/task.c:596
    haproxy#9 0x5607912095f0 in process_runnable_tasks src/task.c:876
    haproxy#10 0x560791135564 in run_poll_loop src/haproxy.c:2966
    haproxy#11 0x5607911363af in run_thread_poll_loop src/haproxy.c:3165
    haproxy#12 0x56079113938c in main src/haproxy.c:3862
    haproxy#13 0x7f92606edd09 in __libc_start_main ../csu/libc-start.c:308
    haproxy#14 0x560790bcd529 in _start (/home/flecaille/src/haproxy/haproxy+0x

Address 0x7fff8e0ea338 is located in stack of thread T0 at offset 1032 i
    #0 0x560790d29b52 in qc_treat_rx_pkts src/quic_rx.c:1341

  This frame has 2 object(s):
    [32, 48) 'ar' (line 1380)
    [64, 1088) '_msg' (line 1368) <== Memory access at offset 1032 is inable
HINT: this may be a false positive if your program uses some custom stacnism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-scope src/quic_frame.c:1222 i
Shadow bytes around the buggy address:
  0x100071c15410: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x100071c15420: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x100071c15430: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x100071c15440: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x100071c15450: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
=>0x100071c15460: f8 f8 f8 f8 f8 f8 f8[f8]f8 f8 f8 f8 f8 f8 f3 f3
  0x100071c15470: f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 f3 00 00
  0x100071c15480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100071c15490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100071c154a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100071c154b0: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 04 f3 f3 f3
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==82989==ABORTING
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer:DEADLYSIGNAL
Aborted (core dumped)

Note that a coredump could not always be produced with all compilers. This was
always the case with clang 11.

When allocating frames to be retransmitted from qc_dgrams_retransmit(), if they
could not be sent for any reason, they could remain attached to a local list to
qc_dgrams_retransmit() and trigger a crash with libasan when releasing the
original frames they were duplicated from.

To fix this, always release the frames which could not be sent during
retransmissions calling qc_free_frm_list() where needed.

Must be backported as far as 2.6.

(cherry picked from commit dc8a20b)
[cf: Adapted with Fred's help]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
  • Loading branch information
@haproxyFred @capflam
haproxyFred authored and capflam committed Dec 5, 2023
1 parent f5c2470 commit fe97c6b
Showing 1 changed file with 21 additions and 23 deletions.
44 changes: 21 additions & 23 deletions src/quic_conn.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2708,7 +2708,7 @@ static void qc_dup_pkt_frms(struct quic_conn *qc,
TRACE_DEVEL("built probing frame", QUIC_EV_CONN_PRSAFRM, qc, origin);
if (origin->pkt) {
TRACE_DEVEL("duplicated from packet", QUIC_EV_CONN_PRSAFRM,
qc, NULL, &origin->pkt->pn_node.key);
qc, dup_frm, &origin->pkt->pn_node.key);
}
else {
/* <origin> is a frame which was sent from a packet detected as lost. */
Expand Down Expand Up @@ -4962,6 +4962,7 @@ int qc_send_hdshk_pkts(struct quic_conn *qc, int old_data,
static int qc_dgrams_retransmit(struct quic_conn *qc)
{
int ret = 0;
int sret;
struct quic_enc_level *iqel = &qc->els[QUIC_TLS_ENC_LEVEL_INITIAL];
struct quic_enc_level *hqel = &qc->els[QUIC_TLS_ENC_LEVEL_HANDSHAKE];
struct quic_enc_level *aqel = &qc->els[QUIC_TLS_ENC_LEVEL_APP];
Expand All @@ -4982,18 +4983,20 @@ static int qc_dgrams_retransmit(struct quic_conn *qc)
iqel->pktns->tx.pto_probe = 1;
if (!LIST_ISEMPTY(&hfrms))
hqel->pktns->tx.pto_probe = 1;
if (!qc_send_hdshk_pkts(qc, 1, QUIC_TLS_ENC_LEVEL_INITIAL, &ifrms,
QUIC_TLS_ENC_LEVEL_HANDSHAKE, &hfrms))
sret = qc_send_hdshk_pkts(qc, 1, QUIC_TLS_ENC_LEVEL_INITIAL, &ifrms,
QUIC_TLS_ENC_LEVEL_HANDSHAKE, &hfrms);
qc_free_frm_list(&ifrms);
qc_free_frm_list(&hfrms);
if (!sret)
goto leave;
/* Put back unsent frames in their packet number spaces */
LIST_SPLICE(&iqel->pktns->tx.frms, &ifrms);
LIST_SPLICE(&hqel->pktns->tx.frms, &hfrms);
}
else {
if (!(qc->flags & QUIC_FL_CONN_ANTI_AMPLIFICATION_REACHED)) {
iqel->pktns->tx.pto_probe = 1;
if (!qc_send_hdshk_pkts(qc, 0, QUIC_TLS_ENC_LEVEL_INITIAL, &ifrms,
QUIC_TLS_ENC_LEVEL_NONE, NULL))
sret = qc_send_hdshk_pkts(qc, 0, QUIC_TLS_ENC_LEVEL_INITIAL, &ifrms,
QUIC_TLS_ENC_LEVEL_NONE, NULL);
qc_free_frm_list(&hfrms);
if (!sret)
goto leave;
}
}
Expand All @@ -5015,12 +5018,11 @@ static int qc_dgrams_retransmit(struct quic_conn *qc)
TRACE_DEVEL("Avail. ack eliciting frames", QUIC_EV_CONN_FRMLIST, qc, &frms1);
if (!LIST_ISEMPTY(&frms1)) {
hqel->pktns->tx.pto_probe = 1;
if (!qc_send_hdshk_pkts(qc, 1, QUIC_TLS_ENC_LEVEL_HANDSHAKE, &frms1,
QUIC_TLS_ENC_LEVEL_NONE, NULL))
sret = qc_send_hdshk_pkts(qc, 1, QUIC_TLS_ENC_LEVEL_HANDSHAKE, &frms1,
QUIC_TLS_ENC_LEVEL_NONE, NULL);
qc_free_frm_list(&frms1);
if (!sret)
goto leave;

/* Put back unsent frames into their packet number spaces */
LIST_SPLICE(&hqel->pktns->tx.frms, &frms1);
}
}
TRACE_STATE("no more need to probe Handshake packet number space",
Expand All @@ -5037,23 +5039,19 @@ static int qc_dgrams_retransmit(struct quic_conn *qc)
TRACE_PROTO("Avail. ack eliciting frames", QUIC_EV_CONN_FRMLIST, qc, &frms2);
if (!LIST_ISEMPTY(&frms1)) {
aqel->pktns->tx.pto_probe = 1;
if (!qc_send_app_probing(qc, &frms1)) {
qc_free_frm_list(&frms1);
sret = qc_send_app_probing(qc, &frms1);
qc_free_frm_list(&frms1);
if (!sret) {
qc_free_frm_list(&frms2);
goto leave;
}

/* Put back unsent frames into their packet number spaces */
LIST_SPLICE(&aqel->pktns->tx.frms, &frms1);
}
if (!LIST_ISEMPTY(&frms2)) {
aqel->pktns->tx.pto_probe = 1;
if (!qc_send_app_probing(qc, &frms2)) {
qc_free_frm_list(&frms2);
sret = qc_send_app_probing(qc, &frms2);
qc_free_frm_list(&frms2);
if (!sret)
goto leave;
}
/* Put back unsent frames into their packet number spaces */
LIST_SPLICE(&aqel->pktns->tx.frms, &frms2);
}
TRACE_STATE("no more need to probe 01RTT packet number space",
QUIC_EV_CONN_TXPKT, qc);
Expand Down

0 comments on commit fe97c6b

Please sign in to comment.