]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
3 years agoMINOR: resolvers: move the resolv.conf parser in parse_resolv_conf()
William Lallemand [Thu, 5 May 2022 15:36:09 +0000 (17:36 +0200)] 
MINOR: resolvers: move the resolv.conf parser in parse_resolv_conf()

Move the resolv.conf parser from the cfg_parse_resolvers so it could be
used separately.

Some changes were made in the memprintf in order to use a char **
instead of a char *. Also the variable is tested before each memprintf
so could skip them if no warnmsg nor errmsg were set.

3 years agoMINOR: resolvers: cleanup alert/warning in parse-resolve-conf
William Lallemand [Thu, 5 May 2022 15:20:08 +0000 (17:20 +0200)] 
MINOR: resolvers: cleanup alert/warning in parse-resolve-conf

Cleanup the alert and warning handling in the "parse-resolve-conf"
parser to use the errmsg and warnmsg variables and memprintf.

This will allow to split the parser and shut the alert/warning if
needed.

3 years agoDOC: config: Update doc for PR/PH session states to warn about rewrite failures
Christopher Faulet [Thu, 5 May 2022 10:27:07 +0000 (12:27 +0200)] 
DOC: config: Update doc for PR/PH session states to warn about rewrite failures

When an HTTP header rewrite failure is triggered, and 500-internal-error
response is returned. A "PR" termination state is logged if the error
occurred on the request and "PH" if the error is reported for the response.

The documentation was updated accordingly.

This patch is related to issue #1597.

3 years agoBUG/MEDIUM: mux-h1: Be able to handle trailers when C-L header was specified
Christopher Faulet [Thu, 5 May 2022 07:39:42 +0000 (09:39 +0200)] 
BUG/MEDIUM: mux-h1: Be able to handle trailers when C-L header was specified

