Since commit 92919f7fd5 ("MEDIUM: h2: make the request parser rebuild
a complete URI") we make sure to rebuild a complete URI. Unfortunately
the test for an empty :path pseudo-header that is mandated by #8.1.2.3
appened to be performed on the URI before this patch, which is never
empty anymore after being rebuilt, causing h2spec to complain :
8. HTTP Message Exchanges
8.1. HTTP Request/Response Exchange
8.1.2. HTTP Header Fields
8.1.2.3. Request Pseudo-Header Fields
- 1: Sends a HEADERS frame with empty ":path" pseudo-header field
-> The endpoint MUST respond with a stream error of type PROTOCOL_ERROR.
Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
Connection closed
Actual: DATA Frame (length:0, flags:0x01, stream_id:1)
It's worth noting that this error doesn't trigger when calling h2spec
with a timeout as some scripts do, which explains why it wasn't detected
after the patch above.
This fixes one half of issue #471 and should be backported to 2.1.
The goal is to use the ckch to store data from PEM files or <payload> and
only for that. This patch adresses the ckch->ocsp_issuer case. It finds
issuers chain if no chain is present in the ckch in ssl_sock_put_ckch_into_ctx(),
filling the ocsp_issuer from the chain must be done after.
It changes the way '.issuer' is managed: it tries to load '.issuer' in
ckch->ocsp_issuer first and then look for the issuer in the chain later
(in ssl_sock_load_ocsp() ). "ssl-load-extra-files" without the "issuer"
parameter can negate extra '.issuer' file check.
The goal is to use the ckch to store data from a loaded PEM file or a
<payload> and only for that. This patch addresses the ckch->chain case.
Looking for the issuers chain, if no chain is present in the ckch, can
be done in ssl_sock_put_ckch_into_ctx(). This way it is possible to know
the origin of the certificate chain without an extra state.
This lock was only needed to protect the buffer_wq list, but now we have
the mt_list for this. This patch simply turns the buffer_wq list to an
mt_list and gets rid of the lock.
It's worth noting that the whole buffer_wait thing still looks totally
wrong especially in a threaded context: the wakeup_cb() callback is
called synchronously from any thread and may end up calling some
connection code that was not expected to run on a given thread. The
whole thing should probably be reworked to use tasklets instead and be
a bit more centralized.
Released version 2.2-dev3 with the following main changes :
- SCRIPTS: announce-release: place the send command in the mail's header
- SCRIPTS: announce-release: allow the user to force to overwrite old files
- SCRIPTS: backport: fix the master branch detection
- BUG/MINOR: http-act: Set stream error flag before returning an error
- BUG/MINOR: http-act: Fix bugs on error path during parsing of return actions
- BUG/MEDIUM: ssl/cli: 'commit ssl cert' wrong SSL_CTX init
- BUG/MEDIUM: tcp-rules: Fix track-sc* actions for L4/L5 TCP rules
- DOC: schematic of the SSL certificates architecture
- BUG/MAJOR: mux-h2: don't wake streams after connection was destroyed
- BUG/MINOR: unix: better catch situations where the unix socket path length is close to the limit
- BUILD: cirrus-ci: switch to "snap" images to unify openssl naming
- BUILD: cirrus-ci: workaround "pkg install" bug
- BUILD: cirrus-ci: add ERR=1 to freebsd builds
- BUG/MINOR: connection: correctly retry I/O on signals
- CLEANUP: mini-clist: simplify nested do { while(1) {} } while (0)
- BUILD: http_act: cast file sizes when reporting file size error
- BUG/MEDIUM: listener: only consider running threads when resuming listeners
- BUG/MINOR: listener: enforce all_threads_mask on bind_thread on init
- BUG/MINOR: tcp: avoid closing fd when socket failed in tcp_bind_listener
- MINOR: build: add aix72-gcc build TARGET and power{8,9} CPUs
- BUILD: travis-ci: no more allowed failures for openssl-1.0.2
- BUILD: travis-ci: harden builds, add ERR=1 (warning ought to be errors)
- BUILD: scripts/build-ssl.sh: use "uname" instead of ${TRAVIS_OS_NAME}
- BUG/MINOR: tcp: don't try to set defaultmss when value is negative
- SCRIPTS: make announce-release executable again
- BUG/MINOR: namespace: avoid closing fd when socket failed in my_socketat
- BUG/MEDIUM: muxes: Use the right argument when calling the destroy method.
- BUG/MINOR: mux-fcgi: Forbid special characters when matching PATH_INFO param
- CLEANUP: ssl: remove unused functions in openssl-compat.h
- MINOR: mux-fcgi: Make the capture of the path-info optional in pathinfo regex
- MINOR: tools: add is_idchar() to tell if a char may belong to an identifier
- MINOR: chunk: implement chunk_strncpy() to copy partial strings
- MINOR: sample/acl: use is_idchar() to locate the fetch/conv name
- MEDIUM: arg: make make_arg_list() stop after its own arguments
- MEDIUM: arg: copy parsed arguments into the trash instead of allocating them
- MEDIUM: arg: make make_arg_list() support quotes in arguments
- MINOR: sample: make sample_parse_expr() able to return an end pointer
- MEDIUM: log-format: make the LF parser aware of sample expressions' end
- BUG/MINOR: arg: report an error if an argument is larger than bufsize
- SCRIPTS: announce-release: use mutt -H instead of -i to include the draft
- BUILD: enable ERR=1 in github cygwin builds
- BUG/MINOR: arg: fix again incorrect argument length check
- MINOR: sample: regsub now supports backreferences
- BUG/MINOR: tools: also accept '+' as a valid character in an identifier
- MINOR: http-htx: Add a function to retrieve the headers size of an HTX message
- MINOR: filters: Forward data only if the last filter forwards something
- BUG/MINOR: filters: Count HTTP headers as filtered data but don't forward them
- BUG/MINOR: http-htx: Don't return error if authority is updated without changes
- BUG/MINOR: stream: Don't incr frontend cum_req counter when stream is closed
- BUG/MINOR: sample: exit regsub() in case of trash allocation error
- MINOR: ssl: add "issuers-chain-path" directive.
- REGTESTS: use "command -v" instead of "which"
- BUG/MINOR: http-ana: Matching on monitor-uri should be case-sensitive
- MINOR: http-ana: Match on the path if the monitor-uri starts by a /
- BUG/MINOR: ssl: Stop passing dynamic strings as format arguments
- BUG/MAJOR: http-ana: Always abort the request when a tarpit is triggered
- BUG/MINOR: mux: do not call conn_xprt_stop_recv() on buffer shortage
- MINOR: checks: do not call conn_xprt_stop_send() anymore
- CLEANUP: epoll: place the struct epoll_event in the stack
- MEDIUM: connection: remove the intermediary polling state from the connection
- MINOR: raw_sock: directly call fd_stop_send() and not conn_xprt_stop_send()
- MINOR: tcp/uxst/sockpair: use fd_want_send() instead of conn_xprt_want_send()
- MINOR: connection: remove the last calls to conn_xprt_{want,stop}_*
- CLEANUP: connection: remove the definitions of conn_xprt_{stop,want}_{send,recv}
- MINOR: connection: introduce a new receive flag: CO_RFL_READ_ONCE
- MINOR: mux-h1: pass CO_RFL_READ_ONCE to the lower layers when relevant
- MINOR: ist: add an iststop() function
- BUG/MINOR: http: http-request replace-path duplicates the query string
- CLEANUP: sample: use iststop instead of a for loop
- BUG/MEDIUM: shctx: make sure to keep all blocks aligned
- MINOR: compiler: move CPU capabilities definition from config.h and complete them
- BUG/MEDIUM: ebtree: don't set attribute packed without unaligned access support
- CLEANUP: http/h1: rely on HA_UNALIGNED_LE instead of checking for CPU families
- BUILD: fix recent build failure on unaligned archs
- MINOR: ssl: load the key from a dedicated file
- BUG/MINOR: ssl: load .key in a directory only after PEM
- MINOR: compiler: drop special cases of likely/unlikely for older compilers
- CLEANUP: conn: Do not pass a pointer to likely
- CLEANUP: net_helper: Do not negate the result of unlikely
- BUILD: remove obsolete support for -mregparm / USE_REGPARM
- CLEANUP: cfgparse: Fix type of second calloc() parameter
- BUILD: ssl: only pass unsigned chars to isspace()
- BUILD: general: always pass unsigned chars to is* functions
- BUG/MINOR: sample: fix the json converter's endian-sensitivity
- BUG/MEDIUM: ssl: fix several bad pointer aliases in a few sample fetch functions
- CLEANUP: fd: use a union in fd_rm_from_fd_list() to shut aliasing warnings
- CLEANUP: cache: use read_u32/write_u32 to access the cache entry's hash
- CLEANUP: stick-tables: use read_u32() to display a node's key
- CLEANUP: sample: use read_u64() in ipmask() to apply an IPv6 mask
- MINOR: pattern: fix all remaining strict aliasing issues
- CLEANUP: lua: fix aliasing issues in the address matching code
- CLEANUP: connection: use read_u32() instead of a cast in the netscaler parser
- BUILD: makefile: re-enable strict aliasing
- BUG/MINOR: connection: make sure to correctly tag local PROXY connections
- MINOR: compiler: add new alignment macros
- BUILD: ebtree: improve architecture-specific alignment
- MINOR: config: mark global.debug as deprecated
- BUILD: travis-ci: enable s390x builds
- MINOR: ssl/cli: 'show ssl cert' displays the chain
- MINOR: ssl/cli: 'show ssl cert'displays the issuer in the chain
- MINOR: ssl/cli: reorder 'show ssl cert' output
- CLEANUP: ssl: move issuer_chain tree and definition
- DOC: proxy-protocol: clarify IPv6 address representation in the spec
Daniel Barclay reported that the wording around "IPv6 addresses must be
indicated as series of 4 hex digits" is confusing and can be interpreted
two ways (only 4 digits or series of sets of 4 digits), so let's adjust
the wording to resolve this ambiguity.
Move the cert_issuer_tree outside the global_ssl structure since it's
not a configuration variable. And move the declaration of the
issuer_chain structure in types/ssl_sock.h
Reorder the 'show ssl cert' output so it's easier to see if the whole
chain is correct.
For a chain to be correct, an "Issuer" line must have the same
content as the next "Subject" line.
Example:
Subject: /C=FR/ST=Paris/O=HAProxy Test Certificate/CN=test.haproxy.local
Issuer: /C=FR/ST=Paris/O=HAProxy Test Intermediate CA 2/CN=ca2.haproxy.local
Chain Subject: /C=FR/ST=Paris/O=HAProxy Test Intermediate CA 2/CN=ca2.haproxy.local
Chain Issuer: /C=FR/ST=Paris/O=HAProxy Test Intermediate CA 1/CN=ca1.haproxy.local
Chain Subject: /C=FR/ST=Paris/O=HAProxy Test Intermediate CA 1/CN=ca1.haproxy.local
Chain Issuer: /C=FR/ST=Paris/O=HAProxy Test Root CA/CN=root.haproxy.local
For each certificate in the chain, displays the issuer, so it's easy to
know if the chain is right.
Also rename "Chain" to "Chain Subject".
Example:
Chain Subject: /C=FR/ST=Paris/O=HAProxy Test Intermediate CA 2/CN=ca2.haproxy.local
Chain Issuer: /C=FR/ST=Paris/O=HAProxy Test Intermediate CA 1/CN=ca1.haproxy.local
Chain Subject: /C=FR/ST=Paris/O=HAProxy Test Intermediate CA 1/CN=ca1.haproxy.local
Chain Issuer: /C=FR/ST=Paris/O=HAProxy Test Root CA/CN=root.haproxy.local
Display the subject of each certificate contained in the chain in the
output of "show ssl cert <filename>".
Each subjects are on a unique line prefixed by "Chain: "
Example:
Chain: /C=FR/ST=Paris/O=HAProxy Test Intermediate CA 2/CN=ca2.haproxy.local
Chain: /C=FR/ST=Paris/O=HAProxy Test Intermediate CA 1/CN=ca1.haproxy.local
This directive has never made any sense and has already caused trouble
by forcing the process to stay in foreground during the boot process.
Let's emit a warning mentioning it's deprecated and will be removed in
2.3.
Commit 2c315ee75e ("BUG/MEDIUM: ebtree: don't set attribute packed
without unaligned access support") addressed alignment issues in
ebtrees in a way that is not really optimal since it will leave holes
in eb32trees for example.
This fix is better in that it restores the packed attribute on ebnode
but enforces proper alignment on the carrying nodes where necessary.
This also has the benefit of closing holes wherever possible and to
align data to the minimally required size.
The only thing it cannot close is the 32-bit hole at the end of ebmbnode
due to the required 64-bit on certain archs but at least it guarantees
that the key correctly points to the end of the node and that there is
never a hole after it.
This is a better fix than the one above and should be backported to
branches where the one above will be backported.
This commit adds ALWAYS_ALIGN(), MAYBE_ALIGN() and ATOMIC_ALIGN() to
be placed as delimitors inside structures to force alignment to a
given size. These depend on the architecture's capabilities so that
it is possible to always align, align only on archs not supporting
unaligned accesses at all, or only on those not supporting them for
atomic accesses (e.g. before a lock).
As reported in issue #511, when sending an outgoing local connection
(e.g. health check) we must set the "local" tag and not a "proxy" tag.
The issue comes from historic support on v1 which required to steal the
address on the outgoing connection for such ones, creating confusion in
the v2 code which believes it sees the incoming connection.
In order not to risk to break existing setups which might rely on seeing
the LB's address in the connection's source field, let's just change the
connection type from proxy to local and keep the addresses. The protocol
spec states that for local, the addresses must be ignored anyway.
This problem has always existed, this can be backported as far as 1.5,
though it's probably not a good idea to change such setups, thus maybe
2.0 would be more reasonable.
For a very long time we've used to build without strict aliasing due to
very few places in the stick-tables code mostly, that initially we didn't
know how to deal with. The problem of doing this is that it encourages
to write possibly incorrect code such as the few SSL sample fetch functions
that were recently fixed.
All places causing aliasing errors on x86_64, i586, armv8, armv7 and
mips were fixed so it's about time to re-enable the warning hoping to
catch such errors early in the development cycle. As a bonus, this
removed about 5kB of code.
There were 8 strict aliasing warnings there due to the dereferences
casting to uint32_t of input and output. We can achieve the same using
two write_u64() and four read_u64() which do not cause this issue and
even let the compiler use 64-bit operations.
Enabling strict aliasing fails on the cache's hash which is a series of
20 bytes cast as u32. And in practice it could even fail on some archs
if the http_txn didn't guarantee the hash was properly aligned. Let's
use read_u32() to read the value and write_u32() to set it, this makes
sure the compiler emits the correct code to access these and knows about
the intentional aliasing.
Enabling strict aliasing fails in fd.c when using the double-word CAS,
let's get rid of the (void**)(void*)&cur_list junk and use a union
instead. This way the compiler knows they do alias.
Sample fetch functions ssl_x_sha1(), ssl_fc_npn(), ssl_fc_alpn(),
ssl_fc_session_id(), as well as the CLI's "show cert details" handler
used to dereference the output buffer's <data> field by casting it to
"unsigned *". But while doing this could work before 1.9, it broke
starting with commit 843b7cbe9d ("MEDIUM: chunks: make the chunk struct's
fields match the buffer struct") which merged chunks and buffers, causing
the <data> field to become a size_t. The impact is only on 64-bit platform
and depends on the endianness: on little endian, there should never be any
non-zero bits in the field as it is supposed to have been zeroed before the
call, so it shouldbe harmless; on big endian, the high part of the value
only is written instead of the lower one, often making the result appear
4 billion times larger, and making such values dropped everywhere due to
being larger than a buffer.
It seems that it would be wise to try to re-enable strict-aliasing to
catch such errors.
This must be backported till 1.9.
About every time there's a pointer cast in the code, there's a hidden
bug, and this one was no exception, as it passes the first octet of the
native representation of an integer as a single-character string, which
obviously only works on little endian machines. On big-endian machines,
something as simple as "str(foo),json" only returns zeroes.
This bug was introduced with the JSON converter in 1.6-dev1 by commit
317e1c4f1e ("MINOR: sample: add "json" converter"), the fix may be
backported to all stable branches.
The isalnum(), isalpha(), isdigit() etc functions from ctype.h are
supposed to take an int in argument which must either reflect an
unsigned char or EOF. In practice on some platforms they're implemented
as macros referencing an array, and when passed a char, they either cause
a warning "array subscript has type 'char'" when lucky, or cause random
segfaults when unlucky. It's quite unconvenient by the way since none of
them may return true for negative values. The recent introduction of
cygwin to the list of regularly tested build platforms revealed a lot
of breakage there due to the same issues again.
So this patch addresses the problem all over the code at once. It adds
unsigned char casts to every valid use case, and also drops the unneeded
double cast to int that was sometimes added on top of it.
It may be backported by dropping irrelevant changes if that helps better
support uncommon platforms. It's unlikely to fix bugs on platforms which
would already not emit any warning though.
A build failure on cygwin was reported on github actions here:
https://github.com/haproxy/haproxy/runs/466507874
It's caused by a signed char being passed to isspace(), and this one
being implemented as a macro instead of a function as the man page
suggests. It's the same issue that regularly pops up on Solaris. This
comes from commit 98263291cc which was merged in 1.8-dev1. A backport
is possible though not incredibly useful.
`curr_idle_thr` is of type `unsigned int`, not `int`. Fix this issue by
taking the size of the dereferenced `curr_idle_thr` array.
This issue was introduced when adding the `curr_idle_thr` struct member
in commit f131481a0a. This commit is first
tagged in 2.0-dev1 and marked for backport to 1.9.
This used to be a minor optimization on ix86 where registers are scarce
and the calling convention not very efficient, but this platform is not
relevant enough anymore to warrant all this dirt in the code for the sake
of saving 1 or 2% of performance. Modern platforms don't use this at all
since their calling convention already defaults to using several registers
so better get rid of this once for all.
This patch turns the double negation of 'not unlikely' into 'likely'
and then turns the negation of 'not smaller' into 'greater or equal'
in an attempt to improve readability of the condition.
[wt: this was not a bug but purposely written like this to improve code
generation on older compilers but not needed anymore as described here:
https://www.mail-archive.com/haproxy@formilux.org/msg36392.html ]
Move the `!` inside the likely and negate it to unlikely.
The previous version should not have caused issues, because it is converted
to a boolean / integral value before being passed to __builtin_expect(), but
it's certainly unusual.
[wt: this was not a bug but purposely written like this to improve code
generation on older compilers but not needed anymore as described here:
https://www.mail-archive.com/haproxy@formilux.org/msg36392.html ]
We used to special-case the likely()/unlikely() macros for a series of
early gcc 4.x compilers which used to produce very bad code when using
__builtin_expect(x,1), which basically used to build an integer (0 or 1)
from a condition then compare it to integer 1. This was already fixed in
5.x, but even now, looking at the code produced by various flavors of 4.x
this bad behavior couldn't be witnessed anymore. So let's consider it as
fixed by now, which will allow to get rid of some ugly tricks at some
specific places. A test on 4.7.4 shows that the code shrinks by about 3kB
now, thanks to some tests being inlined closer to the call place and the
unlikely case being moved to real functions. See the link below for more
background on this.
Link: https://www.mail-archive.com/haproxy@formilux.org/msg36392.html
Don't try to load a .key in a directory without loading its associated
certificate file.
This patch ignores the .key files when iterating over the files in a
directory.
Introduced by 4c5adbf ("MINOR: ssl: load the key from a dedicated
file").
For a certificate on a bind line, if the private key was not found in
the PEM file, look for a .key and load it.
This default behavior can be changed by using the ssl-load-extra-files
directive in the global section
This feature was mentionned in the issue #221.
Last commit 2c315ee ("BUG/MEDIUM: ebtree: don't set attribute packed
without unaligned access support") accidently enclosed the semicolon
in the ifdef so it will fail when HA_UNALIGNED is not set.
Now that we have flags indicating the CPU's capabilities, better use
them instead of missing some updates for new CPU families (ARMv8 was
missing there).
An alignment issue on Sparc64 with ebtrees was reported in early 2017
here https://www.mail-archive.com/haproxy@formilux.org/msg25937.html and
a similar one was finally reported in issue #512.
The problem has its roots in the fact that 64-bit keys will end up being
unaligned on such archs which do not support unaligned accesses. But on
most platforms supporting unaligned accesses, dealing with smaller nodes
results in better performance.
One of the possible problems caused by attribute packed there is that it
promotes a structure both to be unaligned and unpadded, which may come
with fun if some fields of the struct itself are accessed and such a node
is placed at an unaligned location. It's not a problem for regular unaligned
accesses but may become one for atomic operations such as the CAS on leaf_p
that's used in struct task. In practice we know that this struct is properly
aligned and is a very edge case so this patch adds comments there to remind
to be careful about it.
This patch depends on previous patch "MINOR: compiler: move CPU capabilities
definition from config.h and complete them" and could be backported to all
stable branches. It fixes issues #512 and #9.
These ones are irrelevant to the config but rather to the platform, and
as such are better placed in compiler.h.
Here we take the opportunity for declaring a few extra capabilities:
- HA_UNALIGNED : CPU supports unaligned accesses
- HA_UNALIGNED_LE : CPU supports unaligned accesses in little endian
- HA_UNALIGNED_FAST : CPU supports fast unaligned accesses
- HA_UNALIGNED_ATOMIC : CPU supports unaligned accesses in atomics
This will help remove a number of #ifdefs with arch-specific statements.
The blocksize and the extra field are not necessarily aligned on a
machine word. This can result in crashing an align-sensitive machine
when initializing the shctx area. Let's round both sizes up to a pointer
size to make this safe everywhere.
This fixes issue #512. This should be backported as far as 1.8.
In sample_fetch_path we can use iststop() instead of a for loop
to find the '?' and return the correct length. This requires commit
"MINOR: ist: add an iststop() function".
In http_action_replace_uri() we call http_get_path() in the case of
a replace-path rule. http_get_path() will return an ist pointing to
the start of the path, but uri.ptr + uri.len points to the end of the
uri. As as result, we are matching against a string containing the
query, which we append to the "path" later, effectively duplicating
the query string.
This patch uses the iststop() function introduced in "MINOR: ist: add
an iststop() function" to find the '?' character and update the ist
length when needed.
This fixes issue #510.
The bug was introduced by commit 262c3f1a ("MINOR: http: add a new
"replace-path" action"), which was backported to 2.1 and 2.0.
Add a function that finds a character in an ist and returns an
updated ist with the length of the portion of the original string
that doesn't contain the char.
Might be backported to 2.1
When we're in H1_MSG_RQBEFORE or H1_MSG_RPBEFORE, we know that the
first message is highly likely the only one and that it's pointless
to try to perform a second recvfrom() to complete a first partial
read. This is similar to what used to be done in the older I/O methods
with the CF_READ_DONTWAIT flag on the channel. So let's pass
CO_RFL_READ_ONCE to the transport layer during rcv_buf() in this case.
By doing so, in a test involving keep-alive connections with a non-null
client think time, we remove 20% of the recvfrom() calls, all of which
used to systematically fail. More precisely, we observe a drop from 5.0
recvfrom() per request with 60% failure to 4.0 per request with 50%
failure.
This flag is currently supported by raw_sock to perform a single recv()
attempt and avoid subscribing. Typically on the request and response
paths with keep-alive, with short messages we know that it's very likely
that the first message is enough.
This marks the end of the transition from the connection polling states
introduced in 1.5-dev12 and the subscriptions in that arrived in 1.9.
The socket layer can now safely use its FD while all upper layers rely
exclusively on subscriptions. These old functions were removed. Some may
deserve some renaming to improved clarty though. The single call to
conn_xprt_stop_both() was dropped in favor of conn_cond_update_polling()
which already does the same.
The last few calls to conn_xprt_{want,stop}_{recv,send} in the central
connection code were replaced with their strictly exact equivalent fd_*,
adding the call to conn_ctrl_ready() when it was missing.
Just like previous commit, we don't need to pass through the connection
layer anymore to enable polling during a connect(), we know the FD, so
let's simply call fd_want_send().
Now that we know that the connection layer is transparent for polling
changes, we have no reason for hiding behind conn_xprt_stop_send() and
can safely call fd_stop_send() on the FD once the buffer is empty.
