BUILD/MINOR: ssl: Fix compilation with OpenSSL 1.0.2
The X509_STORE_CTX_get0_cert did not exist yet on OpenSSL 1.0.2 and
neither did X509_STORE_CTX_get0_chain, which was not actually needed
since its get1 equivalent already existed.
Willy Tarreau [Thu, 19 Aug 2021 21:06:58 +0000 (23:06 +0200)]
BUG/MEDIUM: h2: match absolute-path not path-absolute for :path
RFC7540 states that :path follows RFC3986's path-absolute. However
that was a bug introduced in the spec between draft 04 and draft 05
of the spec, which implicitly causes paths starting with "//" to be
forbidden. HTTP/1 (and now HTTP core semantics) made it explicit
that the request-target in origin-form follows a purposely defined
absolute-path defined as 1*(/ segment) to explicitly allow "//".
http2bis now fixes this by relying on absolute-path so that "//"
becomes valid and matches other versions. Full discussion here:
This issue appeared in haproxy with commit 4b8852c70 ("BUG/MAJOR: h2:
verify that :path starts with a '/' before concatenating it") when
making the checks on :path fully comply with the spec, and was backported
as far as 2.0, so this fix must be backported there as well to allow
"//" in H2 again.
MEDIUM: ssl: Keep a reference to the client's certificate for use in logs
Most of the SSL sample fetches related to the client certificate were
based on the SSL_get_peer_certificate function which returns NULL when
the verification process failed. This made it impossible to use those
fetches in a log format since they would always be empty.
The patch adds a reference to the X509 object representing the client
certificate in the SSL structure and makes use of this reference in the
fetches.
The reference can only be obtained in ssl_sock_bind_verifycbk which
means that in case of an SSL error occurring before the verification
process ("no shared cipher" for instance, which happens while processing
the Client Hello), we won't ever start the verification process and it
will be impossible to get information about the client certificate.
This patch also allows most of the ssl_c_XXX fetches to return a usable
value in case of connection failure (because of a verification error for
instance) by making the "conn->flags & CO_FL_WAIT_XPRT" test (which
requires a connection to be established) less strict.
Thanks to this patch, a log-format such as the following should return
usable information in case of an error occurring during the verification
process :
log-format "DN=%{+Q}[ssl_c_s_dn] serial=%[ssl_c_serial,hex] \
hash=%[ssl_c_sha1,hex]"
MINOR: httpclient: implement a simple HTTP Client API
This commit implements a very simple HTTP Client API.
A client can be operated by several functions:
- httpclient_new(), httpclient_destroy(): create
and destroy the struct httpclient instance.
- httpclient_req_gen(): generate a complete HTX request using the
the absolute URL, the method and a list of headers. This request
is complete and sets the HTX End of Message flag. This is limited
to small request we don't need a body.
- httpclient_start() fill a sockaddr storage with a IP extracted
from the URL (it cannot resolve an fqdm for now), start the
applet. It also stores the ptr of the caller which could be an
appctx or something else.
- hc->ops contains a list of callbacks used by the
HTTPClient, they should be filled manually after an
httpclient_new():
* res_stline(): the client received a start line, its content
will be stored in hc->res.vsn, hc->res.status, hc->res.reason
* res_headers(): the client received headers, they are stored in
hc->res.hdrs.
* res_payload(): the client received some payload data, they are
stored in the hc->res.buf buffer and could be extracted with the
httpclient_res_xfer() function, which takes a destination buffer
as a parameter
* res_end(): this callback is called once we finished to receive
the response.
Initialize a proxy which contain a server for the raw HTTP, and another
one for the HTTPS. This proxy will use the global server log definition
and the 'option httplog' directive.
This proxy is internal and will only be used for the HTTP Client API.
Willy Tarreau [Tue, 17 Aug 2021 12:08:55 +0000 (14:08 +0200)]
[RELEASE] Released version 2.5-dev4
Released version 2.5-dev4 with the following main changes :
- MINOR: log: rename 'dontloglegacyconnerr' to 'log-error-via-logformat'
- MINOR: doc: rename conn_status in `option httsplog`
- MINOR: proxy: disabled takes a stopping and a disabled state
- MINOR: stats: shows proxy in a stopped state
- BUG/MINOR: server: fix race on error path of 'add server' CLI if track
- CLEANUP: thread: fix fantaisist indentation of thread_harmless_till_end()
- MINOR: threads: make thread_release() not wait for other ones to complete
- MEDIUM: threads: add a stronger thread_isolate_full() call
- MEDIUM: servers: make the server deletion code run under full thread isolation
- BUG/MINOR: server: remove srv from px list on CLI 'add server' error
- MINOR: activity/fd: remove the dead_fd counter
- MAJOR: fd: get rid of the DWCAS when setting the running_mask
- CLEANUP: fd: remove the now unused fd_set_running()
- CLEANUP: fd: remove the now unneeded fd_mig_lock
- BUG/MINOR: server: update last_change on maint->ready transitions too
- MINOR: spoe: Add a pointer on the filter config in the spoe_agent structure
- BUG/MEDIUM: spoe: Create a SPOE applet if necessary when the last one is released
- BUG/MEDIUM: spoe: Fix policy to close applets when SPOE connections are queued
- MINOR: server: unmark deprecated on enable health/agent cli
- MEDIUM: task: implement tasklet kill
- MINOR: server: initialize fields for dynamic server check
- MINOR: check: allocate default check ruleset for every backends
- MINOR: check: export check init functions
- MINOR: check: do not increment global maxsock at runtime
- MINOR: server: implement a refcount for dynamic servers
- MEDIUM: check: implement check deletion for dynamic servers
- MINOR: check: enable safe keywords for dynamic servers
- MEDIUM: server: implement check for dynamic servers
- MEDIUM: server: implement agent check for dynamic servers
- REGTESTS: server: add dynamic check server test
- MINOR: doc: specify ulimit-n usage for dynamic servers
- REGTESTS: server: fix dynamic server with checks test
- CI: travis-ci: temporarily disable arm64 builds
- BUG/MINOR: check: test if server is not null in purge
- MINOR: global: define MODE_STOPPING
- BUG/MINOR: server: do not use refcount in free_server in stopping mode
- ADMIN: dyncookie: implement a simple dynamic cookie calculator
- BUG/MINOR: check: do not reset check flags on purge
- BUG/MINOR: check: fix leak on add dynamic server with agent-check error
- BUG/MEDIUM: check: fix leak on agent-check purge
- BUG/MEDIUM: server: support both check/agent-check on a dynamic instance
- BUG/MINOR: buffer: fix buffer_dump() formatting
- MINOR: channel: remove an htx block from a channel
- BUG/MINOR: tcpcheck: Properly detect pending HTTP data in output buffer
- BUG/MINOR: stream: Don't release a stream if FLT_END is still registered
- MINOR: lua: Add a flag on lua context to know the yield capability at run time
- BUG/MINOR: lua: Yield in channel functions only if lua context can yield
- BUG/MINOR: lua: Don't yield in channel.append() and channel.set()
- MINOR: filters/lua: Release filters before the lua context
- MINOR: lua: Add a function to get a reference on a table in the stack
- MEDIUM: lua: Process buffer data using an offset and a length
- MEDIUM: lua: Improve/revisit the lua api to manipulate channels
- DOC: Improve the lua documentation
- MEDIUM: filters/lua: Add support for dummy filters written in lua
- MINOR: lua: Add a function to get a filter attached to a channel class
- MINOR: lua: Add flags on the lua TXN to know the execution context
- MEDIUM: filters/lua: Be prepared to filter TCP payloads
- MEDIUM: filters/lua: Support declaration of some filter callback functions in lua
- MEDIUM: filters/lua: Add HTTPMessage class to help HTTP filtering
- MINOR: filters/lua: Add request and response HTTP messages in the lua TXN
- MINOR: filters/lua: Support the HTTP filtering from filters written in lua
- DOC: config: Fix 'http-response send-spoe-group' documentation
- BUG/MINOR: lua: Properly check negative offset in Channel/HttpMessage functions
- BUG/MINOR: lua: Properly catch alloc errors when parsing lua filter directives
- BUG/MEDIUM: cfgcheck: verify existing log-forward listeners during config check
- MINOR: cli: delare the CLI frontend as an internal proxy
- MINOR: proxy: disable warnings for internal proxies
- BUG/MINOR: filters: Always set FLT_END analyser when CF_FLT_ANALYZE flag is set
- BUG/MINOR: lua/filters: Return right code when txn:done() is called
- DOC: lua-api: Add documentation about lua filters
- CI: Remove obsolete USE_SLZ=1 CI job
- CLEANUP: assorted typo fixes in the code and comments
- CI: github actions: relax OpenSSL-3.0.0 version comparision
- BUILD: tools: get the absolute path of the current binary on NetBSD.
- DOC: Minor typo fix - 'question mark' -> 'exclamation mark'
- DOC/MINOR: fix typo in management document
- MINOR: http: add a new function http_validate_scheme() to validate a scheme
- BUG/MAJOR: h2: verify early that non-http/https schemes match the valid syntax
- BUG/MAJOR: h2: verify that :path starts with a '/' before concatenating it
- BUG/MAJOR: h2: enforce stricter syntax checks on the :method pseudo-header
- BUG/MEDIUM: h2: give :authority precedence over Host
- REGTESTS: add a test to prevent h2 desync attacks
Amaury Denoyelle [Fri, 13 Aug 2021 07:43:24 +0000 (09:43 +0200)]
REGTESTS: add a test to prevent h2 desync attacks
This test ensure that h2 pseudo headers are properly checked for invalid
characters and the host header is ignored if :authority is present. This
is necessary to prevent h2 desync attacks as described here
https://portswigger.net/research/http2
Willy Tarreau [Wed, 11 Aug 2021 13:39:13 +0000 (15:39 +0200)]
BUG/MEDIUM: h2: give :authority precedence over Host
The wording regarding Host vs :authority in RFC7540 is ambiguous as it
says that an intermediary must produce a host header from :authority if
Host is missing, but, contrary to HTTP/1.1, doesn't say anything regarding
the possibility that Host and :authority differ, which leaves Host with
higher precedence there. In addition it mentions that clients should use
:authority *instead* of Host, and that H1->H2 should use :authority only
if the original request was in authority form. This leaves some gray
area in the middle of the chain for fully valid H2 requests arboring a
Host header that are forwarded to the other side where it's possible to
drop the Host header and use the authority only after forwarding to a
second H2 layer, thus possibly seeing two different values of Host at
a different stage. There's no such issue when forwarding from H2 to H1
as the authority is dropped only only the Host is kept.
Note that the following request is sufficient to re-normalize such a
request:
http-request set-header host %[req.hdr(host)]
The new spec in progress (draft-ietf-httpbis-http2bis-03) addresses
this trouble by being a bit is stricter on these rules. It clarifies
that :authority must always be used instead of Host and that Host ought
to be ignored. This is much saner as it avoids to convey two distinct
values along the chain. This becomes the protocol-level equivalent of:
http-request set-uri %[url]
So this patch does exactly this, which we were initially a bit reluctant
to do initially by lack of visibility about other implementations'
expectations. In addition it slightly simplifies the Host header field
creation by always placing it first in the list of headers instead of
last; this could also speed up the look up a little bit.
This needs to be backported to 2.0. Non-HTX versions are safe regarding
this because they drop the URI during the conversion to HTTP/1.1 so
only Host is used and transmitted.
Willy Tarreau [Wed, 11 Aug 2021 09:12:46 +0000 (11:12 +0200)]
BUG/MAJOR: h2: enforce stricter syntax checks on the :method pseudo-header
Before HTX was introduced, all the HTTP request elements passed in
pseudo-headers fields were used to build an HTTP/1 request whose syntax
was then scrutinized by the HTTP/1 parser, leaving no room to inject
invalid characters.
While NUL, CR and LF are properly blocked, it is possible to inject
spaces in the method so that once translated to HTTP/1, fields are
shifted by one spcae, and a lenient HTTP/1 server could possibly be
fooled into using a part of the method as the URI. For example, the
following request:
It's important to note that the resulting request is *not* valid, and
that in order for this to be a problem, it requires that this request
is delivered to an already vulnerable HTTP/1 server.
A workaround here is to reject malformed methods by placing this rule
in the frontend or backend, at least before leaving haproxy in H1:
http-request reject if { method -m reg [^A-Z0-9] }
Alternately H2 may be globally disabled by commenting out the "alpn"
directive on "bind" lines, and by rejecting H2 streams creation by
adding the following statement to the global section:
tune.h2.max-concurrent-streams 0
This patch adds a check for each character of the method to make sure
they belong to the ones permitted in a token, as mentioned in RFC7231#4.1.
This should be backported to versions 2.0 and above. For older versions
not having HTX_FL_PARSING_ERROR, a "goto fail" works as well as it
results in a protocol error at the stream level. Non-HTX versions are
safe because the resulting invalid request will be rejected by the
internal HTTP/1 parser.
Willy Tarreau [Tue, 10 Aug 2021 14:30:55 +0000 (16:30 +0200)]
BUG/MAJOR: h2: verify that :path starts with a '/' before concatenating it
Tim Düsterhus found that while the H2 path is checked for non-emptiness,
invalid chars and '*', a test is missing to verify that except for '*',
it always starts with exactly one '/'. During the reconstruction of the
full URI when passing to HTX, this missing test allows to affect the
apparent authority by appending a port number or a suffix name.
This only affects H2-to-H2 communications, as H2-to-H1 do not use the
full URI. Like for previous fix, the following rule inserted before
other ones in the frontend is sufficient to renormalize the internal
URI and let haproxy see the same authority as the target server:
http-request set-uri %[url]
This needs to be backported to 2.2. Earlier versions do not rebuild a
full URI using the authority and will fail on the malformed path at the
HTTP layer, so they are safe.
Willy Tarreau [Tue, 10 Aug 2021 13:37:34 +0000 (15:37 +0200)]
BUG/MAJOR: h2: verify early that non-http/https schemes match the valid syntax
While we do explicitly check for strict character sets in the scheme,
this is only done when extracting URL components from an assembled one,
and we have special handling for "http" and "https" schemes directly in
the H2-to-HTX conversion. Sadly, this lets all other ones pass through
if they start exactly with "http://" or "https://", allowing the
reconstructed URI to start with a different looking authority if it was
part of the scheme.
It's interesting to note that in this case the valid authority is in
the Host header and that the request will only be wrong if emitted over
H2 on the backend side, since H1 will not emit an absolute URI by
default and will drop the scheme. So in essence, this is a variant of
the scheme-based attack described below in that it only affects H2-H2
and not H2-H1 forwarding:
https://portswigger.net/research/http2
As such, a simple workaround consists in just inserting the following
rule before other ones in the frontend, which will have for effect to
renormalize the authority in the request line according to the
concatenated version (making haproxy see the same authority and host
as what the target server will see):
http-request set-uri %[url]
This patch simply adds the missing syntax checks for non-http/https
schemes before the concatenation in the H2 code. An improvement may
consist in the future in splitting these ones apart in the start
line so that only the "url" sample fetch function requires to access
them together and that all other places continue to access them
separately. This will then allow the core code to perform such checks
itself.
The patch needs to be backported as far as 2.2. Before 2.2 the full
URI was not being reconstructed so the scheme and authority part were
always dropped from H2 requests to leave only origin requests. Note
for backporters: this depends on this previous patch:
MINOR: http: add a new function http_validate_scheme() to validate a scheme
Many thanks to Tim Düsterhus for figuring that one and providing a
reproducer.
Willy Tarreau [Tue, 10 Aug 2021 13:35:36 +0000 (15:35 +0200)]
MINOR: http: add a new function http_validate_scheme() to validate a scheme
While http_parse_scheme() extracts a scheme from a URI by extracting
exactly the valid characters and stopping on delimiters, this new
function performs the same on a fixed-size string.
Lua filter support is highly experimental. The documentation was added to
allow first lua filter implementations. The API is not stabilized and must
be improved to be fully usable. This docuementation is quite light for
now. But more will be added.
BUG/MINOR: lua/filters: Return right code when txn:done() is called
txn functions can now be called from an action or a filter context. Thus the
return code must be adapted depending on this context. From an action, act.ABORT
is returned. From a filter, -1 is returned. It is the filter error code.
This bug only affects 2.5-dev. No backport needed.
BUG/MINOR: filters: Always set FLT_END analyser when CF_FLT_ANALYZE flag is set
CF_FLT_ANALYZE flags may be set before the FLT_END analyser. Thus if an error is
triggered in the mean time, this may block the stream and prevent it to be
released. It is indeed a problem only for the response channel because the
response analysers may be skipped on early errors.
So, to prevent any issue, depending on the code path, the FLT_END analyser is
systematically set when the CF_FLT_ANALYZE flag is set.
This patch must be backported in all stable branches.
MINOR: proxy: disable warnings for internal proxies
The internal proxies should be part of the proxies list, because of
this, the check_config_validity() fonction could emit warnings about
these proxies.
This patch disables 3 startup warnings for internal proxies:
- "has no 'bind' directive" (this one was already ignored for the CLI
frontend, but we made it generic instead)
- "missing timeouts"
- "log format ignored"
Emeric Brun [Fri, 13 Aug 2021 07:32:50 +0000 (09:32 +0200)]
BUG/MEDIUM: cfgcheck: verify existing log-forward listeners during config check
User reported that the config check returns an error with the message:
"Configuration file has no error but will not start (no listener) => exit(2)."
if the configuration present only a log-forward section with bind or dgram-bind
listeners but no listen/backend nor peer sections.
The process checked if there was 'peers' section avalaible with
an internal frontend (and so a listener) or a 'listen/backend'
section not disabled with at least one configured listener (into the
global proxies_list). Since the log-forward proxies appear in a
different list, they were not checked.
This patch adds a lookup on the 'log-forward' proxies list to check
if one of them presents a listener and is not disabled. And
this is done only if there was no available listener found into
'listen/backend' sections.
I have also studied how to re-work this check considering the 'listeners'
counter used after startup/init to keep the same algo and avoid further
mistakes but currently this counter seems increased during config parsing
and if a proxy is disabled, decreased during startup/init which is done
after the current config check. So the fix still not rely on this
counter.
This patch should fix the github issue #1346
This patch should be backported as far as 2.3 (so on branches
including the "log-forward" feature)
When a lua filter declaration is parsed, some allocation errors were not
properly handled. In addition, we must be sure the filter identifier is defined
in lua to duplicate it when the filter configuration is filled.
This patch fix a defect reported in the issue #1347. It only concerns
2.5-dev. No backport needed.
BUG/MINOR: lua: Properly check negative offset in Channel/HttpMessage functions
In Channel and HTTPMessage classes, several functions uses an offset that
may be negative to start from the end of incoming data. But, after
calculation, the offset must never be negative. However, there is a bug
because of a bad cast to unsigned when "input + offset" is performed. The
result must be a signed integer.
This patch should fix most of defects reported in the issue #1347. It only
affects 2.5-dev. No backport needed.
MINOR: filters/lua: Support the HTTP filtering from filters written in lua
Now an HTTPMessage class is available to manipulate HTTP message from a filter
it is possible to bind HTTP filters callback function on lua functions. Thus,
following methods may now be defined by a lua filter:
http_headers() and http_end() may return one of the constant filter.CONTINUE,
filter.WAIT or filter.ERROR. If nothing is returned, filter.CONTINUE is used as
the default value. On its side, http_payload() may return the amount of data to
forward. If nothing is returned, all incoming data are forwarded.
For now, these functions are not allowed to yield because this interferes with
the filter workflow.
MINOR: filters/lua: Add request and response HTTP messages in the lua TXN
When a lua TXN is created from a filter context, the request and the response
HTTP message objects are accessible from ".http_req" and ".http_res" fields. For
an HTTP proxy, these objects are always defined. Otherwise, for a TCP proxy, no
object is created and nil is used instead. From any other context (action or
sample fetch), these fields don't exist.
MEDIUM: filters/lua: Add HTTPMessage class to help HTTP filtering
This new class exposes methods to manipulate HTTP messages from a filter
written in lua. Like for the HTTP class, there is a bunch of methods to
manipulate the message headers. But there are also methods to manipulate the
message payload. This part is similar to what is available in the Channel
class. Thus the payload can be duplicated, erased, modified or
forwarded. For now, only DATA blocks can be retrieved and modified because
the current API is limited. No HTTPMessage method is able to yield. Those
manipulating the headers are always called on messages containing all the
headers, so there is no reason to yield. Those manipulating the payload are
called from the http_payload filters callback function where yielding is
forbidden.
When an HTTPMessage object is instantiated, the underlying Channel object
can be retrieved via the ".channel" field.
For now this class is not used because the HTTP filtering is not supported
yet. It will be the purpose of another commit.
MEDIUM: filters/lua: Support declaration of some filter callback functions in lua
It is now possible to write some filter callback functions in lua. All
filter callbacks are not supported yet but the mechanism to call them is now
in place. Following method may be defined in the Lua filter class to be
bound on filter callbacks:
hlua_filter_callback() function is responsible to call the good lua function
depending on the filter callback function. Using some flags it is possible
to allow a lua call to yield or not, to retrieve a return value or not, and
to specify if a channel or an http message must be passed as second
argument. For now, the HTTP part has not been added yet. It is also possible
to add extra argument adding them on the stack before the call.
3 new functions are exposed by the global object "filter". The first one,
filter.wake_time(ms_delay), to set the wake_time when a Lua callback
function yields (if allowed). The two others,
filter.register_data_filter(filter, chn) and
filter.unregister_data_filter(filter, chn), to enable or disable the data
filtering on a channel for a specific lua filter instance.
start_analyse() and end_analyse() may return one of the constant
filter.CONTINUE, filter.WAIT or filter.ERROR. If nothing is returned,
filter.CONTINUE is used as the default value. On its side, tcp_payload() may
return the amount of data to forward. If nothing is returned, all incoming
data are forwarded.
For now, these functions are not allowed to yield because this interferes
with the filter workflow.
function MyFilter:new()
flt = {}
setmetatable(flt, MyFilter)
flt.req_len = 0
flt.res_len = 0
return flt
end
function MyFilter:start_analyze(txn, chn)
filter.register_data_filter(self, chn)
end
function MyFilter:end_analyze(txn, chn)
print("<Total> request: "..self.req_len.." - response: "..self.res_len)
end
function MyFilter:tcp_payload(txn, chn)
offset = chn:ouput()
len = chn:input()
if chn:is_resp() then
self.res_len = self.res_len + len
print("<TCP:Response> offset: "..offset.." - length: "..len)
else
self.req_len = self.req_len + len
print("<TCP:Request> offset: "..offset.." - length: "..len)
end
end
MEDIUM: filters/lua: Be prepared to filter TCP payloads
For filters written in lua, the tcp payloads will be filtered using methods
exposed by the Channel class. So the corrsponding C binding functions must
be prepared to process payload in a filter context and not only in an action
context.
The main change is the offset where to start to process data in the channel
buffer, and the length of these data. For an action, all input data are
considered. But for a filter, it depends on what the filter is allow to
forward when the tcp_payload callback function is called. It depends on
previous calls but also on other filters.
In addition, when the payload is modified by a lua filter, its context must
be updated. Note also that channel functions cannot yield when called from a
filter context.
For now, it is not possible to define callbacks to filter data and the
documentation has not been updated.
MINOR: lua: Add flags on the lua TXN to know the execution context
A lua TXN can be created when a sample fetch, an action or a filter callback
function is executed. A flag is now used to track the execute context.
Respectively, HLUA_TXN_SMP_CTX, HLUA_TXN_ACT_CTX and HLUA_TXN_FLT_CTX. The
filter flag is not used for now.
MINOR: lua: Add a function to get a filter attached to a channel class
For now, there is no support for filters written in lua. So this function,
if called, will always return NULL. But when it will be called in a filter
context, it will return the filter structure attached to a channel
class. This function is also responsible to set the offset of data that may
be processed and the length of these data. When called outside a filter
context (so from an action), the offset is the input data position and the
length is the input data length. From a filter, the offset and the length of
data that may be filtered are retrieved the filter context.
MEDIUM: filters/lua: Add support for dummy filters written in lua
It is now possible to write dummy filters in lua. Only the basis to declare
such filters has been added for now. There is no way to declare callbacks to
filter anything. Lua filters are for now empty nutshells.
To do so, core.register_filter() must be called, with 3 arguments, the
filter's name (as it appears in HAProxy config), the lua class that will be
used to instantiate filters and a function to parse arguments passed on the
filter line in HAProxy configuration file. The lua filter class must at
least define the method new(), without any extra args, to create new
instances when streams are created. If this method is not found, the filter
will be ignored.
Here is a template to declare a new Lua filter:
// haproxy.conf
global
lua-load /path/to/my-filter.lua
...
// my-filter.lua
MyFilter = {}
MyFilter.id = "My Lua filter" -- the filter ID (optional)
MyFilter.flags = filter.FLT_CFG_FL_HTX -- process HTX streams (optional)
MyFilter.__index = MyFilter
function MyFilter:new()
flt = {}
setmetatable(flt, MyFilter)
-- Set any flt fields. self.args can be used
flt.args = self.args
return flt -- The new instance of Myfilter
end
core.register_filter("my-lua-filter", MyFilter, function(filter, args)
-- process <args>, an array of strings. For instance:
filter.args = args
return filter
end)
In this example, 2 filters are declared using the same lua class. The
parsing function is called for both, with its own copy of the lua class. So
each filter will be unique.
The global object "filter" exposes some constants and flags, and later some
functions, to help writting filters in lua.
Internally, when a lua filter is instantiated (so when new() method is
called), 2 lua contexts are created, one for the request channel and another
for the response channel. It is a prerequisite to let some callbacks yield
on one side independently on the other one.
It is just informative, there is no warning and functions may still be
used. Howver it is recommended to use new functions. New functions are more
flexible and use a better naming pattern. In addition, the same names will
be used in the http_msg class to manipulate http messages from lua filters.
So for now, the offset is always the input data position and the length is
the input data length. But with the support for filters, from a filter
context, these values will be relative to the filter.
To make all processing clearer, the function _hlua_channel_dup() has been
updated and _hlua_channel_dupline(), _hlua_channel_insert() and
_hlua_channel_delete() have been added.
This patch is mandatory to allow the support of the filters written in lua.
MINOR: lua: Add a function to get a reference on a table in the stack
The hlua_checktable() function may now be used to create and return a
reference on a table in stack, given its position. This function ensures it
is really a table and throws an exception if not.
This patch is mandatory to allow the support of the filters written in lua.
BUG/MINOR: lua: Don't yield in channel.append() and channel.set()
Lua functions to set or append data to the input part of a channel must not
yield because new data may be received while the lua script is suspended. So
adding data to the input part in several passes is highly unpredicatble and
may be interleaved with received data.
Note that if necessary, it is still possible to suspend a lua action by
returning act.YIELD. This way the whole action will be reexecuted later
because of I/O events or a timer. Another solution is to call core.yield().
This bug affects all stable versions. So, it may be backported. But it is
probably not necessary because nobody notice it till now.
BUG/MINOR: lua: Yield in channel functions only if lua context can yield
When a script is executed, it is not always allowed to yield. Lua sample
fetches and converters cannot yield. For lua actions, it depends on the
context. When called from tcp content ruleset, an action may yield until the
expiration of the inspect-delay timeout. From http rulesets, yield is not
possible.
Thus, when channel functions (dup, get, append, send...) are called, instead
of yielding when it is not allowed and triggering an error, we just give
up. In this case, some functions do nothing (dup, append...), some others
just interrupt the in-progress job (send, forward...). But, because these
functions don't yield anymore when it is not allowed, the script regains the
control and can continue its execution.
This patch depends on "MINOR: lua: Add a flag on lua context to know the
yield capability at run time". Both may be backported in all stable
versions. However, because nobody notice this bug till now, it is probably
not necessary, excepted if someone ask for it.
MINOR: lua: Add a flag on lua context to know the yield capability at run time
When a script is executed, a flag is used to allow it to yield. An error is
returned if a lua function yield, explicitly or not. But there is no way to
get this capability in C functions. So there is no way to choose to yield or
not depending on this capability.
To fill this gap, the flag HLUA_NOYIELD is introduced and added on the lua
context if the current script execution is not authorized to yield. Macros
to set, clear and test this flags are also added.
This feature will be usefull to fix some bugs in lua actions execution.
BUG/MINOR: stream: Don't release a stream if FLT_END is still registered
When at least one filter is registered on a stream, the FLT_END analyzer is
called on both direction when all other analyzers have finished their
processing. During this step, filters may release any allocated elements if
necessary. So it is important to not skip it.
Unfortunately, if both stream interfaces are closed, it is possible to not
wait the end of this analyzer. It is possible to be in this situation if a
filter must wait and prevents the analyzer completion. To fix the bug, we
now wait FLT_END analyzer is no longer registered on both direction before
releasing the stream.
This patch may be backported as far as 1.7, but AFAIK, no filter is affected
by this bug. So the backport seems to be optional for now. In any case, it
should remain under observation for some weeks first.
BUG/MINOR: tcpcheck: Properly detect pending HTTP data in output buffer
In tcpcheck_eval_send(), the condition to detect there are still pending
data in the output buffer is buggy. Presence of raw data must be tested for
TCP connection only. But a condition on the connection was missing to be
sure it is not an HTX connection.
The formatting of the buffer_dump() output must be calculated using the
relative counter, not the absolute one, or everything will be broken if
the <from> variable is not a multiple of 16.
Amaury Denoyelle [Tue, 10 Aug 2021 14:24:36 +0000 (16:24 +0200)]
BUG/MEDIUM: server: support both check/agent-check on a dynamic instance
A static server is able to support simultaneously both health chech and
agent-check. Adjust the dynamic server CLI handlers to also support this
configuration.
This should not be backported, unless dynamic server checks are
backported.
Amaury Denoyelle [Tue, 10 Aug 2021 14:23:49 +0000 (16:23 +0200)]
BUG/MEDIUM: check: fix leak on agent-check purge
There is currently a leak on agent-check for dynamic servers. When
deleted, the check rules and vars are not liberated. This leak grows
each time a dynamic server with agent-check is deleted.
Replace the manual purge code by a free_check invocation which
centralizes all the details on check cleaning.
There is no leak for health check because in this case the proxy is the
owner of the check vars and rules.
This should not be backported, unless dynamic server checks are
backported.
Amaury Denoyelle [Tue, 10 Aug 2021 14:22:51 +0000 (16:22 +0200)]
BUG/MINOR: check: fix leak on add dynamic server with agent-check error
If an error occured during a dynamic server creation, free_check is used
to liberate a possible agent-check. However, this does not free
associated vars and rules associated as this is done on another function
named deinit_srv_agent_check.
To simplify the check free and avoid a leak, move free vars/rules in
free_check. This is valid because deinit_srv_agent_check also uses
free_check.
This operation is done only for an agent-check because for a health
check, the proxy instance is the owner of check vars/rules.
This should not be backported, unless dynamic server checks are
backported.
Amaury Denoyelle [Tue, 10 Aug 2021 14:21:55 +0000 (16:21 +0200)]
BUG/MINOR: check: do not reset check flags on purge
Do not reset check flags when setting CHK_ST_PURGE.
Currently, this change has no impact. However, it is semantically wrong
to clear important flags such as CHK_ST_AGENT on purge.
Furthermore, this change will become mandatoy for a future fix to
properly free agent checks on dynamic servers removal. For this, it will
be needed to differentiate health/agent-check on purge via CHK_ST_AGENT
to properly free agent checks.
This must not be backported unless dynamic servers checks are
backported.
BUG/MINOR: server: do not use refcount in free_server in stopping mode
Currently there is a leak at process shutdown with dynamic servers with
check/agent-check activated. Check purges are not executed on process
stopping, so the server is not liberated due to its refcount.
The solution is simply to ignore the refcount on process stopping mode
and free the server on the first free_server invocation.
This should not be backported, unless dynamic server checks are
backported. In this case, the following commit must be backported first. 7afa5c1843521ec3be7549592d2b38ccc9d68b73
MINOR: global: define MODE_STOPPING
BUG/MINOR: check: test if server is not null in purge
Test if server is not null before using free_server in the check purge
operation. Currently, the null server scenario should not occured as
purge is used with refcounted dynamic servers. However, this might not
be always the case if purge is use in the future in other cases; thus
the test is useful for extensibility.
No need to backport, unless dynamic server checks are backported.
This has been reported through a coverity report in github issue #1343.
Write a regtest to validate check support by dynamic servers. Three
differents servers are added on various configuration :
- server OK
- server DOWN
- agent-check
MEDIUM: server: implement agent check for dynamic servers
This commit is the counterpart for agent check of
"MEDIUM: server: implement check for dynamic servers".
The "agent-check" keyword is enabled for dynamic servers. The agent
check must manually be activated via "enable agent" CLI. This can
enable the dynamic server if the agent response is "ready" without an
explicit "enable server" CLI.
MEDIUM: server: implement check for dynamic servers
Implement check support for dynamic servers. The "check" keyword is now
enabled for dynamic servers. If used, the server check is initialized
and the check task started in the "add server" CLI handler. The check is
explicitely disabled and must be manually activated via "enable health"
CLI handler.
The dynamic server refcount is incremented if a check is configured. On
"delete server" handler, the check is purged, which decrements the
refcount.
MINOR: check: enable safe keywords for dynamic servers
Implement a collection of keywords deemed safe and useful to dynamic
servers. The list of the supported keywords is :
- addr
- check-proto
- check-send-proxy
- check-via-socks4
- rise
- fall
- fastinter
- downinter
- port
- agent-addr
- agent-inter
- agent-port
- agent-send
MEDIUM: check: implement check deletion for dynamic servers
Implement a mechanism to free a started check on runtime for dynamic
servers. A new function check_purge is created for this. The check task
will be marked for deletion and scheduled to properly close connection
elements and free the task/tasklet/buf_wait elements.
This function will be useful to delete a dynamic server wich checks.
MINOR: server: implement a refcount for dynamic servers
It is necessary to have a refcount mechanism on dynamic servers to be
able to enable check support. Indeed, when deleting a dynamic server
with check activated, the check will be asynchronously removed. This is
mandatory to properly free the check resources in a thread-safe manner.
The server instance must be kept alive for this.
MINOR: check: do not increment global maxsock at runtime
global maxsock is used to estimate a number of fd to reserve for
internal use, such as checks. It is incremented at startup with the info
from the config file.
Disable this incrementation in checks functions at runtime. First, it
currently serves no purpose to increment it after startup. Worse, it may
lead to out-of-bound accesse on the fdtab.
This will be useful to initiate checks for dynamic servers.
Remove static qualifier on init_srv_check, init_srv_agent_check and
start_check_task. These functions will be called in server.c for dynamic
servers with checks.
MINOR: check: allocate default check ruleset for every backends
Allocate default tcp ruleset for every backend without explicit rules
defined, even if no server in the backend use check. This change is
required to implement checks for dynamic servers.
This allocation is done on check_config_validity. It must absolutely be
called before check_proxy_tcpcheck (called via post proxy check) which
allocate the implicit tcp connect rule.
MINOR: server: unmark deprecated on enable health/agent cli
Remove the "DEPRECATED" marker on "enable/disable health/agent"
commands. Their purpose is to toggle the check/agent on a server.
These commands are still useful because their purpose is not covered by
the "set server" command. Most there was confusion with the commands
'set server health/agent', which in fact serves another goal.
Note that the indication "use 'set server' instead" has been added since
2016 on the commit 2c04eda8b58636ad2ae44e42b1f50f3b5a24a642
REORG: cli: move "{enable|disable} health" to server.c
BUG/MEDIUM: spoe: Fix policy to close applets when SPOE connections are queued
It is the second part of the fix that should solve fairness issues with the
connections management inside the SPOE filter. Indeed, in multithreaded
mode, when the SPOE detects there are some connections in queue on a server,
it closes existing connections by releasing SPOE applets. It is mandatory
when a maxconn is set because few connections on a thread may prenvent new
connections establishment.
The first attempt to fix this bug (9e647e5af "BUG/MEDIUM: spoe: Kill applets
if there are pending connections and nbthread > 1") introduced a bug. In
pipelining mode, SPOE applets might be closed while some frames are pending
for the ACK reply. To fix the bug, in the processing stage, if there are
some connections in queue, only truly idle applets may process pending
requests. In this case, only one request at a time is processed. And at the
end of the processing stage, only truly idle applets may be released. It is
an empirical workaround, but it should be good enough to solve contention
issues when a low maxconn is set.
This patch should partely fix the issue #1340. It must be backported as far
as 2.0.
BUG/MEDIUM: spoe: Create a SPOE applet if necessary when the last one is released
On a thread, when the last SPOE applet is released, if there are still
pending streams, a new one is created. Of course, HAproxy must not be
stopping. It is important to start a new applet in this case to not abort
in-progress jobs, especially when a maxconn is set. Because applets may be
closed to be fair with connections waiting for a free slot.
This patch should partely fix the issue #1340. It depends on the commit
"MINOR: spoe: Create a SPOE applet if necessary when the last one on a
thread is closed". Both must be backported as far as 2.0.
MINOR: spoe: Add a pointer on the filter config in the spoe_agent structure
There was no way to access the SPOE filter configuration from the agent
object. However it could be handy to have it. And in fact, this will be
required to fix a bug.
Willy Tarreau [Wed, 4 Aug 2021 17:35:13 +0000 (19:35 +0200)]
BUG/MINOR: server: update last_change on maint->ready transitions too
Nenad noticed that when leaving maintenance, the servers' last_change
field was not updated. This is visible in the Status column of the stats
page in front of the state, as the cumuled time spent in the current state
is wrong, it starts from the last transition (typically ready->maint). In
addition, the backend's state was not updated either, because the down
transition is performed by set_backend_down() which also emits a log, and
it is this function which was extended to update the backend's last_change,
but it's not called for down->up transitions so that was not done.
The most visible (and unpleasant) effect of this bug is that it affects
slowstart so such a server could immediately restart with a significant
load ratio.
This should likely be backported to all stable releases.
Willy Tarreau [Tue, 3 Aug 2021 07:04:32 +0000 (09:04 +0200)]
MAJOR: fd: get rid of the DWCAS when setting the running_mask
Right now we're using a DWCAS to atomically set the running_mask while
being constrained by the thread_mask. This DWCAS is annoying because we
may seriously need it later when adding support for thread groups, for
checking that the running_mask applies to the correct group.
It turns out that the DWCAS is not strictly necessary because we never
need it to set the thread_mask based on the running_mask, only the other
way around. And in fact, the running_mask is always cleared alone, and
the thread_mask is changed alone as well. The running_mask is only
relevant to indicate a takeover when the thread_mask matches it. Any
bit set in running and not present in thread_mask indicates a transition
in progress.
As such, it is possible to re-arrange this by using a regular CAS around a
consistency check between running_mask and thread_mask in fd_update_events
and by making a CAS on running_mask then an atomic store on the thread_mask
in fd_takeover(). The only other case is fd_delete() but that one already
sets the running_mask before clearing the thread_mask, which is compatible
with the consistency check above.
This change has happily survived 10 billion takeovers on a 16-thread
machine at 800k requests/s.
The fd-migration doc was updated to reflect this change.
Willy Tarreau [Tue, 3 Aug 2021 08:59:50 +0000 (10:59 +0200)]
MINOR: activity/fd: remove the dead_fd counter
This one is set whenever an FD is reported by a poller with a null owner,
regardless of the thread_mask. It has become totally meaningless because
it only indicates a migrated FD that was not yet reassigned to a thread,
but as soon as a thread uses it, the status will change to skip_fd. Thus
there is no reason to distinguish between the two, it adds more confusion
than it helps. Let's simply drop it.
BUG/MINOR: server: remove srv from px list on CLI 'add server' error
If an error occured during the CLI 'add server' handler, the newly
created server must be removed from the proxy list if already inserted.
Currently, this can happen on the extremely rare error during server id
generation if there is no id left.
The removal operation is not thread-safe, it must be conducted before
releasing the thread isolation.
This can be backported up to 2.4. Please note that dynamic server track
is not implemented in 2.4, so the release_server_track invocation must
be removed for the backport to prevent a compilation error.
Willy Tarreau [Wed, 4 Aug 2021 12:42:37 +0000 (14:42 +0200)]
MEDIUM: servers: make the server deletion code run under full thread isolation
In 2.4, runtime server deletion was brought by commit e558043e1 ("MINOR:
server: implement delete server cli command"). A comment remained in the
code about a theoretical race between the thread_isolate() call and another
thread being in the process of allocating memory before accessing the
server via a reference that was grabbed before the memory allocation,
since the thread_harmless_now()/thread_harmless_end() pair around mmap()
may have the effect of allowing cli_parse_delete_server() to proceed.
Now that the full thread isolation is available, let's update the code
to rely on this. Now it is guaranteed that competing threads will either
be in the poller or queued in front of thread_isolate_full().
This may be backported to 2.4 if any report of breakage suggests the bug
really exists, in which case the two following patches will also be
needed:
MINOR: threads: make thread_release() not wait for other ones to complete
MEDIUM: threads: add a stronger thread_isolate_full() call
Willy Tarreau [Wed, 4 Aug 2021 09:44:17 +0000 (11:44 +0200)]
MEDIUM: threads: add a stronger thread_isolate_full() call
The current principle of running under isolation was made to access
sensitive data while being certain that no other thread was using them
in parallel, without necessarily having to place locks everywhere. The
main use case are "show sess" and "show fd" which run over long chains
of pointers.
The thread_isolate() call relies on the "harmless" bit that indicates
for a given thread that it's not currently doing such sensitive things,
which is advertised using thread_harmless_now() and which ends usings
thread_harmless_end(), which also waits for possibly concurrent threads
to complete their work if they took this opportunity for starting
something tricky.
As some system calls were notoriously slow (e.g. mmap()), a bunch of
thread_harmless_now() / thread_harmless_end() were placed around them
to let waiting threads do their work while such other threads were not
able to modify memory contents.
But this is not sufficient for performing memory modifications. One such
example is the server deletion code. By modifying memory, it not only
requires that other threads are not playing with it, but are not either
in the process of touching it. The fact that a pool_alloc() or pool_free()
on some structure may call thread_harmless_now() and let another thread
start to release the same object's memory is not acceptable.
This patch introduces the concept of "idle threads". Threads entering
the polling loop are idle, as well as those that are waiting for all
others to become idle via the new function thread_isolate_full(). Once
thread_isolate_full() is granted, the thread is not idle anymore, and
it is released using thread_release() just like regular isolation. Its
users have to keep in mind that across this call nothing is granted as
another thread might have performed shared memory modifications. But
such users are extremely rare and are actually expecting this from their
peers as well.
Note that that in case of backport, this patch depends on previous patch:
MINOR: threads: make thread_release() not wait for other ones to complete
Willy Tarreau [Wed, 4 Aug 2021 09:22:07 +0000 (11:22 +0200)]
MINOR: threads: make thread_release() not wait for other ones to complete
The original intent of making thread_release() wait for other requesters to
proceed was more of a fairness trade, guaranteeing that a thread that was
granted an access to the CPU would be in turn giving back once its job is
done. But this is counter-productive as it forces such threads to spin
instead of going back to the poller, and it prevents us from implementing
multiple levels of guarantees, as a thread_release() call could spin
waiting for another requester to pass while that requester expects
stronger guarantees than the current thread may be able to offer.
Let's just remove that wait period and let the thread go back to the
poller, a-la "race to idle".
While in theory it could possibly slightly increase the perceived
latency of concurrent slow operations like "show fd" or "show sess",
it is not the case at all in tests, probably because the time needed
to reach the poller remains extremely low anyway.
Willy Tarreau [Wed, 4 Aug 2021 08:33:57 +0000 (10:33 +0200)]
CLEANUP: thread: fix fantaisist indentation of thread_harmless_till_end()
Probably due to a copy-paste, there were two indent levels in this function
since its introduction in 1.9 by commit 60b639ccb ("MEDIUM: hathreads:
implement a more flexible rendez-vous point"). Let's fix this.
BUG/MINOR: server: fix race on error path of 'add server' CLI if track
If an error occurs during a dynamic server creation with tracking, it
must be removed from the tracked list. This operation is not thread-safe
and thus must be conducted under the thread isolation.
Track support for dynamic servers has been introduced in this release.
This does not need to be backported.
Previous patch b5c0d65 ("MINOR: proxy: disabled takes a stopping and a
disabled state") allows us to set 2 states for a stopped or a disabled
proxy. With this patch we are now able to show the stats of all proxies
when the process is in a stopping states, not only when there is some
activity on a proxy.
Willy Tarreau [Sun, 1 Aug 2021 16:19:51 +0000 (18:19 +0200)]
[RELEASE] Released version 2.5-dev3
Released version 2.5-dev3 with the following main changes :
- BUG/MINOR: arg: free all args on make_arg_list()'s error path
- BUG/MINOR: cfgcond: revisit the condition freeing mechanism to avoid a leak
- MEDIUM: proxy: remove long-broken 'option http_proxy'
- CLEANUP: http_ana: Remove now unused label from http_process_request()
- MINOR: deinit: always deinit the init_mutex on failed initialization
- BUG/MEDIUM: cfgcond: limit recursion level in the condition expression parser
- BUG/MEDIUM: mworker: do not register an exit handler if exit is expected
- BUG/MINOR: mworker: do not export HAPROXY_MWORKER_REEXEC across programs
- BUILD/MINOR: memprof fix macOs build.
- BUG/MEDIUM: ssl_sample: fix segfault for srv samples on invalid request
- BUG/MINOR: stats: Add missing agent stats on servers
- BUG/MINOR: check: fix the condition to validate a port-less server
- BUILD: threads: fix pthread_mutex_unlock when !USE_THREAD
- BUG/MINOR: resolvers: Use a null-terminated string to lookup in servers tree
- MINOR: ssl: use __objt_* variant when retrieving counters
- BUG/MINOR: systemd: must check the configuration using -Ws
- BUG/MINOR: mux-h1: Obey dontlognull option for empty requests
- BUG/MINOR: mux-h2: Obey dontlognull option during the preface
- BUG/MINOR: mux-h1: Be sure to swap H1C to splice mode when rcv_pipe() is called
- BUG/MEDIUM: mux-h2: Handle remaining read0 cases on partial frames
- MINOR: proxy: rename PR_CAP_LUA to PR_CAP_INT
- MINOR: mworker: the mworker CLI proxy is internal
- MINOR: stats: don't output internal proxies (PR_CAP_INT)
- CLEANUP: mworker: use the proxy helper functions in mworker_cli_proxy_create()
- CLEANUP: mworker: PR_CAP already initialized with alloc_new_proxy()
- BUG/MINOR: connection: Add missing error labels to conn_err_code_str
- MINOR: connection: Add a connection error code sample fetch
- MINOR: ssl: Enable error fetches in case of handshake error
- MINOR: ssl: Add new ssl_fc_hsk_err sample fetch
- MINOR: ssl: Define a default https log format
- MEDIUM: connection: Add option to disable legacy error log
- REGTESTS: ssl: Add tests for the connection and SSL error fetches
- REGTESTS: ssl: ssl_errors.vtc does not work with old openssl version
- BUG/MEDIUM: connection: close a rare race between idle conn close and takeover
- BUG/MEDIUM: pollers: clear the sleeping bit after waking up, not before
- BUG/MINOR: select: fix excess number of dead/skip reported
- BUG/MINOR: poll: fix abnormally high skip_fd counter
- BUG/MINOR: pollers: always program an update for migrated FDs
- BUG/MINOR: fd: protect fd state harder against a concurrent takeover
- DOC: internals: document the FD takeover process
- MINOR: fd: update flags only once in fd_update_events()
- MINOR: poll/epoll: move detection of RDHUP support earlier
- REORG: fd: uninline fd_update_events()
- MEDIUM: fd: rely more on fd_update_events() to detect changes
- BUG/MINOR: freq_ctr: use stricter barriers between updates and readings
- MEDIUM: atomic: simplify the atomic load/store/exchange operations
- MEDIUM: atomic: relax the load/store barriers on x86_64
- BUILD: opentracing: fixed build when using pkg-config utility
Miroslav Zagorac [Thu, 29 Jul 2021 09:10:08 +0000 (11:10 +0200)]
BUILD: opentracing: fixed build when using pkg-config utility
In case the OpenTracing C Wrapper library was installed as part of the
system (ie in a directory that pkg-config considers part of the system),
HAProxy building was not possible because in that case pkg-config did
not set the value of the OT_CFLAGS variable in the addon Makefile.
MEDIUM: atomic: relax the load/store barriers on x86_64
The x86-tso model makes the load and store barriers unneeded for our
usage as long as they perform at least a compiler barrier: the CPU
will respect store ordering and store vs load ordering. It's thus
safe to remove the lfence and sfence which are normally needed only
to communicate with external devices. Let's keep the mfence though,
to make sure that reads of same memory location after writes report
the value from memory and not the one snooped from the write buffer
for too long.
An in-depth review of all use cases tends to indicate that this is
okay in the rest of the code. Some parts could be cleaned up to
use atomic stores and atomic loads instead of explicit barriers
though.
Doing this reliably increases the overall performance by about 2-2.5%
on a 8c-16t Xeon thanks to less frequent flushes (it's likely that the
biggest gain is in the MT lists which use them a lot, and that this
results in less cache line flushes).
MEDIUM: atomic: simplify the atomic load/store/exchange operations
The atomic_load/atomic_store/atomic_xchg operations were all forced to
__ATOMIC_SEQ_CST, which results in explicit store or even full barriers
even on x86-tso while we do not need them: we're not communicating with
external devices for example and are only interested in respecting the
proper ordering of loads and stores between each other.
These ones being rarely used, the emitted code on x86 remains almost the
same (barring a handful of locations). However they will allow to place
correct barriers at other places where atomics are accessed a bit lightly.
The patch is marked medium because we can never rule out the risk of some
bugs on more relaxed platforms due to the rest of the code.
BUG/MINOR: freq_ctr: use stricter barriers between updates and readings
update_freq_ctr_period() was using relaxed atomics without using barriers,
which usually works fine on x86 but not everywhere else. In addition, some
values were read without being enclosed by barriers, allowing the compiler
to possibly prefetch them a bit earlier. Finally, freq_ctr_total() was also
reading these without enough barriers. Let's make explicit use of atomic
loads and atomic stores to get rid of this situation. This required to
slightly rearrange the freq_ctr_total() loop, which could possibly slightly
improve performance under extreme contention by avoiding to reread all
fields.
A backport may be done to 2.4 if a problem is encountered, but last tests
on arm64 with LSE didn't show any issue so this can possibly stay as-is.
MEDIUM: fd: rely more on fd_update_events() to detect changes
This function already performs a number of checks prior to calling the
IOCB, and detects the change of thread (FD migration). Half of the
controls are still in each poller, and these pollers also maintain
activity counters for various cases.
Note that the unreliable test on thread_mask was removed so that only
the one performed by fd_set_running() is now used, since this one is
reliable.
Let's centralize all that fd-specific logic into the function and make
it return a status among:
FD_UPDT_DONE, // update done, nothing else to be done
FD_UPDT_DEAD, // FD was already dead, ignore it
FD_UPDT_CLOSED, // FD was closed
FD_UPDT_MIGRATED, // FD was migrated, ignore it now
Some pollers already used to call it last and have nothing to do after
it, regardless of the result. epoll has to delete the FD in case a
migration is detected. Overall this removes more code than it adds.
projects / thirdparty / haproxy.git / log