The commit 2eb5243e7 ("BUG/MEDIUM: mux-h1: Set outgoing message to DONE when
payload length is reached") introduced a regression. An internal error is
reported when we try to forward a message with trailers while the
content-length header was specified. Indeed, this case does not exist for H1
messages but it is possible in H2.

This patch should solve the issue #1684. It must be backported as far as
2.4.

3 years agoBUG/MEDIUM: mux-fcgi: Be sure to never set EOM flag on an empty HTX message
Christopher Faulet [Thu, 5 May 2022 07:24:52 +0000 (09:24 +0200)] 
BUG/MEDIUM: mux-fcgi: Be sure to never set EOM flag on an empty HTX message

This bug was already fixed at many places (stats, promex, lua) but the FCGI
multiplexer is also affected. When there is no content-length specified in
the response and when the END_REQUEST record is delayed, the response may be
truncated because an abort is erroneously detected. If the connection is not
closed because "keep-conn" option is set, the response is aborted at the end
of the server timeout.

This bug is a design issue with the HTX. It should be addressed. But it will
probably not be possible to backport them as far as 2.4. So, for now, the
only solution is to explicitly add an EOT block with the EOM flag in this
case.

This patch should fix the issue #1682. It must be backported as far as 2.4.

3 years agoBUG/MEDIUM: conn-stream: Only keep app layer flags of the endpoint on reset
Christopher Faulet [Wed, 4 May 2022 07:52:48 +0000 (09:52 +0200)] 
BUG/MEDIUM: conn-stream: Only keep app layer flags of the endpoint on reset

The commit a6c4a4834 ("BUG/MEDIUM: conn-stream: Don't erase endpoint flags
on reset") was too laxy on reset. Only app layer flags must be preserved. On
reset, the endpoint is detached. Thus all flags set by the endpoint itself
or concerning its type must be removed.

Without this fix, we can experienced crashes when a stream is released while
a server connection attempt failed. Indeed, in this case, endpoint of the
backend conn-stream is reset. But the endpoint type is still set. Thus when
the stream is released, the endpoint is detached again.

This patch is 2.6-specific. No backport needed. This commit depends on the
previous one ("MINOR: conn-stream: Add mask from flags set by endpoint or
app layer").

3 years agoMINOR: conn-stream: Add mask from flags set by endpoint or app layer
Christopher Faulet [Wed, 4 May 2022 07:19:13 +0000 (09:19 +0200)] 
MINOR: conn-stream: Add mask from flags set by endpoint or app layer

In flags set on the endpoints, some are set by endpoints itself and some are
set by the app layer. To help flags manipulations, 2 masks have been
added. The first one, CS_EP_ENDP_MASK, for all flags that an endpoint may
set. The other one, CS_EP_APP_MASK, for flags that the app layer may set.

This patch is mandatory for the next commit.

3 years agoDOC: configuration: httpclient global option
William Lallemand [Wed, 4 May 2022 16:14:25 +0000 (18:14 +0200)] 
DOC: configuration: httpclient global option

Documentation about the 4 options in the global section for the
httpclient:

- httpclient.ssl.verify
- httpclient.ssl.ca-file
- httpclient.resolvers.id
- httpclient.resolvers.prefer

3 years agoMINOR: httpclient: allow ipv4 or ipv6 preference for resolving
William Lallemand [Wed, 4 May 2022 13:59:44 +0000 (15:59 +0200)] 
MINOR: httpclient: allow ipv4 or ipv6 preference for resolving

The httpclient.resolvers.prefer global keyword allows to configure an
ipv4 or ipv6 preference when resolving. This could be useful in
environment where the ipv6 is not supported.

3 years agoMINOR: httpclient: configure the resolvers section to use
William Lallemand [Wed, 4 May 2022 14:10:47 +0000 (16:10 +0200)] 
MINOR: httpclient: configure the resolvers section to use

By default the httpclient uses the resolvers section whose ID is
"default", the httpclient.resolvers.id global option allows to configure
another section to use.

3 years agoMINOR: httpclient: allow to configure the ca-file
William Lallemand [Wed, 4 May 2022 13:43:01 +0000 (15:43 +0200)] 
MINOR: httpclient: allow to configure the ca-file

The global keyword httpclient.ssl.ca-file allows to configure the
ca-file used for the httpclient verify.

3 years agoMEDIUM: httpclient: hard-error when SSL is configured
William Lallemand [Wed, 4 May 2022 12:53:41 +0000 (14:53 +0200)] 
MEDIUM: httpclient: hard-error when SSL is configured

The hard_error_ssl flag is set when the configuration is explicitely
done for the ssl in the httpclient.

If no configuration was made, the features are simply disabled and no
alert is emitted.

3 years agoMINOR: httpclient: cleanup the error handling in init
William Lallemand [Wed, 4 May 2022 12:33:57 +0000 (14:33 +0200)] 
MINOR: httpclient: cleanup the error handling in init

Cleanup the error handling in the initialization so we rely on the
ERR_CODE and use memprintf() to set the errmsg before printing it at the
end of the functions.

3 years agoMINOR: init: exit() after pre-check upon error
William Lallemand [Wed, 4 May 2022 12:29:46 +0000 (14:29 +0200)] 
MINOR: init: exit() after pre-check upon error

Add a test on the err_code variable so we don't go further if one of the
pre-check callback failed.

3 years agoMINOR: httpclient: rename dash by dot in global option
William Lallemand [Wed, 4 May 2022 11:52:29 +0000 (13:52 +0200)] 
MINOR: httpclient: rename dash by dot in global option

Rename the httpclient-ssl-verify into httpclient.ssl.verify.

3 years agoMINOR: httpclient: handle unix and other socket types in dst
William Lallemand [Wed, 4 May 2022 08:59:51 +0000 (10:59 +0200)] 
MINOR: httpclient: handle unix and other socket types in dst

httpclient_set_dst() allows one to set the destination address instead
of using the one in the URL or resolving one from the host.
This function also support other types of socket like sockpair@, unix@,
anything that could be used on a server line.

In order to still support this behavior, the address must be set on the
backend in this particular case because the frontend connection does not
support anything other than ipv4 or ipv6.

3 years agoCLEANUP: httpclient: remove the comment about resolving
William Lallemand [Tue, 3 May 2022 14:52:08 +0000 (16:52 +0200)] 
CLEANUP: httpclient: remove the comment about resolving

remove the comment of httpclient_start which states there is no
resolver.

3 years agoMEDIUM: httpclient: allow address and port change for resolving
William Lallemand [Tue, 3 May 2022 12:09:06 +0000 (14:09 +0200)] 
MEDIUM: httpclient: allow address and port change for resolving

To allow the http-request set-dst to work for the httpclient DNS
resolving, some changes need to be done:

- The destination address need to be set in the frontend (s->csf->dst)
  instead of the backend (s->csb->dst) to be able to use
  tcp_action_req_set_dst()

- SRV_F_MAPPORTS need to be set on the proxy in order to allow the port
  change in alloc_dst_address()

3 years agoMEDIUM: httpclient: http-request rules for resolving
William Lallemand [Thu, 28 Apr 2022 14:55:02 +0000 (16:55 +0200)] 
MEDIUM: httpclient: http-request rules for resolving

The httpclient_resolve_init() adds http-request actions which does the
resolving using the Host header of the HTTP request.

The parse_http_req_cond function is directly used over an array of http
rules.

The do-resolve rule uses the "default" resolvers section. If this
section does not exist in the configuration, the resolving is disabling.

3 years agoMEDIUM: httpclient: remove url2sa to use a more flexible parser
William Lallemand [Thu, 14 Apr 2022 15:50:20 +0000 (17:50 +0200)] 
MEDIUM: httpclient: remove url2sa to use a more flexible parser

The httpclient DNS resolver will need a more efficient URL parser which
splits the URL into parts and does not try to resolve.

httpclient_spliturl uses the http_uri_parser in order to split the URL
into several ist.

The result of the function host part is then processed into str2ip2(),
and fails if it it not an IP, allowing us to resolve the domain later.

3 years agoBUG/MINOR: mux_quic: Dropped packet upon retransmission for closed streams
Frédéric Lécaille [Mon, 2 May 2022 16:58:27 +0000 (18:58 +0200)] 
BUG/MINOR: mux_quic: Dropped packet upon retransmission for closed streams

We rely on the largest ID which was used to open streams to know if the
stream we received STREAM frames for is closed or not. If closed, we return the
same status as the one for a STREAM frame which  was a already received one for
on open stream.

3 years agoBUG/MINOR: quic: Dropped retransmitted STREAM frames
Frédéric Lécaille [Mon, 2 May 2022 16:52:58 +0000 (18:52 +0200)] 
BUG/MINOR: quic: Dropped retransmitted STREAM frames

It is possible that we continue to receive retransmitted STREAM frames after
the mux have been released. We rely on the ->rx.streams[].nb_streams counter
to check the stream was closed. If not, at this time we drop the packet.

3 years agoMINOR: quic: Make the quic_conn be aware of the number of streams
Frédéric Lécaille [Mon, 2 May 2022 16:46:58 +0000 (18:46 +0200)] 
MINOR: quic: Make the quic_conn be aware of the number of streams

This is required when the retransmitted frame types when the mux is released.
We add a counter for the number of streams which were opened or closed by the mux.
After the mux has been released, we can rely on this counter to know if the STREAM
frames are retransmitted ones or not.

3 years agoCLEANUP: mux: Useless xprt_quic-t.h inclusion
Frédéric Lécaille [Mon, 2 May 2022 14:48:40 +0000 (16:48 +0200)] 
CLEANUP: mux: Useless xprt_quic-t.h inclusion

This inclusion is useless for mux_quic-t.h. Furthermore this fixes compilation
issues when we need to refer to mux_quic-t.h from xprt_quic-t.h.

3 years agoMINOR: session: get rid of the now unused SESS_FL_ADDR_*_SET flags
Willy Tarreau [Mon, 2 May 2022 15:51:51 +0000 (17:51 +0200)] 
MINOR: session: get rid of the now unused SESS_FL_ADDR_*_SET flags

That's similar to what was done for conn_streams and connections. The
flags were only set exactly when the relevant pointers were allocated,
so better test the pointer than the flag and stop setting the flag.

3 years agoMINOR: connection: get rid of the CO_FL_ADDR_*_SET flags
Willy Tarreau [Mon, 2 May 2022 15:47:46 +0000 (17:47 +0200)] 
MINOR: connection: get rid of the CO_FL_ADDR_*_SET flags

Just like for the conn_stream, now that these addresses are dynamically
allocated, there is no single case where the pointer is set without the
corresponding flag, and the flag is used as a permission to dereference
the pointer. Let's just replace the test of the flag with a test of the
pointer and remove all flag assignment. This makes the code clearer
(especially in "if" conditions) and saves the need for future code to
think about properly setting the flag after setting the pointer.

3 years agoCLEANUP: protocol: make sure the connect_* functions always receive a dst
Willy Tarreau [Mon, 2 May 2022 15:45:12 +0000 (17:45 +0200)] 
CLEANUP: protocol: make sure the connect_* functions always receive a dst

Some of the protocol-level ->connect() functions currently dereference
the connection's destination address while others test it and return an
error. There's normally no more non-bogus code path that calls such
functions without a valid destination address on the connection, so
let's unify these functions and just place a BUG_ON() there, and drop
the useless test that's supposed to return an internal error.

3 years agoMINOR: conn_stream: remove the now unused CS_FL_ADDR_*_SET flags
Willy Tarreau [Mon, 2 May 2022 15:27:34 +0000 (17:27 +0200)] 
MINOR: conn_stream: remove the now unused CS_FL_ADDR_*_SET flags

These flags indicate that the ->src or ->dst field in the conn_stream
is not null, which is something the caller already sees (and even tests
from the two sets of functions that set them). They maintain some burden
because an agent trying to set a source or destination has to manually
set the flags in addition to setting the pointer, so they provide no
value anymore, let's drop them.

3 years agoMEDIUM: stream: remove the confusing SF_ADDR_SET flag
Willy Tarreau [Mon, 2 May 2022 14:36:47 +0000 (16:36 +0200)] 
MEDIUM: stream: remove the confusing SF_ADDR_SET flag

This flag is no longer needed now that it must always match the presence
of a destination address on the backend conn_stream. Worse, before previous
patch, if it were to be accidently removed while the address is present, it
could result in a leak of that address since alloc_dst_address() would first
be called to flush it.

Its usage has a long history where addresses were stored in an area shared
with the connection, but as this is no longer the case, there's no reason
for putting this burden onto application-level code that should not focus
on setting obscure flags.

The only place where that made a small difference is in the dequeuing code
in case of queue redistribution, because previously the code would first
clear the flag, and only later when trying to deal with the queue, would
release the address. It's not even certain whether there would exist a
code path going to connect_server() without calling pendconn_dequeue()
first (e.g. retries on queue timeout maybe?).

Now the pendconn_dequeue() code will rely on SF_ASSIGNED to decide to
clear and release the address, since that flag is always set while in
a server's queue, and its clearance implies that we don't want to keep
the address. At least it remains consistent and there's no more risk of
leaking it.

3 years agoCLEANUP: backend: make alloc_{bind,dst}_address() idempotent
Willy Tarreau [Mon, 2 May 2022 14:20:36 +0000 (16:20 +0200)] 
CLEANUP: backend: make alloc_{bind,dst}_address() idempotent

These functions dynamically allocate a source or destination address but
start by clearing the previous one. There's a non-null risk of leaking
addresses there in case of misuse. Better have them do nothing if the
address was already allocated.

3 years agoBUG/MINOR: h3: fix parsing of unknown frame type with null length
Amaury Denoyelle [Mon, 2 May 2022 08:35:39 +0000 (10:35 +0200)] 
BUG/MINOR: h3: fix parsing of unknown frame type with null length

HTTP/3 implementation must ignore unknown frame type to support protocol
evolution. Clients can deliberately use unknown type to test that the
server is conformant : this principle is called greasing.

Quiche client uses greasing on H3 frame type with a zero length frame.
This reveals a bug in H3 parsing code which causes the transfer to be
interrupted. Fix this by removing the break statement on ret variable.
Now the parsing loop is only interrupted if input buffer is empty or the
demux is blocked.

This should fix http/3 freeze transfers with the quiche client. Thanks
to Lucas Pardue from Cloudflare for his report on the bug. Frédéric
Lecaille quickly found the source of the problem which helps me to write
this patch.

3 years agoMINOR: mux-quic: support full request channel buffer
Amaury Denoyelle [Mon, 2 May 2022 09:07:06 +0000 (11:07 +0200)] 
MINOR: mux-quic: support full request channel buffer

If the request channel buffer is full, H3 demuxing must be interrupted
on the stream until some read is performed. This condition is reported
if the HTX stream buffer qcs.rx.app_buf is full.

In this case, qcs instance is marked with a new flag QC_SF_DEM_FULL.
This flag cause the H3 demuxing to be interrupted. It is cleared when
the HTX buffer is read by the conn-stream layer through rcv_buf
operation.

When the flag is cleared, the MUX tasklet is woken up. However, as MUX
iocb does not treat Rx for the moment, this is useless. It must be fix
to prevent possible freeze on POST transfers.

In practice, for the moment the HTX buffer is never full as the current
Rx code is limited by the quic-conn receive buffer size and the
incomplete flow-control implementation. So for now this patch is not
testable under the current conditions.

3 years ago[RELEASE] Released version 2.6-dev8 v2.6-dev8
Willy Tarreau [Sat, 30 Apr 2022 12:17:51 +0000 (14:17 +0200)] 
[RELEASE] Released version 2.6-dev8

Released version 2.6-dev8 with the following main changes :
    - BUG/MINOR: quic: fix use-after-free with trace on ACK consume
    - BUG/MINOR: rules: Forbid captures in defaults section if used by a backend
    - BUG/MEDIUM: rules: Be able to use captures defined in defaults section
    - BUG/MINOR: rules: Fix check_capture() function to use the right rule arguments
    - BUG/MINOR: http-act: make release_http_redir() more robust
    - BUG/MINOR: sample: add missing use_backend/use-server contexts in smp_resolve_args
    - MINOR: sample: don't needlessly call c_none() in sample_fetch_as_type()
    - MINOR: sample: make the bool type cast to bin
    - MEDIUM: backend: add new "balance hash <expr>" algorithm
    - MINOR: init: add global setting "fd-hard-limit" to bound system limits
    - BUILD: pollers: use an initcall to register the pollers
    - BUILD: xprt: use an initcall to register the transport layers
    - BUILD: thread: use initcall instead of a constructor
    - BUILD: http: remove the two unused constructors in rules and ana
    - CLEANUP: compression: move the default setting of maxzlibmem to defaults
    - MINOR: tree-wide: always consider EWOULDBLOCK in addition to EAGAIN
    - BUG/MINOR: connection: "connection:close" header added despite 'close-spread-time'
    - MINOR: fd: add functions to set O_NONBLOCK and FD_CLOEXEC
    - CLEANUP: tree-wide: use fd_set_nonblock() and fd_set_cloexec()
    - CLEANUP: tree-wide: remove 25 occurrences of unneeded fcntl.h
    - REGTESTS: fix the race conditions in be2dec.vtc ad field.vtc
    - REGTESTS: webstats: remove unused stats socket in /tmp
    - MEDIUM: httpclient: disable SSL when the ca-file couldn't be loaded
    - BUG/MINOR: httpclient/lua: error when the httpclient_start() fails
    - BUG/MINOR: ssl: free the cafile entries on deinit
    - BUG/MINOR: ssl: memory leak when trying to load a directory with ca-file
    - MEDIUM: httpclient: re-enable the verify by default
    - BUG/MEDIUM: ssl/cli: fix yielding in show_cafile_detail
    - BUILD: compiler: properly distinguish weak and global symbols
    - MINOR: connection: Add way to disable active connection closing during soft-stop
    - BUG/MEDIUM: http-ana: Fix memleak in redirect rules with ignore-empty option
    - CLEANUP: Destroy `http_err_chunks` members during deinit
    - BUG/MINOR: resolvers: Fix memory leak in resolvers_deinit()
    - MINOR: Call deinit_and_exit(0) for `haproxy -vv`
    - BUILD: fd: disguise the fd_set_nonblock/cloexec result
    - BUG/MINOR: pools: make sure to also destroy shared pools in pool_destroy_all()
    - MINOR: ssl: add a new global option "tune.ssl.hard-maxrecord"
    - CLEANUP: errors: also call deinit_errors_buffers() on deinit()
    - CLEANUP: chunks: release trash also in deinit
    - CLEANUP: deinit: release the pre-check callbacks
    - CLEANUP: deinit: release the config postparsers
    - CLEANUP: listeners/deinit: release accept queue tasklets on deinit
    - CLEANUP: connections/deinit: destroy the idle_conns tasks
    - BUG/MINOR: mux-quic: fix build in release mode
    - MINOR: mux-quic: adjust comment on emission function
    - MINOR: mux-quic: remove unused bogus qcc_get_stream()
    - BUG/MINOR: mux-quic: fix leak if cs alloc failure
    - MINOR: mux-quic: count local flow-control stream limit on reception
    - BUG/MINOR: h3: fix incomplete POST requests
    - BUG/MEDIUM: h3: fix use-after-free on mux Rx buffer wrapping
    - MINOR: mux-quic: partially copy Rx frame if almost full buf
    - MINOR: h3: change frame demuxing API
    - MINOR: mux-quic: add a app-layer context in qcs
    - MINOR: h3: implement h3 stream context
    - MINOR: h3: support DATA demux if buffer full
    - MINOR: quic: decode as much STREAM as possible
    - MINOR: quic: Improve qc_prep_pkts() flexibility
    - MINOR: quic: Prepare quic_frame struct duplication
    - MINOR: quic: Do not retransmit frames from coalesced packets
    - MINOR: quic: Add traces about TX frame memory releasing
    - MINOR: quic: process_timer() rework
    - MEDIUM: quic: New functions for probing rework
    - MEDIUM: quic: Retransmission functions rework
    - MEDIUM: quic: qc_requeue_nacked_pkt_tx_frms() rework
    - MINOR: quic: old data distinction for qc_send_app_pkt()
    - MINOR: quic: Mark packets as probing with old data
    - MEDIUM: quic: Mark copies of acknowledged frames as acknowledged
    - MEDIUM: quic: Enable the new datagram probing process
    - MINOR: quic: Do not send ACK frames when probing
    - BUG/MINOR: quic: Wrong returned status by qc_build_frms()
    - BUG/MINOR: quic: Avoid sending useless PADDING frame
    - BUG/MINOR: quic: Traces fix about remaining frames upon packet build failure
    - MINOR: quic: Wake up the mux to probe with new data
    - BUG/MEDIUM: quic: Possible crash on STREAM frame loss
    - BUG/MINOR: quic: Missing Initial packet length check
    - CLEANUP: quic: Rely on the packet length set by qc_lstnr_pkt_rcv()
    - MINOR: quic: Drop 0-RTT packets if not allowed
    - BUG/MINOR: httpclient/ssl: use the correct verify constant
    - BUG/MEDIUM: conn-stream: Don't erase endpoint flags on reset
    - BUG/MEDIUM: httpclient: Fix loop consuming HTX blocks from the response channel
    - BUG/MINOR: httpclient: Count metadata in size to transfer via htx_xfer_blks()
    - MINOR: httpclient: Don't use co_set_data() to decrement output
    - BUG/MINOR: conn_stream: do not confirm a connection from the frontend path
    - MEDIUM: quic: do not ACK packet with STREAM if MUX not present
    - MEDIUM: quic: do not ack packet with invalid STREAM
    - MINOR: quic: Drop 0-RTT packets without secrets
    - CLEANUP: quic: Remaining fprintf() debug trace
    - MINOR: quic: moving code for QUIC loss detection
    - BUG/MINOR: quic: Missing time threshold multiplifier for loss delay computation
    - CI: github actions: update LibreSSL to 3.5.2
    - SCRIPTS: announce-release: add URL of dev packages

3 years agoSCRIPTS: announce-release: add URL of dev packages
Willy Tarreau [Sat, 30 Apr 2022 12:16:15 +0000 (14:16 +0200)] 
SCRIPTS: announce-release: add URL of dev packages

This is the shortened URL of the nightly builds maintained by William,
let's have them in announce messages.

3 years agoCI: github actions: update LibreSSL to 3.5.2
Ilya Shipitsin [Thu, 28 Apr 2022 06:46:53 +0000 (11:46 +0500)] 
CI: github actions: update LibreSSL to 3.5.2

LibreSSL-3.5.2 was released on Apr 23nd 2022, let us switch to it

3 years agoBUG/MINOR: quic: Missing time threshold multiplifier for loss delay computation
Frédéric Lécaille [Fri, 29 Apr 2022 14:00:17 +0000 (16:00 +0200)] 
BUG/MINOR: quic: Missing time threshold multiplifier for loss delay computation

It seems this multiplier ended up in oblivion. Indeed a multiplier must be
applied to the loss delay expressed as an RTT multiplier: 9/8.

So, some packets were detected as lost too soon, leading to be retransmitted too
early!

3 years agoMINOR: quic: moving code for QUIC loss detection
Frédéric Lécaille [Fri, 29 Apr 2022 13:07:48 +0000 (15:07 +0200)] 
MINOR: quic: moving code for QUIC loss detection

qc_qc_packet_loss_lookup() is definitively a QUIC loss detection function.

3 years agoCLEANUP: quic: Remaining fprintf() debug trace
Frédéric Lécaille [Thu, 28 Apr 2022 20:21:56 +0000 (22:21 +0200)] 
CLEANUP: quic: Remaining fprintf() debug trace

Development remaining trace.

3 years agoMINOR: quic: Drop 0-RTT packets without secrets
Frédéric Lécaille [Thu, 28 Apr 2022 13:43:46 +0000 (15:43 +0200)] 
MINOR: quic: Drop 0-RTT packets without secrets

If we received 0-RTT packets and no secrets were provided by the TLS stack
we must drop them.

3 years agoMEDIUM: quic: do not ack packet with invalid STREAM
Amaury Denoyelle [Fri, 29 Apr 2022 13:58:22 +0000 (15:58 +0200)] 
MEDIUM: quic: do not ack packet with invalid STREAM

If the MUX cannot handle immediately nor buffer a STREAM frame, the
packet containing it must not be acknowledge. This is in conformance
with the RFC9000.

qcc_recv() return codes have been adjusted to differentiate an invalid
frame with an already fully received offset which must be acknowledged.

3 years agoMEDIUM: quic: do not ACK packet with STREAM if MUX not present
Amaury Denoyelle [Fri, 29 Apr 2022 13:57:49 +0000 (15:57 +0200)] 
MEDIUM: quic: do not ACK packet with STREAM if MUX not present

If a packet contains a STREAM frame but the MUX is not allocated, the
frame cannot be enqueued. According to the RFC9000, we must not
acknowledge the packet under this condition.

This may prevents a bug with firefox which keeps trying on refreshing
the web page. This issue has already been detected before closing state
implementation : haproxy wasn't emitted CONNECTION_CLOSE and keeps
acknowledge STREAM frames despite not handle them.

In the future, it might be necessary to respond with a CONNECTION_CLOSE
if the MUX has already been freed.

3 years agoBUG/MINOR: conn_stream: do not confirm a connection from the frontend path
Willy Tarreau [Fri, 29 Apr 2022 13:04:41 +0000 (15:04 +0200)] 
BUG/MINOR: conn_stream: do not confirm a connection from the frontend path

In issue #1468 it was reported that sometimes server-side connection
attempts were only validated after the "timeout connect" value, and
that would only happen with an H2 client. A long code analysis with the
output dumps showed only one possible call path: an I/O event on the
frontend while reading had just been disabled calls h2_wake() which in
turns wakes cs_conn_io_cb(), which tries cs_conn_process() and cs_notify(),
which sees that the other side is not blocked (already in CS_ST_CON)
and tries cs_chk_snd() on it. But on that side the connection had just
finished to be set up and not yet woken the stream up, cs_notify()
would then call cs_conn_send() which succeeds and passes the connection
to CS_ST_RDY. The problem is that nothing new happened on the frontend
side so there's no reason to wake the stream up and the backend-side
conn_stream remains in CS_ST_RDY state with the stream never being
woken up.

Once the "timeout connect" strikes, process_stream() is woken up and
finds the connection finally setup, so it ignores the timeout and goes
on.

The number of conditions to meet to reproduce this is huge, which also
explains why the reporter says it's "occasional" and we were never able
to reproduce it in the lab. It needs at least reads to be disabled and
immediately re-enabled on the frontend side (e.g. buffer full) with an
I/O even reported before the poller had an opportunity to be disabled
but with no subscribe being reinstalled, so that sock_conn_iocb() has
no other choice but calling h2_wake(), and exactly at the same time
the backend connection must finish to set up so that it was not yet
reported by the poller, the data were sent and the polling for writes
disabled.

Several factors are to be considered here:
  - h2_wake() should probably not call h2_wake_some_streams() for
    ret >= 0 (common case), but only if some special event is reported
    for at least one stream; that part is sensitive though as in the
    past we managed to lose some rare cases (e.g. restart processing
    after a pause), and such wakeups are extremely rare so we'd better
    make that effort once in a while.

  - letting a lazy forward attempt on the frontend confirm a backend
    connection establishment is too smart to be reliable. That wasn't
    in fact the intent and it's inherited from the very old code where
    muxes didn't exist and where it was guaranteed that an even at this
    layer would wake everyone up.

Here the best thing to do is to refrain from attempting to forward data
until the connection is confirmed. This will let the poller report the
connect() event to the backend side which will process it as it should
and does in all other cases.

Thanks to Jimmy Crutchfield for having reported useful traces and
tested patches.

This will have to be backported to all stable branches after some
observation. Before 2.6 the function is stream_int_chk_snd_conn(),
and the flag to remove is SI_SB_CON.

3 years agoMINOR: httpclient: Don't use co_set_data() to decrement output
Christopher Faulet [Fri, 29 Apr 2022 12:09:03 +0000 (14:09 +0200)] 
MINOR: httpclient: Don't use co_set_data() to decrement output

The use of co_set_data() should be strictly limited to setting the amount of
existing data to be transmitted. It ought not be used to decrement the
output after the data have left the buffer, because doing so involves
performing incorrect calculations using co_data() that still comprises data
that are not in the buffer anymore. Let's use c_rew() for this, which is
made exactly for this purpose, i.e. decrement c->output by as much as
requested.

3 years agoBUG/MINOR: httpclient: Count metadata in size to transfer via htx_xfer_blks()
Christopher Faulet [Fri, 29 Apr 2022 11:56:12 +0000 (13:56 +0200)] 
BUG/MINOR: httpclient: Count metadata in size to transfer via htx_xfer_blks()

When HTX blocks are transfer from the HTTP client context to the request
channel, via htx_xfer_blks() function, the metadata must also be counted, in
addition to the data size. Otherwise, expected payload size will not be
copied because the metadata of an HTX block (8 bytes) will be reserved. And
if the payload size is lower than 8 bytes, nothing will be copied. Thus only
a zero-copy will be able to copy the payload.

This issue is 2.6-specific, no backport is needed.

3 years agoBUG/MEDIUM: httpclient: Fix loop consuming HTX blocks from the response channel
Christopher Faulet [Fri, 29 Apr 2022 11:44:46 +0000 (13:44 +0200)] 
BUG/MEDIUM: httpclient: Fix loop consuming HTX blocks from the response channel

When the HTTP client consumes the response, it loops on the HTX message to
copy blocks content and it removes blocks by calling htx_remove_blk(). But
this function removes a block and returns the next one in the HTX
message. The result must be used instead of using htx_get_next(). It is
especially important because the block used in htx_get_next() loop was
removed. It only works because the message is not defragmented during the
loop.

In addition, the loop on the response was simplified to iter on blocks
instead of positions.

This patch must be backported to 2.5.

3 years agoBUG/MEDIUM: conn-stream: Don't erase endpoint flags on reset
Christopher Faulet [Thu, 28 Apr 2022 16:25:24 +0000 (18:25 +0200)] 
BUG/MEDIUM: conn-stream: Don't erase endpoint flags on reset

Only CS_EP_ERROR flag is now removed from the endpoint when a reset is
performed. When a new the endpoint is allocated, flags are preserved. It is
the caller responsibility to remove other flags, depending on its need.

Concretly, during a connection retry or a L7 retry, we must preserve
flags. In tcpcheck and the CLI, we reset flags.

This patch is 2.6-specific. No backport needed.

3 years agoBUG/MINOR: httpclient/ssl: use the correct verify constant
William Lallemand [Thu, 28 Apr 2022 17:35:21 +0000 (19:35 +0200)] 
BUG/MINOR: httpclient/ssl: use the correct verify constant

The SSL_SERVER_VERIFY_* constants were incorrectly set on the httpclient
server verify. The right constants are SSL_SOCK_VERIFY_* .

This could cause issues when using "httpclient-ssl-verify" or when the
SSL certificates can't be loaded.

No backport needed

3 years agoMINOR: quic: Drop 0-RTT packets if not allowed
Frédéric Lécaille [Wed, 27 Apr 2022 13:37:28 +0000 (15:37 +0200)] 
MINOR: quic: Drop 0-RTT packets if not allowed

Drop the 0-RTT packets for a listener without early data configuration enabled.

3 years agoCLEANUP: quic: Rely on the packet length set by qc_lstnr_pkt_rcv()
Frédéric Lécaille [Wed, 27 Apr 2022 13:09:53 +0000 (15:09 +0200)] 
CLEANUP: quic: Rely on the packet length set by qc_lstnr_pkt_rcv()

This function is used to parse the QUIC packets carried by a UDP datagram.
When a correct packet could be found, the ->len RX packet structure value
is set to the packet length value. On the contrary, it is set to the remaining
number of bytes in the UDP datagram if no correct QUIC packet could be found.
So, there is no need to make this function return a status value. It allows
the caller to parse any QUIC packet carried by a UDP datagram without this.

3 years agoBUG/MINOR: quic: Missing Initial packet length check
Frédéric Lécaille [Wed, 27 Apr 2022 09:42:08 +0000 (11:42 +0200)] 
BUG/MINOR: quic: Missing Initial packet length check

Any client Initial packet carried in a datagram smaller than QUIC_INITIAL_PACKET_MINLEN(200)
bytes must be discarded. This does not mean we must discard the entire datagram.
So we must at least try to parse the packet length before dropping the packet
and return its length from qc_lstnr_pkt_rcv().

3 years agoBUG/MEDIUM: quic: Possible crash on STREAM frame loss
Frédéric Lécaille [Wed, 27 Apr 2022 05:17:56 +0000 (07:17 +0200)] 
BUG/MEDIUM: quic: Possible crash on STREAM frame loss

A crash is possible under such circumtances:
    - The congestion window is drastically reduced to its miniaml value
    when a quic listener is experiencing extreme packet loss ;
    - we enqueue several STREAM frames to be resent and some of them could not be
    transmitted ;
    - some of the still in flight are acknowledged and trigger the
    stream memory releasing ;
    - when we come back to send the remaing STREAM frames, haproxy
    crashes when it tries to build them.

To fix this issue, we mark the STREAM frame as lost when detected as lost.
Then a lookup if performed for the stream the STREAM frames are attached
to before building them. They are released if the stream is no more available
or the data range of the frame is consumed.

3 years agoMINOR: quic: Wake up the mux to probe with new data
Frédéric Lécaille [Tue, 26 Apr 2022 11:54:28 +0000 (13:54 +0200)] 
MINOR: quic: Wake up the mux to probe with new data

When we have to probe the peer, we must first try to send new data. This is done
here waking up the mux after having set the number of maximum number of datagrams
to send to QUIC_MAX_NB_PTO_DGRAMS (2). Of course, this is only the case if the
mux was subscribed to SEND events.

3 years agoBUG/MINOR: quic: Traces fix about remaining frames upon packet build failure
Frédéric Lécaille [Mon, 25 Apr 2022 15:48:40 +0000 (17:48 +0200)] 
BUG/MINOR: quic: Traces fix about remaining frames upon packet build failure

3 years agoBUG/MINOR: quic: Avoid sending useless PADDING frame
Frédéric Lécaille [Mon, 25 Apr 2022 15:17:07 +0000 (17:17 +0200)] 
BUG/MINOR: quic: Avoid sending useless PADDING frame

This may happen in rare cases with extreme packet loss (30% for both TX and RX)
which leads the congestion window to decrease down to its minimal value (two
datagrams). Under such circumtances, no ack-eliciting frame can be added to
a packet by qc_build_frms(). In this case we must cancel the packet building
process if there is no ACK or probe (PING frame) to send.

3 years agoBUG/MINOR: quic: Wrong returned status by qc_build_frms()
Frédéric Lécaille [Mon, 25 Apr 2022 13:42:56 +0000 (15:42 +0200)] 
BUG/MINOR: quic: Wrong returned status by qc_build_frms()

This function must return a successful status as soon as it could be build
a frame to be embedded by a packet. This behavior was broken by the last
modifications. This was due to a dangerous "ret = 1" statement inside
a loop. This statement must be reach only if we go out of a switch/case
after a "break" statement.
Add comments to mention this information.

3 years agoMINOR: quic: Do not send ACK frames when probing
Frédéric Lécaille [Mon, 25 Apr 2022 08:38:27 +0000 (10:38 +0200)] 
MINOR: quic: Do not send ACK frames when probing

When we are probing, we do not receive packets, furthermore all ACK frames have
already been sent. This is useless to send ACK when probing the peer. This
modification does not reset the flag which marks the connection as requiring an
ACK frame to be sent. If this is the case, this will be taken into an account
by after the probing process.

3 years agoMEDIUM: quic: Enable the new datagram probing process
Frédéric Lécaille [Mon, 25 Apr 2022 08:33:12 +0000 (10:33 +0200)] 
MEDIUM: quic: Enable the new datagram probing process

Make the two I/O handlers quic_conn_io_cb() and quic_conn_app_io_cb() call
qc_dgrams_retransmit() after probing retransmissions need was detected by
the timer task (qc_process_timer()).
We must modify qc_prep_pkts() to support QUIC_TLS_ENC_LEVEL_NONE as <next_tel>
parameter when called from qc_dgrams_retransmit().

3 years agoMEDIUM: quic: Mark copies of acknowledged frames as acknowledged
Frédéric Lécaille [Mon, 25 Apr 2022 08:28:49 +0000 (10:28 +0200)] 
MEDIUM: quic: Mark copies of acknowledged frames as acknowledged

We call qc_release_frm() to do so from this function everywhere a frame
is released.

3 years agoMINOR: quic: Mark packets as probing with old data
Frédéric Lécaille [Mon, 25 Apr 2022 08:24:12 +0000 (10:24 +0200)] 
MINOR: quic: Mark packets as probing with old data

When probing retranmissions with old data are needed for the connection we
mark the packets as probing with old data to track them when acknowledged:
we do not resend frames with old data when lost.

3 years agoMINOR: quic: old data distinction for qc_send_app_pkt()
Frédéric Lécaille [Mon, 25 Apr 2022 08:17:00 +0000 (10:17 +0200)] 
MINOR: quic: old data distinction for qc_send_app_pkt()

Modify qc_send_app_pkt() to distinguish the case where it sends new data
against the case where it sends old data during probing retransmissions.
We add <old_data> boolean parameter to this function to do so. The mux
never directly send old data when probing retransmissions are needed by
the connection.

3 years agoMEDIUM: quic: qc_requeue_nacked_pkt_tx_frms() rework
Frédéric Lécaille [Mon, 25 Apr 2022 07:40:19 +0000 (09:40 +0200)] 
MEDIUM: quic: qc_requeue_nacked_pkt_tx_frms() rework

This function is used to requeue the TX frames from TX packets which have
been detected as lost. The modifications consist in avoiding resending frames from
duplicated frames used to probe the peer. This is useless. Only the original
frames loss must be taken into an account because detected as lost before
the retransmitted frames. If these latter are also detected as lost, other
duplicated frames would have been retransmitted before their loss detection.

3 years agoMEDIUM: quic: Retransmission functions rework
Frédéric Lécaille [Mon, 25 Apr 2022 07:25:56 +0000 (09:25 +0200)] 
MEDIUM: quic: Retransmission functions rework

qc_prep_fast_retrans() and qc_prep_hdshk_fast_retrans() are modified to
take two list of frames as parameters. Two lists are needed for
qc_prep_hdshk_fast_retrans() to build datagrams with two packets during
handshake. qc_prep_fast_retrans() needs two lists of frames to be used
to send two datagrams with one list by datagram.

3 years agoMEDIUM: quic: New functions for probing rework
Frédéric Lécaille [Mon, 25 Apr 2022 06:58:04 +0000 (08:58 +0200)] 
MEDIUM: quic: New functions for probing rework

We want to be able to resend frames from list of frames during handshakes to
resend datagrams with the same frames as during the first transmissions.
This leads to decrease drasctically the chances of frame fragmentation due to
variable lengths of the packet fields. Furthermore the frames were not duplicated
when retransmitted from a packet to another one. This must be the case only during
packet loss dectection.

qc_dup_pkt_frms() is there to duplicate the frames from an input list to an output
list. A distinction is made between STREAM frames and the other ones because we
can rely on the "acknowledged offset" the aim of which is to track the number
of bytes which were acknowledged from TX STREAM frames.

qc_release_frm() in addition to release the frame passed as parameter, also mark
the duplicate STREAM frames as acknowledeged.

qc_send_hdshk_pkts() is the qc_send_app_pkts() counterpart to send datagrams from
at most two list of frames to be able to coalesced packets from two different
packet number spaces

qc_dgrams_retransmit() is there to probe the peer with datagrams depending on the
need of the packet number spaces which must be flag with QUIC_FL_PKTNS_PROBE_NEEDED
by the PTO timer task (qc_process_timer()).

3 years agoMINOR: quic: process_timer() rework
Frédéric Lécaille [Thu, 21 Apr 2022 16:26:22 +0000 (18:26 +0200)] 
MINOR: quic: process_timer() rework

Add QUIC_FL_CONN_RETRANS_NEEDED connection flag definition to mark a quic_conn
struct as needing a retranmission.
Add QUIC_FL_PKTNS_PROBE_NEEDED to mark a packet number space as needing a
datagram probing.
Set these flags from process_timer() to trigger datagram probings.
Do not initiate anymore datagrams probing from any quic encryption level.
This will be done from the I/O handlers (quic_conn_io_cb() during handshakes and
quic_conn_app_io_cb() after handshakes).

3 years agoMINOR: quic: Add traces about TX frame memory releasing
Frédéric Lécaille [Thu, 21 Apr 2022 16:10:41 +0000 (18:10 +0200)] 
MINOR: quic: Add traces about TX frame memory releasing

Add such traces in qc_treat_acked_tx_frm(). This should be helpful to track memory
leak issues for TX frames.

3 years agoMINOR: quic: Do not retransmit frames from coalesced packets
Frédéric Lécaille [Thu, 21 Apr 2022 15:58:46 +0000 (17:58 +0200)] 
MINOR: quic: Do not retransmit frames from coalesced packets

Add QUIC_FL_TX_PACKET_COALESCED flag to mark a TX packet as coalesced with others
to build a datagram.
Ensure we do not directly retransmit frames from such coalesced packets. They must
be retransmitted from their packet number spaces to avoid duplications.

3 years agoMINOR: quic: Prepare quic_frame struct duplication
Frédéric Lécaille [Thu, 21 Apr 2022 15:32:10 +0000 (17:32 +0200)] 
MINOR: quic: Prepare quic_frame struct duplication

We want to track the frames which have been duplicated during retransmissions so
that to avoid uselessly retransmitting frames which would already have been
acknowledged. ->origin new member is there to store the frame from which a copy
was done, ->reflist is a list to store the frames which are copies.
Also ensure all the frames are zeroed and that their ->reflist list member is
initialized.
Add QUIC_FL_TX_FRAME_ACKED flag definition to mark a TX frame as acknowledged.

3 years agoMINOR: quic: Improve qc_prep_pkts() flexibility
Frédéric Lécaille [Mon, 11 Apr 2022 13:39:34 +0000 (15:39 +0200)] 
MINOR: quic: Improve qc_prep_pkts() flexibility

We want to be able to chosse the list of frames we want to prepare in packets
to be send. This is to modify the retransmission process (to come).

3 years agoMINOR: quic: decode as much STREAM as possible
Amaury Denoyelle [Wed, 27 Apr 2022 14:53:16 +0000 (16:53 +0200)] 
MINOR: quic: decode as much STREAM as possible

Add a loop in the bidi STREAM function. This will call repeatdly
qcc_decode_qcs() and dequeue buffered frames.

This is useful when reception of more data is interrupted because the
MUX buffer was full. qcc_decode_qcs() has probably free some space so it
is useful to immediatly retry reception of buffered frames of the qcs
tree.

This may fix occurences of stalled Rx transfers with large payload.
Note however that there is still room for improvment. The conn-stream
layer is not able at this moment to retrigger demuxing. This is because
the mux io-handler does not treat Rx : this may continue to cause
stalled tranfers.

3 years agoMINOR: h3: support DATA demux if buffer full
Amaury Denoyelle [Wed, 27 Apr 2022 13:37:20 +0000 (15:37 +0200)] 
MINOR: h3: support DATA demux if buffer full

Previously, h3 layer was not able to demux a DATA frame if not fully
received in the Rx buffer. This causes evident limitation and prevents
to be able to demux a frame bigger than the buffer.

Improve h3_data_to_htx() to support partial frame demuxing. The demux
state is preserved in the h3s new fields : this is useful to keep the
current type and length of the demuxed frame.

3 years agoMINOR: h3: implement h3 stream context
Amaury Denoyelle [Wed, 27 Apr 2022 16:04:01 +0000 (18:04 +0200)] 
MINOR: h3: implement h3 stream context

Define a new structure h3s used to provide context for a H3 stream. This
structure is allocated and stored in the qcs thanks to previous commit
which provides app-layer context storage.

For now, h3s is empty. It will soon be completed to be able to support
stateful demux : this is required to be able to demux an incomplete
frame if the rx buffer is full.

3 years agoMINOR: mux-quic: add a app-layer context in qcs
Amaury Denoyelle [Wed, 27 Apr 2022 13:17:11 +0000 (15:17 +0200)] 
MINOR: mux-quic: add a app-layer context in qcs

Define 2 new callback for qcc_app_ops : attach and detach. They are
called when a qcs instance is respectively allocated and freed. If
implemented, they can allocate a custom context stored in the new
abstract field ctx of qcs.

For now, h3 and hq-interop does not use these new callbacks. They will
be soon implemented by the h3 layer to allocate a context used for
stateful demuxing.

This change is required to support the demuxing of H3 frames bigger than
a buffer.

3 years agoMINOR: h3: change frame demuxing API
Amaury Denoyelle [Wed, 27 Apr 2022 12:52:52 +0000 (14:52 +0200)] 
MINOR: h3: change frame demuxing API

Edit the functions used for HEADERS and DATA parsing. They now return
the number of bytes handled.

This change will help to demux H3 frames bigger than the buffer.

3 years agoMINOR: mux-quic: partially copy Rx frame if almost full buf
Amaury Denoyelle [Tue, 26 Apr 2022 09:36:40 +0000 (11:36 +0200)] 
MINOR: mux-quic: partially copy Rx frame if almost full buf

Improve the reception for STREAM frames. In qcc_recv(), if the frame is
bigger than the remaining space in rx buffer, do not reject it wholly.
Instead, copy as much data as possible. The rest of the data is
buffered.

This is necessary to handle H3 frames bigger than a buffer. The H3 code
does not demux until the frame is complete or the buffer is full.
Without this, the transfer on payload larger than the Rx buffer can
rapidly freeze.

3 years agoBUG/MEDIUM: h3: fix use-after-free on mux Rx buffer wrapping
Amaury Denoyelle [Wed, 27 Apr 2022 12:41:53 +0000 (14:41 +0200)] 
BUG/MEDIUM: h3: fix use-after-free on mux Rx buffer wrapping

Handle wrapping buffer in h3_data_to_htx(). If data is wrapping, first
copy the contiguous data, then copy the data in front of the buffer.

Note that h3_headers_to_htx() is not able to handle wrapping data. For
the moment, a BUG_ON was added as a reminder. This cas never happened,
most probably because HEADERS is the first frame of the stream.

3 years agoBUG/MINOR: h3: fix incomplete POST requests
Amaury Denoyelle [Tue, 26 Apr 2022 14:24:39 +0000 (16:24 +0200)] 
BUG/MINOR: h3: fix incomplete POST requests

Always set HTX flag HTX_SL_F_XFER_LEN for http/3. This is correct
becuase the size of H3 requests is always known thanks to the protocol
framing.

This may fix occurences of incomplete POST requests when the client side
of the connection has been closed before.

3 years agoMINOR: mux-quic: count local flow-control stream limit on reception
Amaury Denoyelle [Tue, 26 Apr 2022 09:21:10 +0000 (11:21 +0200)] 
MINOR: mux-quic: count local flow-control stream limit on reception

Add new qcs fields to count the sum of bytes received for each stream.
This is necessary to enforce flow-control for reception on the peer.

For the moment, the implementation is partial. No MAX_STREAM_DATA or
FLOW_CONTROL_ERROR are emitted. BUG_ON statements are here as a
remainder.

This means that for the moment we do not support POST payloads greater
that the initial max-stream-data announced (256k currently).

At least, we now ensure that we never buffer a frame which overflows the
flow-control limit : this ensures that the memory consumption per stream
should stay under control.

3 years agoBUG/MINOR: mux-quic: fix leak if cs alloc failure
Amaury Denoyelle [Wed, 27 Apr 2022 13:09:27 +0000 (15:09 +0200)] 
BUG/MINOR: mux-quic: fix leak if cs alloc failure

qcs and its field are not properly freed if the conn-stream allocation
fail in qcs_new(). Fix this by having a proper deinit code with a
dedicated label.

3 years agoMINOR: mux-quic: remove unused bogus qcc_get_stream()
Amaury Denoyelle [Tue, 26 Apr 2022 15:36:56 +0000 (17:36 +0200)] 
MINOR: mux-quic: remove unused bogus qcc_get_stream()

qcc_get_stream() was used when qcs and qc_stream_desc shared the same
node-tree. This is not the case anymore since
  e4301da5ed9e77844c653ea4caf233546febe591
  MINOR: quic-stream: use distinct tree nodes for quic stream and qcs

Now this function is broken as the qcc tree only contains qcs.
Thankfully it is unused so it can be removed without impact.

3 years agoMINOR: mux-quic: adjust comment on emission function
Amaury Denoyelle [Wed, 27 Apr 2022 14:44:49 +0000 (16:44 +0200)] 
MINOR: mux-quic: adjust comment on emission function

Comments were not properly edited since the splitting of functions for
stream emission. Also "payload" argument has been renamed to "in" as it
better reflects the function purpose.

3 years agoBUG/MINOR: mux-quic: fix build in release mode
Amaury Denoyelle [Thu, 28 Apr 2022 12:41:50 +0000 (14:41 +0200)] 
BUG/MINOR: mux-quic: fix build in release mode

Fix build when not using DEBUG_STRICT. 'ret' is reported as unused as it
is only tested in a BUG_ON statement.

3 years agoCLEANUP: connections/deinit: destroy the idle_conns tasks
Willy Tarreau [Wed, 27 Apr 2022 16:53:07 +0000 (18:53 +0200)] 
CLEANUP: connections/deinit: destroy the idle_conns tasks

This adds a deinit_idle_conns() function that's called on deinit to
release the per-thread idle connection management tasks. The global
task was already taken care of.

3 years agoCLEANUP: listeners/deinit: release accept queue tasklets on deinit
Willy Tarreau [Wed, 27 Apr 2022 16:42:47 +0000 (18:42 +0200)] 
CLEANUP: listeners/deinit: release accept queue tasklets on deinit

There was no function to release these ones, they were only created
so the patch adds an accept_queue_deinit() call.

3 years agoCLEANUP: deinit: release the config postparsers
Willy Tarreau [Wed, 27 Apr 2022 16:07:24 +0000 (18:07 +0200)] 
CLEANUP: deinit: release the config postparsers

These ones were not released either, it just requires to export the list
("postparsers") and it makes valgrind happy.

3 years agoCLEANUP: deinit: release the pre-check callbacks
Willy Tarreau [Wed, 27 Apr 2022 16:02:54 +0000 (18:02 +0200)] 
CLEANUP: deinit: release the pre-check callbacks

The freeing of pre-check callbacks was missing when this feature was
recently added with commit b53eb8790 ("MINOR: init: add the pre-check
callback"), let's do it to make valgrind happy.

3 years agoCLEANUP: chunks: release trash also in deinit
Willy Tarreau [Wed, 27 Apr 2022 15:55:41 +0000 (17:55 +0200)] 
CLEANUP: chunks: release trash also in deinit

Tim reported in issue #1676 that just like startup_logs, trash buffers
are not released on deinit since they're thread-local, but valgrind
notices it when quitting before creating threads like "-c -f ...". Let's
just subscribe the function to deinit in addition to threads' end.

The two "free(x);x=NULL;" in free_trash_buffers_per_thread() were
also simplified using ha_free().

3 years agoCLEANUP: errors: also call deinit_errors_buffers() on deinit()
Willy Tarreau [Wed, 27 Apr 2022 15:50:53 +0000 (17:50 +0200)] 
CLEANUP: errors: also call deinit_errors_buffers() on deinit()

Tim reported in issue #1676 that we don't release startup logs if we
warn during startup and quit before creating threads (e.g. -c -f ...).
Let's subscribe deinit_errors_buffers() to both thread's end and
deinit. That's OK since it uses both per-thread and global variables,
and is idempotent.

3 years agoMINOR: ssl: add a new global option "tune.ssl.hard-maxrecord"
Thomas Prückl [Wed, 27 Apr 2022 11:04:54 +0000 (13:04 +0200)] 
MINOR: ssl: add a new global option "tune.ssl.hard-maxrecord"

Low footprint client machines may not have enough memory to download a
complete 16KB TLS record at once. With the new option the maximum
record size can be defined on the server side.

Note: Before limiting the the record size on the server side, a client should
consider using the TLS Maximum Fragment Length Negotiation Extension defined
in RFC6066.

This patch fixes GitHub issue #1679.

3 years agoBUG/MINOR: pools: make sure to also destroy shared pools in pool_destroy_all()
Willy Tarreau [Wed, 27 Apr 2022 09:33:13 +0000 (11:33 +0200)] 
BUG/MINOR: pools: make sure to also destroy shared pools in pool_destroy_all()

In issue #1677, Tim reported that we don't correctly free some shared
pools on exit. What happens in fact is that pool_destroy() is meant to
be called once per pool *pointer*, as it decrements the use count for
each pass and only releases the pool when it reaches zero. But since
pool_destroy_all() iterates over the pools list, it visits each pool
only once and will not eliminate some of them, which thus remain in the
list.

In an ideal case, the function should loop over all pools for as long
as the list is not empty, but that's pointless as we know we're exiting,
so let's just set the users count to 1 before the call so that
pool_destroy() knows it can delete and release the entry.

This could be backported to all versions (memory.c in 2.0 and older) but
it's not a real problem in practice.

3 years agoBUILD: fd: disguise the fd_set_nonblock/cloexec result
Willy Tarreau [Wed, 27 Apr 2022 08:50:00 +0000 (10:50 +0200)] 
BUILD: fd: disguise the fd_set_nonblock/cloexec result

We thought that we could get rid of some DISGUISE() with commit a80e4a354
("MINOR: fd: add functions to set O_NONBLOCK and FD_CLOEXEC") thanks to
the calls being in a function but that was without counting on Coverity.
Let's put it directly in the function since most if not all callers don't
care about this result.

3 years agoMINOR: Call deinit_and_exit(0) for `haproxy -vv`
Tim Duesterhus [Tue, 26 Apr 2022 22:08:11 +0000 (00:08 +0200)] 
MINOR: Call deinit_and_exit(0) for `haproxy -vv`

It appears that it is safe to call perform a clean deinit at this point, so
let's do this to exercise the deinit paths some more.

Running `valgrind --leak-check=full --show-leak-kinds=all ./haproxy -vv` with
this change reports:

    ==261864== HEAP SUMMARY:
    ==261864==     in use at exit: 344 bytes in 11 blocks
    ==261864==   total heap usage: 1,178 allocs, 1,167 frees, 1,102,089 bytes allocated
    ==261864==
    ==261864== 24 bytes in 1 blocks are still reachable in loss record 1 of 2
    ==261864==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==261864==    by 0x324BA6: hap_register_pre_check (init.c:92)
    ==261864==    by 0x155824: main (haproxy.c:3024)
    ==261864==
    ==261864== 320 bytes in 10 blocks are still reachable in loss record 2 of 2
    ==261864==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==261864==    by 0x26E54E: cfg_register_postparser (cfgparse.c:4238)
    ==261864==    by 0x155824: main (haproxy.c:3024)
    ==261864==
    ==261864== LEAK SUMMARY:
    ==261864==    definitely lost: 0 bytes in 0 blocks
    ==261864==    indirectly lost: 0 bytes in 0 blocks
    ==261864==      possibly lost: 0 bytes in 0 blocks
    ==261864==    still reachable: 344 bytes in 11 blocks
    ==261864==         suppressed: 0 bytes in 0 blocks

which is looking pretty good.

3 years agoBUG/MINOR: resolvers: Fix memory leak in resolvers_deinit()
Tim Duesterhus [Tue, 26 Apr 2022 21:28:47 +0000 (23:28 +0200)] 
BUG/MINOR: resolvers: Fix memory leak in resolvers_deinit()

A config like the following:

    global
     stats socket /run/haproxy/admin.sock mode 660 level admin expose-fd listeners

    resolvers unbound
     nameserver unbound 127.0.0.1:53

will report the following leak when running a configuration check:

    ==241882== 6,991 (6,952 direct, 39 indirect) bytes in 1 blocks are definitely lost in loss record 8 of 13
    ==241882==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==241882==    by 0x25938D: cfg_parse_resolvers (resolvers.c:3193)
    ==241882==    by 0x26A1E8: readcfgfile (cfgparse.c:2171)
    ==241882==    by 0x156D72: init (haproxy.c:2016)
    ==241882==    by 0x156D72: main (haproxy.c:3037)

because the `.px` member of `struct resolvers` is not freed.

The offending allocation was introduced in
c943799c865c04281454a7a54fd6c45c2b4d7e09 which is a reorganization that
happened during development of 2.4.x. This fix can likely be backported without
issue to 2.4+ and is likely not needed for earlier versions as the leak happens
during deinit only.

3 years agoCLEANUP: Destroy `http_err_chunks` members during deinit
Tim Duesterhus [Tue, 26 Apr 2022 21:35:07 +0000 (23:35 +0200)] 
CLEANUP: Destroy `http_err_chunks` members during deinit

To make the deinit function a proper inverse of the init function we need to
free the `http_err_chunks`:

    ==252081== 311,296 bytes in 19 blocks are still reachable in loss record 50 of 50
    ==252081==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==252081==    by 0x2727EE: http_str_to_htx (http_htx.c:914)
    ==252081==    by 0x272E60: http_htx_init (http_htx.c:1059)
    ==252081==    by 0x26AC87: check_config_validity (cfgparse.c:4170)
    ==252081==    by 0x155DFE: init (haproxy.c:2120)
    ==252081==    by 0x155DFE: main (haproxy.c:3037)

3 years agoBUG/MEDIUM: http-ana: Fix memleak in redirect rules with ignore-empty option
Christopher Faulet [Tue, 26 Apr 2022 18:34:38 +0000 (20:34 +0200)] 
BUG/MEDIUM: http-ana: Fix memleak in redirect rules with ignore-empty option

A memory leak was introduced when ignore-empty option was added to redirect
rules. If there is no location, when this option is set, the redirection is
aborted and the processing continues. But when this happened, the trash buffer
allocated to format the redirect response was not released.

The bug was introduced by commit bc1223be7 ("MINOR: http-rules: add a new
"ignore-empty" option to redirects.").

This patch should fix the issue #1675. It must be backported to 2.5.

3 years agoMINOR: connection: Add way to disable active connection closing during soft-stop
Remi Tricot-Le Breton [Tue, 26 Apr 2022 13:17:18 +0000 (15:17 +0200)] 
MINOR: connection: Add way to disable active connection closing during soft-stop

If the "close-spread-time" option is set to "infinite", active
connection closing during a soft-stop can be disabled. The 'connection:
close' header or the GOAWAY frame will not be added anymore to the
server's response and active connections will only be closed once the
clients disconnect. Idle connections will not be closed all at once when
the soft-stop starts anymore, and each idle connection will follow its
own timeout based on the multiple timeouts set in the configuration (as
is the case during regular execution).

This feature request was described in GitHub issue #1614.
This patch should be backported to 2.5. It depends on 'MEDIUM: global:
Add a "close-spread-time" option to spread soft-stop on time window'.

3 years agoBUILD: compiler: properly distinguish weak and global symbols
Willy Tarreau [Tue, 26 Apr 2022 17:35:38 +0000 (19:35 +0200)] 
BUILD: compiler: properly distinguish weak and global symbols

While weak symbols were finally fixed with commit fb1b6f5bc ("BUILD:
compiler: use a more portable set of asm(".weak") statements"), it
was an error to think that initcall symbols were also weak. They must
not be and they're only global. The reason is that any externally
linked code loaded as a .so would drop its weak symbols when being
loaded, hence its initcalls that may contain various function
registration calls.

The ambiguity came from the fact that we initially reused the initcall's
HA_GLOBL macro for OSX then generalized it, then turned it to a choice
between .globl and .weak based on the OS, while in fact we needed a
macro to define weak symbols.

Let's rename the macro to HA_WEAK() to make it clear it's only for weak
symbols, and redefine HA_GLOBL() that initcall needs.

This will need to be backported wherever the commit above is backported
(at least 2.5 for now).

3 years agoBUG/MEDIUM: ssl/cli: fix yielding in show_cafile_detail
William Lallemand [Tue, 26 Apr 2022 16:17:15 +0000 (18:17 +0200)] 
BUG/MEDIUM: ssl/cli: fix yielding in show_cafile_detail

HAProxy crashes when "show ssl ca-file" is being called on a ca-file
which contains a lot of certificates. (127 in our test with
/etc/ssl/certs/ca-certificates.crt).

The root cause is the fonction does not yield when there is no available
space when writing the details, and we could write a lot.

To fix the issue, we try to put the data with ci_putchk() after every
show_cert_detail() and we yield if the ci_putchk() fails.

This patch also cleans up a little bit the code:

- the end label is now a real end with a return 1;
- i0 is used instead of (long)p1
- the ID is stored upon yield

3 years agoMEDIUM: httpclient: re-enable the verify by default
William Lallemand [Tue, 26 Apr 2022 10:00:06 +0000 (12:00 +0200)] 
MEDIUM: httpclient: re-enable the verify by default

Since the httpclient verify now has a fallback which disable the SSL in
the httpclient without exiting haproxy at startup, we can safely
re-enable it by default.

It could still be disabled with "httpclient-ssl-verify none".

3 years agoBUG/MINOR: ssl: memory leak when trying to load a directory with ca-file
William Lallemand [Tue, 26 Apr 2022 13:57:33 +0000 (15:57 +0200)] 
BUG/MINOR: ssl: memory leak when trying to load a directory with ca-file

This patch fixes a memory leak of the ca structure when trying to load a
directory with the ca-file directive.

No backport needed.

3 years agoBUG/MINOR: ssl: free the cafile entries on deinit
William Lallemand [Tue, 26 Apr 2022 13:44:53 +0000 (15:44 +0200)] 
BUG/MINOR: ssl: free the cafile entries on deinit

The cafile_tree was never free upon deinit, making valgrind and ASAN
complains when haproxy quits.

This could be backported as far as 2.2 but it requires the
ssl_store_delete_cafile_entry() helper from
5daff3c8abc658760a0d0c5fbbc633bfff1afe44.