The test to remove trailers from chunked messages was inverted and is thus
ineffective. The flag for the requests was tested on client side and the flag
for the response was tested on server side. It should be the opposite.
This patch must be backported as far as 3.2.
When the EOH block is processed, before sending message headers, there is a
test to know if there is no payload. In case of a chunked message, a
null-chunk is emitted, except for bodyless response. For instance, a
response to a HEAD request has no payload at all and no null-chunk.
However, the test for bodyless responses is not correct. Only
H1S_F_BODYLESS_RESP flag is tested. But this flag can be set on server side
when we are processing the request. To fix the issue, the test was
adapted. The null-chunk is added if a message with no payload is chunked and
it is a request or a non-bodyless responses.
This patch must be backported to all stable version.
A typo in commit e51be30f78 ("BUG/MINOR: log: consider format expression
dependencies to decide when to log") made HRSHP appear twice (persistent
response) while the second one ought to be HRSHV (volatile response, e.g.
header values). This is harmless in practice since logs always wait for
at least headers.
This should be backported wherever the patch above was backported.
In h2_init(), if we have a failure while creating the h2c, and we
allocated shared_tx_bufs, don't forget to free it, otherwise we'll have
a memory leak.
This was introduced in 3.1 by commit a891534bfd ("MINOR: mux-h2: allocate
the array of shared rx bufs in the h2c"), so the fix should be backported
as far as 3.2.
When receiving a PRIORITY frame, when checking if the stream id provided
is ours, ignore bit 31, as it is the exclusive bit, and not part of the
stream id, whoever sends a PRIORITY frame with its own id and the
exclusive bit set will not be considered an error, as it should per the
RFC.
The impact is basically non-existent since we don't use PRIORITY frames,
it's only that we would ignore such an invalid frame instead of breaking
the connection.
The bug was introduced in 1.9 with commit 92153fccd3 ("BUG/MINOR: h2:
properly check PRIORITY frames") so the fix must be backported to all
versions.
Commit e67e36c9eb35eb1477ae0e425a660ee0c631cecd introduced
tune.h2.log-errors, that would let you pick if you wanted to know about
stream errors, connection errors, or no error.
However, a logic error made it so no error will be picked for any value
except for "none", in which case connection would be picked. Fix that by
just checking the strcmp() return value correctly.
This should be backported wherever e67e36c9eb35eb1477ae0e425a660ee0c631cecd
has been backported.
A malformed tcp option with an option length set to 0 can cause
an infinite loop on ip.fp converter.
The patch also forces the computation to use an unsigned char to
avoid a shift back during the parsing.
This fix should be backported on all versions including the ip.fp
converter.
In task_schedule(), before attempting to set the new task expiration
date, make sure it is not running by trying to set the TASK_RUNNING
flag, and waiting if it is already there. Having the flag set will
ensure that the task won't be running while we're modifying it.
There is a very rare race condition, where the expire would be set by
task_schedule(), then the running task might set it to something else,
and if it sets it to TICK_ETERNITY before task_schedule() calls
__task_queue(), then we will hit a BUG_ON() there.
This is very hard to reproduce, but has been reported a few times,
included in Github issue #3327, which should now be fixed.
This should be backported as far back as 2.8.
WIP: Make sure the task is not running before changing expire
The proxy error counter was not updated in h2c_frt_handle_headers() in
case of failure to decode a HEADERS frame. Make sure to keep it updated.
This can be backported to all stable versions.
Commit aab1a60977 ("BUG/MEDIUM: h2/htx: always fail on too large trailers")
explicitly returned an RST_STREAM on failure to decode some trailers, and
used the code H2_ERR_INTERNAL_ERROR. However there are multiple possible
causes for this failure to happen, and it turns out that it's much more
likely to be related to a protocol error than a decompression error. So
let's change this to PROTOCOL_ERROR, and count a protocol error on the
proxy and in the session.
This can be backported to all stable versions (with adjustments related
to these versions, maybe focusing on 3.2 max is reasonable).
This pointer was used during the appctx refactoring performed in 2.6. The
ctx union was still there and this pointer was used as the "shadow" of the
svcctx pointer used by most commands. In 2.7, the union was removed, making
the shadow pointer useless. Let's remove it now.
A new type of transaction was introduced for master-cli streams. So
SF_TXN_PCLI flag and functions to allocate and destroy PCLI transactions
were added.
In the stream structure, all pcli_* members were moved in the pcli
transaction and the txn union was updated accordingly.
When it was ambiguous, a test on the transaction type was performed. For
instance to destroy the transaciton.
To be able to deal with different types of transaction for a stream, new
stream flags was added to know the transaction type when allocated. For now
only HTTP transactions can be allocated, so only SF_TXN_HTTP was
introduced. The mask SF_TXN_MASK must be used to get the transaction type.
The transaction type is set when it is allocated and removed when it is
destroyed.
The HTTP transaction is moved in an union. For now, it is the only possible
transaction that can be allocated. But that will change. Thanks to this
commit and the next one, it will be possible to deal with different kind of
transactions for a stream.
This patch looks quite huge, but it is more or less a renaming of all
accesses to "txn" field by "txn.http".
The maximum size allowed for the payload pattern was increase up to 64 bytes
(65 bytes because of the trailing \0), to be able to use a sha256 of random
data for instance. It could be useful to prevent any data smuggling on the
payload.
Note that on the CLI, it could be possible to have only the buffer size as a
limit, because the command line is only consumed once all commands are
executed. The payload pattern is only a pointer in the buffer where the
command line was copied. However, for the master CLI, the data are streamed
to the worker, so we must keep a copy of he payload pattern. This is why we
must limit its size.
It is now possible to deal with too big payload to fit in a buffer, without
changing the buffer size. By default, a payload up to 128 KB can be
dynamically allocated. "tune.cli.max-payload-size" global parameter can be
used to change this value, with some caution for huge values.
For CLI command handler functions, there is no change at all. A pointer on
the payload is still passed as parameter. Internally, an area is allocated
for the payload only if it is too big.
The payload pattern used to detect the end of the payload is part from the
allocated area.
The payload is now saved as a buffer in the CLI context instead of a simple
pointer. It is mandatory to be able to reallocate the payload if it is too
big.
Instead of copying the payload pattern in the CLI context, we now only save
a pointer on this pattern. It is possible because the command line is copied
in the CLI context. Arguments are already handled this way when the command
is processed.
Detect shell parser errors in test LOG files right after vtest execution
and mark the run as failed when such errors are found.
This turns malformed feature cmd expressions from warning-like diagnostics
into hard failures, so broken test conditions are caught reliably.
The single-threaded build is currently broken in development since commit
0af603f46f ("MEDIUM: threads: change the default max-threads-per-group
value to 16") because it doesn't set the default for the non-threaded
build. Let's set it to 1.
No backport is needed.
Add an i686 job in order to run reg-tests on 32-bit architecture.
Use the i386 SSL and PCRE2 library provided by ubuntu.
VTest is still compiled in x86_64.
Commit 7d40b3134 ("MEDIUM: sched: do not run a same task multiple times
in series") required to slightly reorder a few fields in struct tasklet
and task in order to reuse an existing hole and keep tree nodes aligned.
The problem is that nice+expire were placed in struct task just before
rq, and that a 48-bit hole replaces them in struct tasklet on 64-bit
platforms, just before the struct list. However, on 32-bit platforms,
the hole is only 16-bit and preserves nice, but expire is overwritten
by the first pointer of the list element. This is not a problem for
real tasklets which do not use these fields, but it definitely is a
problem for tasks that are cast to tasklets in the run queues, because
the expire field can be overwritten when the task is woken up, and if
requeued as-is, it will expire at a completely random date.
This is what caused certain regtests to fail on i386 and 32-bit arm
machines.
This fix needs to be backported wherever the patch above was backported.
The bug has no effect on 64-bit platforms. The fix doesn't inflate
structs on 64-bit, but will raise struct tasklet from 40 to 44 bytes on
32-bit platforms.
Thanks to William for spotting the problem, bisecting it and providing
a working reproducer.
There was a test below the "release" label on conn->owner to decide
whether to kill the connection or not. But this test is not needed,
because:
- for frontends, it's always set so the test never matches
- for backends, it was NULL on the second stream once a request
was being reused from an idle pool, so it couldn't be used to
discriminate between connections. In practice, the goal was to
try to detect certain dead connections but all cases leading to
such connections are either already handled in the tests before
(which don't reach this label), or are handled by the other
conditions.
Thus, let's remove this confusing test.
Some places use conn->owner to retrieve the session. It's valid because
each time it is done, it's on the frontend, though it's not always 100%
obvious and sometimes requires deep code analysis. Let's clarify these
points and even rely on an intermediary variable to make it clearer. One
case where the owner couldn't differ from the session without being NULL
was also eliminated.
When installing a mux on the backend, unless we have a good reason for
keeping the session set in conn->owner, we must reset it. Having the
session there just hides potential bugs and prevents certain tests from
being properly done.
Now it is much clearer: conn->owner remains set to the session on
frontend connections, is set to the session when the connection is
private or assimilated private and belongs to the session list, or
is NULL.
When an idle connection is private or considered private, session_add_conn()
is called to add it to the list of connections owned by the session. But
in case of allocation failure, the session is not set, which results in
a long list of possible situations that are all corner cases which are
difficult to test (and debug).
This commit relies on the fact that it is already permitted to have
conn->owner pointing to a session even if the connection couldn't be
added to the session's list, as this was already the case in
conn_backend_get() when dealing with HOL_RISK. Also as seen in commit
3aab17bd566 added in 2.4, it is already possible to have conn->owner
set with the connection not being in a list, and only the list element
is checked for this.
This commit modifies session_add_conn() to always set conn->onwer, even
if the list element couldn't be allocated. This way it's possible to
always refer to conn->owner to find the session owning a private conn
even in case of failure to allocate an entry. This requires to change
the checks on conn->owner to a check of the list element to see if the
connection belongs to a session, the pre-assignment of sess to
conn->owner in conn_backend_get() is no longer needed, same for the
pre-assignment in http_wait_for_response(), and that's all.
The H1 mux remained unchanged because since it cannot multiplex, in
case it fails to allocate a pconn, it instantly kills the connection.
The bytes_in, bytes_out, {req,res}.bytes_{in,out} sample fetch functions
are marked as internal dependencies only. But that's not exact, they are
statistics. Request traffic (bytes_in, req.bytes*) is usable starting
from the request, while response traffic (bytes_out, res.bytes*) is usable
as soon as a response begins to be received, and all are valid till the
end of the transaction.
The impact is that the log-format below:
log-format "req.bytes_in=%[req.bytes_in] req.bytes_out=%[req.bytes_out] res.bytes_in=%[res.bytes_in] res.bytes_out=%[res.bytes_out]"
is emitted too early and only logs zeroes when uploading 1MB and
downloading 1MB:
req.bytes_in=0 req.bytes_out=0 res.bytes_in=15288 res.bytes_out=0
This patch marks the request stats RQFIN and the response stats RSFIN,
so that they're valid at any moment and the logs backend knows it must
wait for the latest moment to emit such a line. With this change, the
line above now correctly produces:
req.bytes_in=1000157 req.bytes_out=1000157 res.bytes_in=1048629 res.bytes_out=1048629
This should be backported as far as the latest LTS probably, along with
these 2 previous patches:
BUG/MINOR: log: consider format expression dependencies to decide when to log
MINOR: sample: make RQ/RS stats available everywhere
Sample fetch functions working on the request/response stats were marked
as being only compatible with the log phase. This is a mistake because
by definitions, stats can be consulted anywhere from the moment they
start to appear. It's only that they are valid as far as the logs. At the
moment, no sample fetch function depends on RQFIN, and only res.timer.data
depends on RSFIN. But this will be needed to relax certain sample fetch
functions (and will need to be backported along with a few other patches).
Log-format properly takes into account the LW_* flags set by the log
aliases, however its consideration for the sample fetch expressions is
very minimalistic (HTTP y/n). It poses a problem because logging some
statistics doesn't work unless some log aliases are involved to force
the log to wait till the end.
Before this change, the following log-format:
log-format "res.timer.data=%[res.timer.data]"
would log "res.timer.data=0" regardless of the time taken to transfer
data, and the log would be emitted instantly. However, this line:
log-format "res.timer.data=%[res.timer.data] %B"
would properly log the time taken to transfer the data because %B which
carries the log flag LW_BYTES forces the log to wait till the end.
This patch makes sure that anything requiring response (headers or body)
waits for at least the response, and that anything requiring response body
or end of transfer (req/res) waits till the end (LW_BYTES). Thanks to
this, the log above is now correct even without the "%B" hack.
This should be backported at least till the latest LTS.
The ACME Profiles extension (draft-ietf-acme-profiles) allows a client
to request a specific certificate profile by including a "profile" field
in the newOrder request. This lets the CA select the appropriate
certificate issuance policy (e.g. "classic", "shortlived") for a given
order.
A new "profile" keyword is added to the acme section. When set, its
value is included in the newOrder JSON payload sent to the CA.
pcre-devel (PCRE1) was removed in bded31dd3b but the make flags
were not updated to match; switch to USE_PCRE2/USE_PCRE2_JIT.
32bits job was also lacking pcre2-devel.
The target address type has been added to checks in commit
d759e60a3292f425aee66384e87ae227ce191c08, but as part of that address
type is the "alt_proto" field, that was not properly set for dynamic
servers, That could lead to checks not working for any protocol that use
a non-zero alt_proto, such as QUIC. So set it properly.
gcc flags aead_tag_trash as potentially NULL at the chunk_memcpy call
inside the (!dec && gcm) block, because it cannot correlate the
condition with the allocation that only happens in that same branch. Add
an explicit NULL check to silence the warning.
This was caught by cross-zoo.yml:
In file included from include/haproxy/connection.h:28,
from src/ssl_sample.c:27:
In function ‘b_orig’,
inlined from ‘sample_conv_aes’ at src/ssl_sample.c:540:23:
include/haproxy/buf.h:80:17: error: potential null pointer dereference [-Werror=null-dereference]
80 | return b->area;
| ~^~~~~~
In function ‘b_data’,
inlined from ‘sample_conv_aes’ at src/ssl_sample.c:540:3:
include/haproxy/buf.h💯17: error: potential null pointer dereference [-Werror=null-dereference]
100 | return b->data;
| ~^~~~~~
When trying to read QMux transport parameters frame, the record length
is checked to ensure it is not bigger than the buffer size. The
objective is to detect as soon as possible when receiving data that
cannot be handled and to close the connection.
In fact, this check is not accurate, as it did not take into account the
size of the Record length field itself. This patch fixes the comparison
by substracting with the size of the decoded varint.
No need to backport.
This patch is related to the issue reported on the previous issue
related to QMux record length parsing.
QCC rx.rlen is used to store the decoded record length. Convert it into
a plain 64bits integer instead of a size_t. This ensures it is
sufficient to decode record length, even with an increase of
max_record_length value (not currently implemented).
This should fix github build issue #3334 for 32bits architecture.
No need to backport.
QMux record lengths are encoded as a QUIC varint. Thus in theory, it
requires a 64bits integer to be able to read the whole value. In
practice, if the record is bigger than bufsize, read operation cannot be
completed and an error must be reported.
This patch fixes record length decoding both in xprt_qstrm layer, which
is now performed in two steps. The value is first read in a 64bits
integer instead of a size_t whose size is dependent on the architecture.
Result is then checked against bufsize and if inferior stored in the
previously used variable (xprt ctx rxrlen member).
This should partially fix build issue reported on github #3334.
No need to backport.
