When the strict level is zero and BUG_ON() is not implemented, some
possible null-deref warnings are emitted again because some were
covering for these cases. Let's make it fall back to ASSUME() so that
the compiler continues to know that the tested expression never happens.
It also allows to further optimize certain functions by helping the
compiler eliminate certain tests for impossible values. However it
requires that the expression is really evaluated before passing the
result through ASSUME() otherwise it was shown that gcc-11 and above
will fail to evaluate its implications and will continue to emit the
null-deref warnings in case the expression is non-trivial (e.g. it
has multiple terms).
We don't do it for BUG_ON_HOT() however because the extra cost of
evaluating the condition is generally not welcome in fast paths,
particularly when that BUG_ON_HOT() was kept disabled for
performance reasons.
At plenty of places we have ALREADY_CHECKED() or DISGUISE() on a pointer
just to avoid "possibly null-deref" warnings. These ones have the side
effect of weakening optimizations by passing through an assembly step.
Using ASSUME_NONNULL() we can avoid that extra step. And when the
__builtin_unreachable() builtin is not present, we fall back to the old
method using assembly. The macro returns the input value so that it may
be used both as a declarative way to claim non-nullity or directly inside
an expression like DISGUISE().
Clang apparently has __builtin_assume() which does exactly the same
as our macro, since at least v3.8. Let's enable it, in case it may
even better detect assumptions vs unreachable code.
This macro takes an expression, tests it and calls an unreachable
statement if false. This allows the compiler to know that such a
combination does not happen, and totally eliminate tests that would
be related to this condition. When the statement is not available
in the compiler, we just perform a break from a do {} while loop
so that the expression remains evaluated if needed (e.g. function
call).
Due to __builtin_unreachable() only being associated to gcc 4.5 and
above, it turns out it was not enabled for clang. It's not used *that*
much but still a little bit, so let's enable it now. This reduces the
code size by 0.2% and makes it a bit more efficient.
We already have a __has_attribute() macro to detect when the compiler
supports a specific attribute, but we didn't have the equivalent for
builtins. clang-3 and gcc-10 have __has_builtin() for this. Let's just
bring it using the same mechanism as __has_attribute(), which will allow
us to simply define the macro's value for older compilers. It will save
us from keeping that many compiler-specific tests that are incomplete
(e.g. the __builtin_unreachable() test currently doesn't cover clang).
If neither DEBUG_GLITCHES nor DEBUG_STRICT is set, we end up with
no dbg_cnt section, resulting in debug_parse_cli_counters not
building due to __stop_dbg_cnt and __start_dbg_cnt not being defined.
Let's just condition the end of the function to these conditions.
An alternate approach (less elegant) is to always declare a dummy
entry of type DBG_COUNTER_TYPES in debug.c.
This must be backported to 3.1 since it was brought with glitches.
pendconn_grab_from_px() was called when a server was brought back up, to
get some streams waiting in the proxy's queue and get them to run on the
newly available server. It is very similar to process_srv_queue(),
except it only goes through the proxy's queue, which can be a problem,
because there is a small race condition that could lead us to add more
streams to the server queue just as it's going down. If that happens,
the server would just be ignored when back up by new streams, as its
queue is not empty, and it would never try to process its queue.
The other problem with pendconn_grab_from_px() is that it is very
liberal with how it dequeues streams, and it is not very good at
enforcing maxconn, it could lead to having 3*maxconn connections.
For both those reasons, just get rid of pendconn_grab_from_px(), and
just use process_srv_queue().
Both problems are easy to reproduce, especially on a 64 threads machine,
set a maxconn to 100, inject in H2 with 1000 concurrent connections
containing up to 100 streams each, and after a few seconds/minutes the
max number of concurrent output streams will be much higher than
maxconn, and eventually the server will stop processing connections.
It may be related to github issue #2744. Note that it doesn't totally
fix the problem, we can occasionally see a few more connections than
maxconn, but the max that have been observed is 4 more connections, we
no longer get multiple times maxconn.
have more outgoing connections than maxconn,
This should be backported up to 2.6.
In stream_free(), make sure we call process_srv_queue() each time we
call sess_change_server(), otherwise a server may end up not dequeuing
any stream when it could do so. In some extreme cases it could lead to
an infinite loop, as the server would appear to be available, as its
"served" parameter would be < maxconn, but would end up not being used,
as there are elements still in its queue.
This should be backported up to 2.6.
In sc_notify(), it remained a case where it was possible to set an
expiration date on the stream in the past, leading to a crash because of a
BUG_ON(). This must never happen of course.
In sc_notify(), The stream's expiration may be updated in case no wakeup
conditions are encoutered. In that case, we must take care to never set an
expiration date in the past. However, it appeared there was still a
condition to do so. This code is based on an implicit postulate: the
stream's expiration date must always be set when we leave
process_stream(). It was true since the 2.9. But in 3.0, the buffer
allocation mechanism was improved and on an alloc failure in
process_stream(), the stream is inserted in a wait-list and its expiration
date is set to TICK_ETERNITY. With the good timing, and an analysis
expiration date set on a channel, it is possible to set the stream's
expiration date in past.
After analysis, it appeared that the proper way to fix the issue is to only
evaluate I/O timers (read and write timeout) and not stream's timers
(analase_exp or conn_exp) because only I/O timers may have changed since the
last process_stream() call.
This patch must be backported as far as 3.0 to fix the issue. But it is
probably a good idea to also backported it as far as 2.8.
When doing a 'show ssl ca-file <filename>', prefixing a filename with a '*'
allows to show the uncommited transaction asociated to this filename.
However for people using '*' as the first character of their
filename, there is no way to access this filename.
This patch fixes the problem by allowing to escape the first
character with \.
This should be backported in every stable branches.
When doing a 'show ssl crl-file <filename>', prefixing a filename with a '*'
allows to show the uncommited transaction asociated to this filename.
However for people using '*' as the first character of their
filename, there is no way to access this filename.
This patch fixes the problem by allowing to escape the first
character with \.
This should be backported in every stable branches.
When doing a 'show ssl cert <filename>', prefixing a filename with a '*'
allows to show the uncommited transaction asociated to this filename.
However for people using '*' as the first character of their filename,
there is no way to access this filename.
This patch fixes the problem by allowing to escape the first character
with \.
This should be backported in every stable branches.
Let's encapsulate the code, which checks the applied nofile limit into
a separate helper check_nofile_lim_and_prealloc_fd(). Let's keep in this new
function scope the block, which tries to create a copy of FD with the highest
number, if prealloc-fd is set in the configuration.
In step_init_3() we try to apply provided or calculated earlier haproxy
maxsock and memmax limits.
Let's encapsulate these code blocks in dedicated functions:
apply_nofile_limit() and apply_memory_limit() and let's move them into
limits.c. Limits.c gathers now all the logic for calculating and setting
system limits in dependency of the provided configuration.
Let's encapsulate the code, which calculates global.maxconn and
global.maxsslconn into a dedicated function set_global_maxconn() and let's
move this function in limits.c. In limits.c we keep helpers to calculate and
check haproxy internal limits, based on the system nofile and memory limits.
Rename bbr_is_probing_bw() to bbr_is_in_a_probe_state() and
bbr_is_accelerating_probing_bw() to bbr_is_probing_bw() to match
the function names of the BBR v3 internet draft.
Must be backported to 3.1 to ease any further backport to come.
Startup state is also a probing with acceleration bandwidth state.
This modification should have come with this previous one:
BUG/MINOR: quic: reduce packet losses at least during ProbeBW_CRUISE (BBR)
Must be backported to 3.1.
User tried to update a PEM, generated automatically. Part of this PEM has LF
line endings, and another part (CA certificate), added by some API, has CRLF
line endings. This has revealed a bug in cli_snd_buf(), see more
details in issue GitHUB #2818. So, let's add an example of such PEM in our
SSL regtest.
cli_snd_buf() analyzez input line by line. Before this patch it has always
scanned a given line for the presence of '\r' followed by '\n'.
This is only needed for strings, that contain the commands itself like
"show ssl cert\n", "set ssl cert test.pem <<\n".
In case of strings, which contain the command's payload, like
"-----BEGIN CERTIFICATE-----\r\n", '\r\n' should be preserved
as is.
This patch fixes the GitHub issue #2818.
This patch should be backported in v3.1 and in v3.0.
This bug fixes the 3rd condition used by bbr_check_startup_high_loss() to decide
it has detected some high loss as mentioned by the BBR v3 RFC draft:
4.3.1.3. Exiting Startup Based on Packet Loss
...
There are at least BBRStartupFullLossCnt=6 discontiguous sequence ranges lost in that round trip.
where a <= operator was used in place of <.
Must be backported to 3.1.
bbr_congestion_event() role is to track the start time of recovery periods.
This was done using <ts> passed as parameter. But this parameter is the
time the newest lost packet has been sent.
The timestamp value to store in ->recovery_start_ts is <now_ms>.
Must be backported to 3.1.
->app_limited quic_drs struct member is not a boolean. This is
the index of the last transmitted packet marked as application-limited, or 0 if
the connection is not currently application-limited (see C.app_limited
definition in BBR v3 draft).
After these commits:
BUG/MINOR: quic: remove max_bw filter from delivery rate sampling
BUG/MINOR: quic: fix BBB max bandwidth oscillation issue
where some members were removed from bbr struct, the private data
size of QUIC cc algorithms may be reduced from 160 to 144 uint32_t.
Should be easily backported to 3.1 alonside the commits mentioned above.
Upon congestion events (for a instance packet loss),
bbr_adapt_lower_bounds_from_congestion() role is to adapt some BBR internal
variables in relation with the estimated bandwidth (BBR.bw).
According to the BBR v3 draft, this function should do nothing
if BBRIsProbingBW() pseudo-code returns true. That said, this function
is not defined by the BBR v3 draft. But according to this part mentioned before
defining the pseudo-code for BBRAdaptLowerBoundsFromCongestion():
4.5.10.3. When not Probing for Bandwidth
When not explicitly accelerating to probe for bandwidth (Drain, ProbeRTT,
ProbeBW_DOWN, ProbeBW_CRUISE), BBR responds to loss by slowing down to some extent.
This is because loss suggests that the available bandwidth and safe volume of
in-flight data may have decreased recently, and the flow needs to adapt, slowing
down toward the latest delivery process. BBR flows implement this response by
reducing the short-term model parameters, BBR.bw_lo and BBR.inflight_lo.
BBRIsProbingBW() should concern the accelerating probe for bandwidth states
which are BBR_ST_PROBE_BW_REFILL and BBR_ST_PROBE_BW_UP.
Adapt the code to match this latter assumption. At least this reduce
drastically the packet loss volumes at least during ProbeBW_CRUISE.
As an example, on a 100MBits/s internet link with ~94ms as RTT, before
this patch, 4329640 sent packets were needed with 1617119 lost packets (!!!) to
download a 3GB object. After this patch, 2843952 sent packets vs 144134 lost packets
are needed. There may be some packet loss issue. I suspect the maximum bandwidth
which may be overestimated. More this is the case, more the packet loss is big.
That said, at this time, it remains below 5% depending on the size of the objects,
5% being for more than 2GB objects.
Must be backported to 3.1.
Add a test to ensure that values of a local variable used by
bbr_inflight_hi_from_lost_packet() is not be impacted by underflow issues
when subtracting too big numbers and make this function return a correct value.
Must be backported to 3.1.
This filter is no more needed after this commit:
BUG/MINOR: quic: fix BBB max bandwidth oscillation issue.
Indeed, one added this filter at delivery rate sampling level to filter
the BBR max bandwidth estimations and was inspired from ngtcp2 code source when
trying to fix the oscillation issue. But this BBR max bandwidth oscillation issue
was fixed by the aforementioned commit.
Furthermore this code tends to always increment the BBR max bandwidth. From my point
of view, this is not a good idea at all.
Must be backported to 3.1.
This bug arrived with this commit:
6404b7a18a BUG/MINOR: quic: fix bbr_inflight() calls with wrong gain value
This patch partially reverts after having checked the BBR v3 draft.
This bug was invisible when testing long BBR flows.
Must be backported to 3.1.
Remove the code in relation with BBR.ack_phase as per this commit:
ee98c12ad6
I do now kwow at this time why such a request was pushed on GH for the BBR v3 draft
pseudo-code. That said, the use of such an ack phase seemed confusing, adding much
more information about a BBR flow state than needed. Indeed, the ack phase
state is modified several times in the BBR draft pseudo-code but only used to
decide if the max bandwidth filter virtual clock had to be incremented by
BBRAdvanceMaxBwFilter().
In addition to this, when discussing about haproxy BBR implementation with
Neal Cardwell on the BBR development google group about an oscillation issue
of the max bandwidth (BBR.max_bw), I concluded that this was due to the fact
that its filter virutal clock was too often update, due to the ack phase wich
was stalled in BBR_ACK_PHASE_ACKS_PROBE_STOPPING state for too long. This is
where Neal asked me to test the aforementioned commit. This definitively
makes the max bandwidth (BBR.max_bw) oscillation issue disappear.
Another solution would have been to add a new ack phase enum afer
BBR_ACK_PHASE_ACKS_PROBE_STOPPING. BBR_ACK_PHASE_ACKS_PROBE_STOPPED
would have been a good candidate.
Remove the code in relation with BBR.ack_phase.
Must be backported to 3.1.
The windowed filters are used only the BBR implementation for QUIC to filter
the maximum bandwidth samples for its estimation over a virtual time interval
tracked by counting the cyclical progression through ProbeBW cycles. ngtcp2
and quiche use such windowed filters in their BBR implementation. But in a
slightly different way. When updating the 2nd or 3rd filter samples, this
is done based on their values in place of the time they have been sampled.
It seems more logical to rely on the sample timestamps even if this has no
implication because when a sample is updated using another sample because it
has the same value, they have both the same timestamps!
This patch modifies two statements which compare two consecutive filter samples
based on their values (smp[]->v) by statements which compare them based on the
virtual time they have been sampled (smp[]->t). This fully complies which the
code used by the Linux kernel in lib/win_minmax.c.
Alo take the opportunity of this patch to shorten some statements using <smp>
local variable value to update smp[2] sample in place of initializing its two
members with the <smp> member values.
This patch SHOULD be easily backported to 3.1 where BBR was first implemented.
It's been some time since we are compatible with multiple SSL libraries,
let's rename the "OpenSSL library" strings in "SSL library" strings in
haproxy -vv, in order to be more generic.
Add the FIPS mode in haproxy -vv, it need to be activated on the system
with openssl.cnf or by compiling the SSL library with the right options.
Can't work with OpenSSL >= 3.0 because fips a "provider" to load, works
with AWS-LC, WolfSSL and OpenSSL 1.1.1.
Provide alternative code during stats dump when stress mode is active.
The objective is to force the applet to yield on every output line. This
allows to easily test reentrant code paths, in particular while adding
and removing server instances.
To support this, output is interrupted every time the output buffer (or
its equivalent) is not empty. Use COND_STRESS() macro to provide default
and stress alternative conditions.
Previous patch introduced stress mode to be able to easily test
alternative code paths.
The first point would be to force interruption of stats dump on every
line and check reentrant patchs, in particular while adding and removing
servers instances.
The purpose of this patch is to be able to use applet_putchk_stress()
during stats dump while not impacting other applets. To support this,
extract applet_putchk() into an internal _applet_putchk() which have a
new argument stress. Define two helpers applet_putchk() and
applet_putchk_stress(), the latter to set the stress argument to true.
For the moment, applet_putchk_stress() is not used. This will be the
subject of the next patch.
Define a new build mode DEBUG_STRESS. This will be used to stress some
code parts which cannot be reproduce easily with an alternative
suboptimal code.
First, a global <mode_stress> is set either to 1 or 0 depending on
DEBUG_STRESS compilation. A new global keyword "stress-level" is also
defined. It allows to specify a level from 0 to 9, to increase the
stress incurred on the code.
Helper macro STRESS_RUN* are defined for each stress level. This allows
to easily specify an instruction in default execution and a stress
counterpart if running on the corresponding stress level.
Released version 3.2-dev1 with the following main changes :
- MINOR: pattern: split pat_ref_set()
- MINOR: pattern: add pat_ref_gen_set() function
- MINOR: pattern: add pat_ref_gen_find_elt() function
- MINOR: pattern: add pat_ref_gen_delete() function
- MEDIUM: pattern: consider gen_id in pat_ref_set_from_node()
- MEDIUM: pattern: always consider gen_id for pat_ref lookup operations
- MINOR: version: this is development again (3.2)
- DEV: patchbot: prepare for new version 3.2-dev
- BUG/MEDIUM: sock: Remove FD_POLL_HUP during connect() if FD_POLL_ERR is not set
- MINOR: proxy: Add support of 421-Misdirected-Request in retry-on status
- BUG/MINOR: log: fix lf_text() behavior with empty string
- MINOR: log: always consider "+M" option in lf_text_len()
- BUG/MINOR: improve BBR throughput on very fast links
- MINOR: event_hdl: add PAT_REF events
- MINOR: pattern: publish event_hdl events on pat_ref updates
- MINOR: hlua: add patref class
- MINOR: hlua: add core.get_patref method
- MINOR: hlua_fcn: implement index and pair metamethods for patref class
- MINOR: hlua_fcn: wrap pat_ref struct for patref class
- MINOR: pattern: add pat_ref_may_commit() helper function
- MINOR: hlua_fcn: add Patref:commit() method
- MINOR: hlua_fcn: add Patref:prepare() method
- MINOR: hlua_fcn: add Patref:purge() method
- MINOR: hlua_fcn: add Patref:giveup()
- MINOR: hlua_fcn: add Patref:add()
- MINOR: hlua_fcn: add Patref:del()
- MINOR: hlua_fcn: add Patref:set()
- MINOR: hlua_fcn: add Patref:add_bulk()
- MINOR: hlua_fcn: add Patref:event_sub()
- DOC: lua: prefer Patref:{set,add}() over legacy methods for acl and maps
- BUG/MINOR: hlua_fcn: fix Patref:set() force parameter
- BUG/MEDIUM: event_hdl: fix uninitialized value in async mode when no data is provided
- BUG/MEDIUM: quic: prevent stream freeze on pacing
- BUG/MEDIUM: http-ana: Reset request flag about data sent to perform a L7 retry
- BUG/MINOR: h1-htx: Use default reason if not set when formatting the response
- BUILD: quic: fix a build error about an non initialized timestamp
- CI: github: allow coredumps on aws-lc and wolfssl jobs
- BUG/MINOR: listener: fix potential null pointer dereference in listener_release()
- MINOR: hlua: fix ambiguous hlua usage in hlua_filter_delete()
- BUG/MINOR: signal: register default handler for SIGINT in signal_init()
- BUG/MINOR: startup: close pidfd and free global.pidfile in handle_pidfile()
- BUG/MINOR: startup: fix pidfile creation
- MINOR: tools: add a new macro DEFVAL() to provide a default argument
- MINOR: tasklet: set TASK_WOKEN_OTHER on tasklets by default
- BUG/MINOR: quic: fix bbr_inflight() calls with wrong gain value
- BUG/MEDIUM: init: make sure only daemonized processes change their session
- BUG/MINOR: init: do not call fork_poller() for non-forked processes
- BUG/MEDIUM: mux-quic: remove pacing status when everything is sent
- BUG/MINOR: quic: remove startup alert if conn socket-owner unsupported
- BUG/MINOR: quic: remove startup alert if GSO unsupported
- MINOR: stktable: implement "recv-only" table option
- CLEANUP: stktable: replace nopurge attribute with flag
- CLEANUP: stktable: add some stktable flags polishing
- BUG/MEDIUM: mux-h2: make sure not to touch dummy streams when sending WU
- MINOR: mux-quic: clean up zero-copy done_ff callback
- BUG/MINOR: config: Fix parsing of accept-invalid-http-{request,response}
- BUG/MINOR: mworker: don't save program PIDs in oldpids
- BUG/MINOR: mworker: fix -D -W -sf/-st modes
- BUG/MINOR: startup: fix error path for master, if can't open pidfile
- CLEANUP: startup: make if condition to kill old pids more readable
- DOC: config: fix confusing init-state examples
- MINOR: mux-h1: use explicit __objt_server on idle conn reinsert
- MINOR: mux-h2: use explicit __objt_server on idle conn reinsert
- MINOR: mux-spop: use explicit __objt_server on idle conn reinsert
- MINOR: mux-fcgi: use explicit __objt_server on idle conn reinsert
- MINOR: quic: convert startup check in a freestanding function
- MINOR: quic: split startup check function
- MINOR: quic: implement build options report
- BUG/MINOR: debug: COUNT_IF() should return true/false
- MINOR: mux-h2/traces: add a missing trace on negative initial window size
- CLEANUP: mux-h2/traces: reword certain ambiguous traces
- MINOR: mux-h2/glitches: add a description to the H2 glitches
- BUG/MINOR: mux-h2: fix expression when detecting excess of CONTINUATION frames
- BUILD: debug: fix build issues in COUNT_IF() with -Wunused-value
- MINOR: tools: make fddebug() automatically emit the location
- MINOR: ssl: add notBefore and notAfter utility functions
- MEDIUM: ssl/cli: "show ssl sni" list the loaded SNI in frontends
- BUG/MEDIUM: startup: don't daemonize if started with -c
- BUG/MEDIUM: startup: report status if daemonized process fails
- BUG/MEDIUM: mworker: report status, if daemonized master fails
- BUG/MINOR: mworker: detach from tty when received READY from worker
- BUG/MINOR: namespace: handle a possible strdup() failure
- BUG/MINOR: ssl_crtlist: handle a possible strdup() failure
- BUG/MINOR: resolvers: handle a possible strdup() failure
- CI: use "/tmp" as default value for TMPDIR when searching logs
- DOC: management: fix typos and paragraph ordering in 'show ssl sni'
- CLEANUP: ssl: fix comment in 'show ssl sni'
- MINOR: ssl/cli: add negative filters to "show ssl sni"
- BUG/MINOR: stats: decrement srv refcount on stats-file release
- MINOR: list: define a watcher type
- BUG/MEDIUM: stats/server: use watcher to track server during stats dump
- MINOR: server: remove prev_deleted server list
- BUG/MINOR: http-fetch: Ignore empty argument string for query()
- BUG/MINOR: server-state: Fix expiration date of srvrq_check tasks
- BUG/MINOR: hlua_fcn: restore server pairs iterator pointer consistency
Since 9c91b30 ("MINOR: server: remove prev_deleted server list"), hlua
server pair iterator may use and return invalid (stale) server pointer
if multiple servers were deleted between two iterations.
Indeed, the server refcount mechanism (using srv_take()) is no longer
sufficient as the prev_deleted mitigation was removed.
To ensure server pointer consistency between two yields, the new watcher
mechanism must be used (as it already the case for stats dumping).
Thus in this patch we slightly change the server iteration logic:
hlua_server_list_iterator_context struct now stores the next valid server
pointer, and a watcher is added to ensure this pointer is never stale.
Then in hlua_listable_servers_pairs_iterator(), this next pointer is used
to create the Lua server object, and the next valid pointer is obtained by
leveraging watcher_next().
No backport needed unless 9c91b30 ("MINOR: server: remove prev_deleted
server list") is. Please note that dynamic servers were not supported in
Lua prior to 2.8, so it doesn't make sense to backport this patch further
than 2.8.
"hold.timeout" was used as expiration date for srvrq_check tasks. But it is
not accurrate. The expiration date must be based on the resolution timeouts
instead (resolve and retry).
The purpose of srvrq_check task is to clean up the server resolution status
when outdated info are inherited from the state file. Using "hold.timeout"
is not accurrate here because hold timeouts concern the resolution response
items not the resolution status of servers. It may be set to a huge value or
0. The expiration date of these tasks must be based on the resolution
timeouts instead.
So now the ("timeout resolve" + resolve_retries * "timeout retry") value is
used.
This patch should fix the issue #2816. It must be backported to all stable
versions.
query() sample fetch function takes an optional argument string. During
configuration parsing, empty string must be ignored. It is especially
important when the sample is used with empty parenthesis. The argument is
optional and it is a list of options to configure the behavior of the sample
fetch. So it is logical to ignore empty strings.
This patch should fix the issue #2815. It must be backported to 3.1.
This patch is a direct follow-up to the previous one. Thanks to watcher
type, it is not safe to assume that servers manipulated via stats dump
were not targetted by a "delete server" CLI command. As such,
prev_deleted list server member is now unneeded. This patch thus removes
any reference to it.
