]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
7 years agoBUG/MAJOR: connection: refine the situations where we don't send shutw()
Willy Tarreau [Fri, 22 Dec 2017 17:46:33 +0000 (18:46 +0100)] 
BUG/MAJOR: connection: refine the situations where we don't send shutw()

Since commit f9ce57e ("MEDIUM: connection: make conn_sock_shutw() aware
of lingering"), we refrain from performing the shutw() on the socket if
there is no lingering risk. But there is a problem with this in tunnel
and in TCP modes where a client is explicitly allowed to send a shutw
to the server, eventhough it it risky.

Not doing it creates this situation reported by Ricardo Fraile and
diagnosed by Christopher : a typical HTTP client (eg: curl) connecting
via the config below to an HTTP server would receive its response,
immediately close while the server remains in keep-alive mode. The
shutr() received by haproxy from the client is "propagated" to the
server side but not acted upon because fdtab[fd].linger_risk is set,
so we expect that the next close will immediately complete this
operation.

  listen proxy-tcp
    bind 127.0.0.1:8888
    mode tcp
    timeout connect 5s
    timeout server  10s
    timeout client  10s
    server server1 127.0.0.1:8000

But since the whole stream will not end until the server closes in
turn, the server doesn't close and haproxy expires on server timeout.
This problem has already struck by waking up an older bug and was
partially fixed with commit 8059351 ("BUG/MEDIUM: http: don't disable
lingering on requests with tunnelled responses") though it was not
enough.

The problem is that linger_risk is not suited here. In fact we need to
know whether or not it is desired to close normally or silently, and
whether or not a shutr() has already been received on this connection.

This is the approach this patch takes, and it solves the problem for
the various difficult modes (tcp, http-server-close, pretend-keepalive).

This fix needs to be backported to 1.8. Many thanks to Ricardo for
providing very detailed traces and configurations.

7 years agoBUG/MEDIUM: cache: don't cache the response on no-cache="set-cookie"
Willy Tarreau [Fri, 22 Dec 2017 17:03:04 +0000 (18:03 +0100)] 
BUG/MEDIUM: cache: don't cache the response on no-cache="set-cookie"

If the server mentions no-cache="set-cookie" in the response headers,
we must guarantee that any set-cookie field will not be stored. We
cannot edit the stored response on the fly to trim the set-cookie
header so we can refrain from storing a response containing such a
header. In theory we could use TX_SCK_PRESENT for this but this one
is only set when the cookie is being watched by the configuration.
Since these responses are not very frequent and often accompanied
with a set-cookie header, let's simply refrain from caching whenever
such directive is present.

This needs to be backported to 1.8.

7 years agoBUG/MEDIUM: cache: respect the request cache-control header
Willy Tarreau [Fri, 22 Dec 2017 16:47:35 +0000 (17:47 +0100)] 
BUG/MEDIUM: cache: respect the request cache-control header

Till now if a client emitted a request featureing a cache-control header,
this one was not respected and a stale object could still be delievered.r
 This patch ensures that :
  - cache-control: no-cache disables retrieval from the cache but does
    not prevent the newly fetched object from being stored ;
  - cache-control: no-store can safely retrieve from the cache but prevents
    from storing any fetched object
  - cache-control: max-age/max-stale/min-fresh act like no-cache
  - pragma: no-cache acts like cache-control: no-cache.

This needs to be backported to 1.8.

7 years agoBUG/MEDIUM: cache: replace old object on store
Willy Tarreau [Fri, 22 Dec 2017 16:42:46 +0000 (17:42 +0100)] 
BUG/MEDIUM: cache: replace old object on store

Currently the cache aborts a store operation if the object to store
already exists in the cache. This is used to avoid storing multiple
copies at the same time on concurrent accesses. It causes an issue
though, which is that existing unexpired objects cannot be updated.
This happens when any request criterion disables the retrieval from
the cache (eg: with max-age or any other cache-control condition).

For now, let's simply replace the previous existing entry by unlinking
it from the index. This could possibly be improved in the future if
needed.

This fix needs to be backported to 1.8.

7 years agoBUG/MEDIUM: cache: do not try to retrieve host-less requests from the cache
Willy Tarreau [Fri, 22 Dec 2017 15:32:43 +0000 (16:32 +0100)] 
BUG/MEDIUM: cache: do not try to retrieve host-less requests from the cache

All HTTP/1.1 requests the Host header share the same hash key 0 and
will be return the first cached object. Let's add the check on the call
to sha1_hosturi() to prevent this from happening.

This must be backported to 1.8.

7 years agoMINOR: http: add a function to check request's cache-control header field
Willy Tarreau [Fri, 22 Dec 2017 14:03:36 +0000 (15:03 +0100)] 
MINOR: http: add a function to check request's cache-control header field

The new function check_request_for_cacheability() is used to check if
a request may be served from the cache, and/or allows the response to
be stored into the cache. For this it checks the cache-control and
pragma header fields, and adjusts the existing TX_CACHEABLE and a new
TX_CACHE_IGNORE flags.

For now, just like its response side counterpart, it only checks the
first value of the header field. These functions should be reworked to
improve their parsers and validate all elements.

7 years agoBUG/MINOR: cache: do not force the TX_CACHEABLE flag before checking cacheability
Willy Tarreau [Thu, 21 Dec 2017 14:59:17 +0000 (15:59 +0100)] 
BUG/MINOR: cache: do not force the TX_CACHEABLE flag before checking cacheability

The cache used to set this flag before calling
check_response_for_cacheability() due to the way the flags were previously
set (too late), but this is a bad idea as it loses the information of the
implicit caching rules related to the method and the status code. Let's
only rely on what was determined during the request and response parsing
instead and not change it.

This fix must be backported to 1.8, and it requires that the following
patches are also merged :
 - MINOR: http: adjust the list of supposedly cacheable methods
 - MINOR: http: update the list of cacheable status codes as per RFC7231
 - MINOR: http: start to compute the transaction's cacheability from the request
 - BUG/MINOR: http: do not ignore cache-control: public

7 years agoBUG/MINOR: http: properly detect max-age=0 and s-maxage=0 in responses
Willy Tarreau [Fri, 22 Dec 2017 14:35:11 +0000 (15:35 +0100)] 
BUG/MINOR: http: properly detect max-age=0 and s-maxage=0 in responses

In 1.3.8, commit a15645d ("[MAJOR] completed the HTTP response processing.")
improved the response parser by taking care of the cache-control header
field. The parser is wrong because it is split in two parts, one checking
for elements containing an equal sign and the other one for those without.
The "max-age=0" and "s-maxage=0" tests were located at the wrong place and
thus have never matched. In practice the side effect was very minimal given
that this code used to be enabled only when checking if a cookie had the
risk of being cached or not. Recently in 1.8 it was also used to decide if
the response could be cached but in practice the cache takes care of these
values by itself so there is very limited impact.

This fix can be backported to all stable versions.

7 years agoBUG/MINOR: http: do not ignore cache-control: public
Willy Tarreau [Thu, 21 Dec 2017 15:08:09 +0000 (16:08 +0100)] 
BUG/MINOR: http: do not ignore cache-control: public

In check_response_for_cacheability(), we don't check the
cache-control flags if the response is already supposed not to be
cacheable. This was introduced very early when cache-control:public
was not checked, and it basically results in this last one not being
able to properly mark the response as cacheable if it uses a status
code which is non-cacheable by default. Till now the impact is very
limited as it doesn't check that cookies set on non-default status
codes are not cacheable, and it prevents the cache from caching such
responses.

Let's fix this by doing two things :
  - remove the test for !TX_CACHEABLE in the aforementionned function
  - however take care of 1xx status codes here (which used to be
    implicitly dealt with by the test above) and remove the explicit
    check for 101 in the caller

This fix must be backported to 1.8.

7 years agoMINOR: http: start to compute the transaction's cacheability from the request
Willy Tarreau [Thu, 21 Dec 2017 14:13:09 +0000 (15:13 +0100)] 
MINOR: http: start to compute the transaction's cacheability from the request

There has always been something odd with the way the cache-control flags
are checked. Since it was made for checking for the risk of leaking cookies
only, all the processing was done in the response. Because of this it is not
possible to reuse the transaction flags correctly for use with the cache.

This patch starts to change this by moving the method check in the request
so that we know very early whether the transaction is expected to be cacheable
and that this status evolves along with checked headers. For now it's not
enough to use from the cache yet but at least it makes the flag more
consistent along the transaction processing.

7 years agoMINOR: http: update the list of cacheable status codes as per RFC7231
Willy Tarreau [Thu, 21 Dec 2017 10:41:38 +0000 (11:41 +0100)] 
MINOR: http: update the list of cacheable status codes as per RFC7231

Since RFC2616, the following codes were added to the list of codes
cacheable by default : 204, 404, 405, 414, 501. For now this it only
checked by the checkcache option to detect cacheable cookies.

7 years agoMINOR: http: adjust the list of supposedly cacheable methods
Willy Tarreau [Thu, 21 Dec 2017 10:32:55 +0000 (11:32 +0100)] 
MINOR: http: adjust the list of supposedly cacheable methods

We used to have a rule inherited from RFC2616 saying that the POST
method was the only uncacheable one, but things have changed since
and RFC7231+7234 made it clear that in fact only GET/HEAD/OPTIONS/TRACE
are cacheable. Currently this rule is only used to detect cacheable
cookies.

7 years agoBUG/MEDIUM: lua: fix crash when using bogus mode in register_service()
Eric Salama [Thu, 21 Dec 2017 13:30:07 +0000 (14:30 +0100)] 
BUG/MEDIUM: lua: fix crash when using bogus mode in register_service()

When using an incorrect 'mode' as 2nd argument of core.register_service(),
HAProxy crashes while displaying the error message.

To be backported to 1.8, 1.7 and 1.6.

7 years agoBUG/MEDIUM: checks: a server passed in maint state was not forced down.
Emeric Brun [Thu, 21 Dec 2017 13:42:26 +0000 (14:42 +0100)] 
BUG/MEDIUM: checks: a server passed in maint state was not forced down.

Setting a server in maint mode, the required next_state was not set
before calling the 'lb_down' function and so the system state was never
commited.

This patch should be backported in 1.8

7 years agoBUG/MEDIUM: stream: don't consider abortonclose on muxes which close cleanly
Willy Tarreau [Wed, 20 Dec 2017 15:56:50 +0000 (16:56 +0100)] 
BUG/MEDIUM: stream: don't consider abortonclose on muxes which close cleanly

The H2 mux can cleanly report an error when a client closes, which is not
the case for the pass-through mux which only reports shutr. That was the
reason why "option abortonclose" was created since there was no way to
distinguish a clean shutdown after sending the request from an abort.

The problem is that in case of H2, the streams are always shut read after
the request is complete (when the END_STREAM flag is received), and that
when this lands on a backend configured with "option abortonclose", this
aborts the request. Disabling abortonclose is not always an option when
H1 and H2 have to coexist.

This patch makes use of the newly introduced mux capabilities reported
via the stream interface's SI_FL_CLEAN_ABRT indicating that the mux is
safe and that there is no need to turn a clean shutread into an abort.
This way abortonclose has no effect on requests initiated from an H2
mux.

This patch as well as these 3 previous ones need to be backported to
1.8 :
 - BUG/MINOR: h2: properly report a stream error on RST_STREAM
 - MINOR: mux: add flags to describe a mux's capabilities
 - MINOR: stream-int: set flag SI_FL_CLEAN_ABRT when mux supports clean aborts

7 years agoMINOR: stream-int: set flag SI_FL_CLEAN_ABRT when mux supports clean aborts
Willy Tarreau [Wed, 20 Dec 2017 15:31:43 +0000 (16:31 +0100)] 
MINOR: stream-int: set flag SI_FL_CLEAN_ABRT when mux supports clean aborts

By copying the info in the stream interface that the mux cleanly reports
aborts, we'll have the ability to check this flag wherever needed regardless
of the presence of a mux or not.

7 years agoMINOR: mux: add flags to describe a mux's capabilities
Willy Tarreau [Wed, 20 Dec 2017 15:14:44 +0000 (16:14 +0100)] 
MINOR: mux: add flags to describe a mux's capabilities

This new field will be used to describe certain properties of some
muxes. For now we only add MX_FL_CLEAN_ABRT to indicate that a mux
is able to unambiguously report aborts using CS_FL_ERROR contrary
to others who may only report it via a read0. This will be used to
improve handling of the abortonclose option with H2. Other flags
may come later to report multiplexing capabilities or not, support
of client/server sides etc.

7 years agoBUG/MINOR: h2: properly report a stream error on RST_STREAM
Willy Tarreau [Fri, 15 Dec 2017 10:56:29 +0000 (11:56 +0100)] 
BUG/MINOR: h2: properly report a stream error on RST_STREAM

We want to report such an error since H2 allows to differenciate
between an end of stream and an abort.

To be backported to 1.8.

7 years agoCONTRIB: halog: Fix compiler warnings in halog.c
Ryan O'Hara [Fri, 15 Dec 2017 16:21:39 +0000 (10:21 -0600)] 
CONTRIB: halog: Fix compiler warnings in halog.c

There were several unused variables in halog.c that each caused a
compiler warning [-Wunused-but-set-variable]. This patch simply
removes the declaration of said vairables and any instance where the
unused variable was assigned a value.

7 years agoCONTRIB: iprange: Fix compiler warning in iprange.c
Ryan O'Hara [Fri, 15 Dec 2017 16:21:29 +0000 (10:21 -0600)] 
CONTRIB: iprange: Fix compiler warning in iprange.c

The declaration of main() in iprange.c did not specify a type, causing
a compiler warning [-Wimplicit-int]. This patch simply declares main()
to be type 'int' and calls exit(0) at the end of the function.

7 years agoMINOR: spoe: add force-set-var option in spoe-agent configuration
Etienne Carriere [Thu, 14 Dec 2017 09:36:40 +0000 (09:36 +0000)] 
MINOR: spoe: add force-set-var option in spoe-agent configuration

For security reasons, the spoe filter was only able to change values of
existing variables. In specific cases (ex : with LUA code), the name of
variables are unknown at the configuration parsing phase.
The force-set-var option can be enabled to register all variables.

7 years agoMEDIUM: netscaler: add support for standard NetScaler CIP protocol
Bertrand Jacquin [Tue, 12 Dec 2017 01:17:23 +0000 (01:17 +0000)] 
MEDIUM: netscaler: add support for standard NetScaler CIP protocol

It looks like two version of the protocol exist as reported by
Andreas Mahnke. This patch add support for both legacy and standard CIP
protocol according to NetScaler specifications.

7 years agoMEDIUM: netscaler: do not analyze original IP packet size
Bertrand Jacquin [Wed, 13 Dec 2017 01:29:56 +0000 (01:29 +0000)] 
MEDIUM: netscaler: do not analyze original IP packet size

Original informations about the client are stored in the CIP encapsulated
IP header, hence there is no need to consider original IP packet length
to determine if data are missing. Instead this change detect missing
data if the remaining buffer is large enough to contain a minimal IP and
TCP header and if the buffer has as much data as CIP is telling.

7 years agoMINOR: netscaler: check in one-shot if buffer is large enough for IP and TCP header
Bertrand Jacquin [Wed, 13 Dec 2017 01:15:05 +0000 (01:15 +0000)] 
MINOR: netscaler: check in one-shot if buffer is large enough for IP and TCP header

There is minimal gain in checking first the IP header length and then
the TCP header length since we always want to capture information about
both protocols.

IPv4 length calculation was incorrect since IPv4 ip_len actually defines
the total length of IPv4 header and following data.

7 years agoBUG/MAJOR: netscaler: address truncated CIP header detection
Bertrand Jacquin [Wed, 13 Dec 2017 00:53:33 +0000 (00:53 +0000)] 
BUG/MAJOR: netscaler: address truncated CIP header detection

Buffer line is manually incremented in order to progress in the trash
buffer but calculation are made omitting this manual offset.

This leads to random packets being rejected with the following error:

  HTTP/1: Truncated NetScaler Client IP header received

Instead, once original IP header is found, use the IP header length
without considering the CIP encapsulation.

7 years agoBUG/MEDIUM: netscaler: use the appropriate IPv6 header size
Bertrand Jacquin [Wed, 13 Dec 2017 01:40:24 +0000 (01:40 +0000)] 
BUG/MEDIUM: netscaler: use the appropriate IPv6 header size

IPv6 header has a fixed size of 40 bytes, not 20.

7 years agoMINOR: netscaler: rename cip_len to clarify its uage
Bertrand Jacquin [Wed, 13 Dec 2017 01:23:39 +0000 (01:23 +0000)] 
MINOR: netscaler: rename cip_len to clarify its uage

cip_len was meant to be the length of the data encapsulated in the CIP
protocol, the size the IP and TCP header

7 years agoMINOR: netscaler: remove the use of cip_magic only used once
Bertrand Jacquin [Wed, 13 Dec 2017 01:07:12 +0000 (01:07 +0000)] 
MINOR: netscaler: remove the use of cip_magic only used once

7 years agoMINOR: netscaler: respect syntax
Bertrand Jacquin [Wed, 13 Dec 2017 00:58:51 +0000 (00:58 +0000)] 
MINOR: netscaler: respect syntax

As per doc/coding-style.txt

7 years agoDOC/MINOR: intro: typo, wording, formatting fixes
Davor Ocelic [Tue, 19 Dec 2017 22:30:39 +0000 (23:30 +0100)] 
DOC/MINOR: intro: typo, wording, formatting fixes

- Fix a couple typos
- Introduce a couple simple rewordings
- Eliminate > 80 column lines

Changes do not affect technical content and can be backported.

7 years agoBUG/MEDIUM: mworker: Set FD_CLOEXEC flag on log fd
Christopher Faulet [Tue, 19 Dec 2017 09:35:53 +0000 (10:35 +0100)] 
BUG/MEDIUM: mworker: Set FD_CLOEXEC flag on log fd

A log socket (UDP or UNIX) is opened by the master during its startup, when the
first log message is sent. So, to prevent FD leaks, we must ensure we correctly
close it during a reload. By setting FD_CLOEXEC bit on it, we are sure it will
be automatically closed it during a reload.

This patch must be backported in 1.8.

7 years agoMINOR: sample: rename the "len" converter to "length"
Willy Tarreau [Fri, 15 Dec 2017 06:13:48 +0000 (07:13 +0100)] 
MINOR: sample: rename the "len" converter to "length"

This converter was recently introduced by commit ed0d24e ("MINOR:
sample: add len converter").

As found by Cyril, it causes an issue in "http-request capture"
statements. The non-obvious problem is that an old syntax for sample
expressions and converters used to support a series of words, each
representing a converter. This used to be how the "stick" directives
were created initially. By having a converter called "len", a
statement such as "http-request capture foo len 10" considers "len"
as a converter and not as the capture length.

This obsolete syntax needs to be changed in 1.9 but it's too late
for other versions. It's worth noting that the same problem can
happen if converters are registered on the fly using Lua. Other
language keywords that currently have to be avoided in converters
include "id", "table", "if", "unless".

7 years agoBUG: MINOR: http: don't check http-request capture id when len is provided
Cyril Bonté [Thu, 14 Dec 2017 21:44:41 +0000 (22:44 +0100)] 
BUG: MINOR: http: don't check http-request capture id when len is provided

Randomly, haproxy could fail to start when a "http-request capture"
action is defined, without any change to the configuration. The issue
depends on the memory content, which may raise a fatal error like :
  unable to find capture id 'xxxx' referenced by http-request capture
rule

Commit fd608dd2 already prevents the condition to happen, but this one
should be included for completeness and to reclect the code on the
response side.

The issue was introduced recently by commit 29730ba5 and should only be
backported to haproxy 1.8.

7 years agoBUG: MAJOR: lb_map: server map calculation broken
Cyril Bonté [Thu, 14 Dec 2017 15:39:26 +0000 (16:39 +0100)] 
BUG: MAJOR: lb_map: server map calculation broken

Adrian Williams reported that several balancing methods were broken and
sent all requests to one backend. This is a regression in haproxy 1.8 where
the server score was not correctly recalculated.

This fix must be backported to the 1.8 branch.

7 years agoMINOR: sample: add len converter
Etienne Carriere [Wed, 13 Dec 2017 12:41:34 +0000 (13:41 +0100)] 
MINOR: sample: add len converter

Add len converter that returns the length of a string

7 years agoBUG/MINOR: stream-int: don't try to receive again after receiving an EOS
Willy Tarreau [Tue, 12 Dec 2017 08:58:40 +0000 (09:58 +0100)] 
BUG/MINOR: stream-int: don't try to receive again after receiving an EOS

When an end of stream has been reported, we should not try to receive again
as the mux layer might not be prepared to this and could report unexpected
errors.

This is more of a strengthening measure that follows the introduction of
conn_stream that came in 1.8. It's desired to backport this into 1.8 though
it's uncertain at this time whether it may have caused real issues.

7 years agoBUG/MEDIUM: h2: fix stream limit enforcement
Willy Tarreau [Thu, 14 Dec 2017 11:00:14 +0000 (12:00 +0100)] 
BUG/MEDIUM: h2: fix stream limit enforcement

Commit 4974561 ("BUG/MEDIUM: h2: enforce the per-connection stream limit")
implemented a stream limit enforcement on the connection but it was not
correctly done as it would count streams still known by the connection,
which includes the lingering ones that are already marked close. We need
to count only the non-closed ones, which this patch does. The effect is
that some streams are rejected a bit before the limit.

This fix needs to be backported to 1.8.

7 years agoBUG/MEDIUM: http: don't disable lingering on requests with tunnelled responses
Willy Tarreau [Thu, 14 Dec 2017 09:43:31 +0000 (10:43 +0100)] 
BUG/MEDIUM: http: don't disable lingering on requests with tunnelled responses

The HTTP forwarding engine needs to disable lingering on requests in
case the connection to the server has to be suddenly closed due to
http-server-close being used, so that we don't accumulate lethal
TIME_WAIT sockets on the outgoing side. A problem happens when the
server doesn't advertise a response size, because the response
message quickly goes through the MSG_DONE and MSG_TUNNEL states,
and once the client has transferred all of its data, it turns to
MSG_DONE and immediately sets NOLINGER and closes before the server
has a chance to respond. The problem is that this destroys some of
the pending DATA being uploaded, the server doesn't receive all of
them, detects an error and closes.

This early NOLINGER is inappropriate in this situation because it
happens before the response is transmitted. This state transition
to MSG_TUNNEL doesn't happen when the response size is known since
we stay in MSG_DATA (and related states) during all the transfer.

Given that the issue is only related to connections not advertising
a response length and that by definition these connections cannot be
reused, there's no need for NOLINGER when the response's transfer
length is not known, which can be verified when entering the CLOSED
state. That's what this patch does.

This fix needs to be backported to 1.8 and very likely to 1.7 and
older as it affects the very rare case where a client immediately
closes after the last uploaded byte (typically a script). However
given that the risk of occurrence in HTTP/1 is extremely low, it is
probably wise to wait before backporting it before 1.8.

7 years agoBUG/MEDIUM: h2: don't close after the first DATA frame on tunnelled responses
Willy Tarreau [Thu, 14 Dec 2017 09:55:21 +0000 (10:55 +0100)] 
BUG/MEDIUM: h2: don't close after the first DATA frame on tunnelled responses

Tunnelled responses are those without a content-length nor a chunked
encoding. They are specially dealt with in the current code but the
behaviour is not correct. The fact that the chunk size is left to zero
with a state artificially set to CHUNK_SIZE validates the test on
whether or not to set the end of stream flag. Thus the first DATA
frame always carries the ES flag and subsequent ones remain blocked.

This patch fixes it in two ways :
  - update h1m->curr_len to the size of the current buffer so that it
    is properly subtracted later to find the real end ;
  - don't set the state to CHUNK_SIZE when there's no content-length
    and instead set it to CHUNK_SIZE only when there's chunking.

This fix needs to be backported to 1.8.

7 years agoBUG/MEDIUM: h2: don't switch the state to HREM before end of DATA frame
Willy Tarreau [Mon, 11 Dec 2017 17:45:08 +0000 (18:45 +0100)] 
BUG/MEDIUM: h2: don't switch the state to HREM before end of DATA frame

We used to switch the stream's state to HREM when seeing and ES bit on
the DATA frame before actually being able to process that frame, possibly
resulting in the DATA frame being processed after the stream was seen as
half-closed and possibly being rejected. The state must not change before
the frame is really processed.

Also fixes a harmless typo in the flag name which should have DATA and
not HEADERS in its name (but all values are equal).

Must be backported to 1.8.

7 years agoMINOR: h2: don't demand that a DATA frame is complete before processing it
Willy Tarreau [Mon, 11 Dec 2017 17:36:37 +0000 (18:36 +0100)] 
MINOR: h2: don't demand that a DATA frame is complete before processing it

Since last commit it's not required that the DATA frames are complete anymore
so better start with what we have. Only the HEADERS frame requires this. This
may be backported as part of the upload fixes.

7 years agoBUG/MEDIUM: h2: support uploading partial DATA frames
Willy Tarreau [Mon, 11 Dec 2017 17:27:15 +0000 (18:27 +0100)] 
BUG/MEDIUM: h2: support uploading partial DATA frames

We currently have a problem with DATA frames when they don't fit into
the destination buffer. While it was imagined that in theory this never
happens, in practice it does when "option http-buffer-request" is set,
because the headers don't leave the target buffer before trying to read
so if the frame is full, there's never enough room.

This fix consists in reading what can be read from the frame and advancing
the input buffer. Once the contents left are only the padding, the frame
is completely processed. This also solves another problem we had which is
that it was possible to fill a request buffer beyond its reserve because
the <count> argument was not respected in h2_rcv_buf(). Thus it's possible
that some POST requests sent at once with a headers+body filling exactly a
buffer could result in "400 bad req" when trying to add headers.

This fix must be backported to 1.8.

7 years agoMINOR: h2: store the demux padding length in the h2c struct
Willy Tarreau [Mon, 11 Dec 2017 14:17:36 +0000 (15:17 +0100)] 
MINOR: h2: store the demux padding length in the h2c struct

We'll try to process partial frames and for this we need to know the
padding length. The first step requires to extract it during the parsing
and store it in the demux context in the connection. Till now it was only
processed at once.

7 years agoBUG/MEDIUM: h2: debug incoming traffic in h2_wake()
Willy Tarreau [Thu, 14 Dec 2017 09:34:52 +0000 (10:34 +0100)] 
BUG/MEDIUM: h2: debug incoming traffic in h2_wake()

Even after previous commit ("BUG/MEDIUM: h2: work around a connection
API limitation") there is still a problem with some requests. Sometimes
when polling for more request data while some pending data lies in the
buffer, there's no way to enter h2_recv() because the FD is not marked
ready for reading.

We need to slightly change the approach and make h2_recv() only receive
from the buffer and h2_wake() always attempt to demux if the demux is not
blocked.

However, if the connection is already being polled for reading, it will
not wake up from polling. For this reason we need to cheat and also
pretend a request for sending data, which ensures that as soon as any
direction may move, we can continue to demux. This shows that in the
long term we probably need a better way to resume an interrupted
operation at the mux level.

With this fix, no more hangups happen during uploads. Note that this
time the setup required to provoke the hangups was a bit complex :
  - client is "curl" running on local host, uploading 1.7 MB of
    data via haproxy
  - haproxy running on local host, forwarding to a remote server
    through a 100 Mbps only switch
  - timeouts disabled on haproxy
  - remote server made of thttpd executing a cgi reading request data
    through "dd bs=10" to slow down everything.

With such a setup, around 3-5% of the connections would hang up.

This fix needs to be backported to 1.8.

7 years agoBUG/MEDIUM: h2: work around a connection API limitation
Willy Tarreau [Tue, 12 Dec 2017 10:01:44 +0000 (11:01 +0100)] 
BUG/MEDIUM: h2: work around a connection API limitation

The connection API permits us to enable or disable receiving on a
connection. The underlying FD layer arranges this with the polling
and the fd cache. In practice, if receiving was allowed and an end
of buffer was reached, the FD is subscribed to the polling. If later
we want to process pending data from the buffer, we have to enable
receiving again, but since it's already enabled (in polled mode),
nothing happens and the pending data remain stuck until a new event
happens on the connection to wake the FD up. This is a limitation of
the internal connection API which is not very friendly to the new mux
architecture.

The visible effect is that certain uploads to slow servers experience
truncation on timeout on their last blocks because nothing new comes
from the connection to wake it up while it's being polled.

In order to work around this, there are two solutions :
  - either cheat on the connection so that conn_update_xprt_polling()
    always performs a call to fd_may_recv() after fd_want_recv(), that
    we can trigger from the mux by always calling conn_xprt_stop_recv()
    before conn_xprt_want_recv(), but that's a bit tricky and may have
    side effects on other parts (eg: SSL)

  - or we refrain from receiving in the mux as soon as we're busy on
    anything else, regardless of whether or not some room is available
    in the receive buffer.

This patch takes the second approach above. This way once we read some
data, as soon as we detect that we're stuck, we immediately stop receiving.
This ensures the event doesn't go into polled mode for this period and
that as soon as we're unstuck we can continue. In fact this guarantees
that we can only wait on one side of the mux for a given direction. A
future improvement of the connection layer should make it possible to
resume processing of an interrupted receive operation.

This fix must be backported to 1.8.

7 years agoBUG/MEDIUM: h2: enable recv polling whenever demuxing is possible
Willy Tarreau [Sun, 10 Dec 2017 21:17:57 +0000 (22:17 +0100)] 
BUG/MEDIUM: h2: enable recv polling whenever demuxing is possible

In order to allow demuxing when the dmux buffer is full, we need to
enable data receipt in multiple conditions. Since the conditions are a
bit complex, they have been delegated to a new function h2_recv_allowed()
which follows these rules :

  - if an error or a shutdown was detected on the connection and the buffer
    is empty, we must not attempt to receive
  - if the demux buf failed to be allocated, we must not try to receive and
    we know there is nothing pending
  - if the buffer is not full, we may attempt to receive
  - if no flag indicates a blocking condition, we may attempt to receive
  - otherwise must may not attempt

No more truncated payloads are detected in tests anymore, which seems to
indicate that the issue was worked around. A better connection API will
have to be created for new versions to make this stuff simpler and more
intuitive.

This fix needs to be backported to 1.8 along with the rest of the patches
related to CS_FL_RCV_MORE.

7 years agoBUG/MEDIUM: h2: automatically set CS_FL_RCV_MORE when the output buffer is full
Willy Tarreau [Sun, 10 Dec 2017 20:28:43 +0000 (21:28 +0100)] 
BUG/MEDIUM: h2: automatically set CS_FL_RCV_MORE when the output buffer is full

If we can't demux pending data due to a stream buffer full condition, we
now set CS_FL_RCV_MORE on the conn_stream so that the stream layer knows
it must call back as soon as possible to restart demuxing. Without this,
some uploaded payloads are truncated if the server does not consume them
fast enough and buffers fill up.

Note that this is still not enough to solve the problem, some changes are
required on the recv() and update_poll() paths to allow to restart reading
even with a buffer full condition.

This patch must be backported to 1.8.

7 years agoBUG/MEDIUM: stream-int: always set SI_FL_WAIT_ROOM on CS_FL_RCV_MORE
Willy Tarreau [Sun, 10 Dec 2017 20:19:33 +0000 (21:19 +0100)] 
BUG/MEDIUM: stream-int: always set SI_FL_WAIT_ROOM on CS_FL_RCV_MORE

When a stream interface tries to read data from a mux using rcv_buf(),
sometimes it sees 0 as the return value and concludes that there's no
more data while there are, resulting in the connection being polled for
more data and no new attempt being made at reading these pending data.

Now it will automatically check for flag CS_FL_RCV_MORE to know if the
mux really did not have anything available or was not able to provide
these data by lack of room in the destination buffer, and will set
SI_FL_WAIT_ROOM accordingly. This will ensure that once current data
lying in the buffer are forwarded to the other side, reading chk_rcv()
will be called to re-enable reading.

It's important to note that in practice it will rely on the mux's
update_poll() function to re-enable reading and that where the calls
are placed in the stream interface, it's not possible to perform a
new synchronous rcv_buf() call. Thus a corner case remains where the
mux cannot receive due to a full buffer or any similar condition, but
needs to be able to wake itself up to deliver pending data. This is a
limitation of the current connection/conn_stream API which will likely
need a new event subscription to at least call ->wake() asynchronously
(eg: mux->{kick,restart,touch,update} ?).

For now the affected mux (h2 only) will have to take care of the extra
logic to carefully enable polling to restart processing incoming data.

This patch relies on previous one (MINOR: conn_stream: add new flag
CS_FL_RCV_MORE to indicate pending data) and both must be backported to
1.8.

7 years agoMINOR: conn_stream: add new flag CS_FL_RCV_MORE to indicate pending data
Willy Tarreau [Sun, 10 Dec 2017 20:13:25 +0000 (21:13 +0100)] 
MINOR: conn_stream: add new flag CS_FL_RCV_MORE to indicate pending data

Due to the nature of multiplexed protocols, it will often happen that
some operations are only performed on full frames, preventing any partial
operation from being performed. HTTP/2 is one such example. The current
MUX API causes a problem here because the rcv_buf() function has no way
to let the stream layer know that some data could not be read due to a
lack of room in the buffer, but that data are definitely present. The
problem with this is that the stream layer might not know it needs to
call the function again after it has made some room. And if the frame
in the buffer is not followed by any other, nothing will move anymore.

This patch introduces a new conn_stream flag CS_FL_RCV_MORE whose purpose
is to indicate on the stream that more data than what was received are
already available for reading as soon as more room will be available in
the buffer.

This patch doesn't make use of this flag yet, it only declares it. It is
expected that other similar flags may come in the future, such as reports
of pending end of stream, errors or any such event that might save the
caller from having to poll, or simply let it know that it can take some
actions after having processed data.

7 years agoBUG/MEDIUM: lua/notification: memory leak
Thierry FOURNIER [Sun, 10 Dec 2017 16:10:57 +0000 (17:10 +0100)] 
BUG/MEDIUM: lua/notification: memory leak

The thread patches adds refcount for notifications. The notifications are
used with the Lua cosocket. These refcount free the notifications when
the session is cleared. In the Lua task case, it not have sessions, so
the nofications are never cleraed.

This patch adds a garbage collector for signals. The garbage collector
just clean the notifications for which the end point is disconnected.

This patch should be backported in 1.8

7 years agoDOC: notifications: add precisions about thread usage
Thierry FOURNIER [Sun, 10 Dec 2017 16:14:07 +0000 (17:14 +0100)] 
DOC: notifications: add precisions about thread usage

Precise the terms of use the notification functions.

7 years agoMINOR: systemd: remove comment about HAPROXY_STATS_SOCKET
Vincent Bernat [Sat, 9 Dec 2017 07:32:13 +0000 (08:32 +0100)] 
MINOR: systemd: remove comment about HAPROXY_STATS_SOCKET

This variable was used by the wrapper which was removed in
a6cfa9098e5a. The correct way to do seamless reload is now to enable
"expose-fd listeners" on the stat socket.

7 years agoBUG/MEDIUM: threads/vars: Fix deadlock in register_name
Christopher Faulet [Fri, 8 Dec 2017 08:17:39 +0000 (09:17 +0100)] 
BUG/MEDIUM: threads/vars: Fix deadlock in register_name

In register_name, before locking the var_names array, we check the variable name
validity. So if we try to register an invalid or empty name, we need to return
without unlocking it (because it was never locked).

This patch must be backported in 1.8.

7 years agoBUG/MEDIUM: email-alert: don't set server check status from a email-alert task
PiBa-NL [Wed, 6 Dec 2017 00:35:43 +0000 (01:35 +0100)] 
BUG/MEDIUM: email-alert: don't set server check status from a email-alert task

This avoids possible 100% cpu usage deadlock on a EMAIL_ALERTS_LOCK and
avoids sending lots of emails when 'option log-health-checks' is used.
It is avoided to change the server state and possibly queue a new email
while processing the email alert by setting check->status to
HCHK_STATUS_UNKNOWN which will exit the set_server_check_status(..) early.

This needs to be backported to 1.8.

7 years agoCONTRIB: halog: Add help text for -s switch in halog program
Aleksandar Lazic [Tue, 5 Dec 2017 00:35:21 +0000 (01:35 +0100)] 
CONTRIB: halog: Add help text for -s switch in halog program

It was not documented. May be backported to older releases.

7 years agoMINOR: mworker: Improve wording in `void mworker_wait()`
Tim Duesterhus [Tue, 5 Dec 2017 17:14:13 +0000 (18:14 +0100)] 
MINOR: mworker: Improve wording in `void mworker_wait()`

Replace "left" / "leaving" with "exit" / "exiting".

This should be backported to haproxy 1.8.

7 years agoMINOR: mworker: Update messages referencing exit-on-failure
Tim Duesterhus [Tue, 5 Dec 2017 17:14:12 +0000 (18:14 +0100)] 
MINOR: mworker: Update messages referencing exit-on-failure

Commit 4cfede87a313456fcbce7a185312460b4e1d05b7 removed
`exit-on-failure` in favor of `no-exit-on-failure`, but failed
to update references to the former in user facing messages.

This should be backported to haproxy 1.8.

7 years agoBUG/MEDIUM: h2: fix handling of end of stream again
Willy Tarreau [Thu, 7 Dec 2017 14:59:29 +0000 (15:59 +0100)] 
BUG/MEDIUM: h2: fix handling of end of stream again

Commit 9470d2c ("BUG/MINOR: h2: try to abort closed streams as
soon as possible") tried to address the situations where a stream
is closed by the client, but caused a side effect which is that in
some cases, a regularly closed stream reports an error to the stream
layer. The reason is that we purposely matched H2_SS_CLOSED in the
test for H2_SS_ERROR to report this so that we can check for RST,
but it accidently catches certain end of transfers as well. This
results in valid requests to report flags "CD" in the logs.

Instead, let's roll back to detecting H2_SS_ERROR and explicitly check
for a received RST. This way we can correctly abort transfers without
mistakenly reporting errors in normal situations.

This fix needs to be backported to 1.8 as the fix above was merged into
1.8.1.

7 years agoBUG/MEDIUM: peers: set NOLINGER on the outgoing stream interface
Willy Tarreau [Wed, 6 Dec 2017 16:39:53 +0000 (17:39 +0100)] 
BUG/MEDIUM: peers: set NOLINGER on the outgoing stream interface

Since peers were ported to an applet in 1.5, an issue appeared which
is that certain attempts to close an outgoing connection are a bit
"too nice". Specifically, protocol errors and stream timeouts result
in a clean shutdown to be sent, waiting for the other side to confirm.
This is particularly problematic in the case of timeouts since by
definition the other side will not confirm as it has disappeared.

As found by Fred, this issue was further emphasized in 1.8 by commit
f9ce57e ("MEDIUM: connection: make conn_sock_shutw() aware of
lingering") which causes clean shutdowns not to be sent if the fd is
marked as linger_risk, because now even a clean timeout will not be
sent on an idle peers session, and the other one will have nothing
to respond to.

The solution here is to set NOLINGER on the outgoing stream interface
to ensure we always close whenever we attempt a simple shutdown.

However it is important to keep in mind that this also underlines
some weaknesses of the shutr/shutw processing inside process_stream()
and that all this part needs to be reworked to clearly consider the
abort case, and to stop the confusion between linger_risk and NOLINGER.

This fix needs to be backported as far as 1.5 (all versions are affected).
However, during testing of the backport it was found that 1.5 never tries
to close the peers connection on timeout, so it suffers for another issue.

7 years agoBUG/MEDIUM: checks: a down server going to maint remains definitely stucked on down...
Emeric Brun [Wed, 6 Dec 2017 15:47:17 +0000 (16:47 +0100)] 
BUG/MEDIUM: checks: a down server going to maint remains definitely stucked on down state.

The new admin state was not correctly commited in this case.
Checks were fully disabled but the server was not marked in MAINT state.
It results with a server definitely stucked on the DOWN state.

This patch should be backported on haproxy 1.8

7 years agoBUG/MEDIUM: ssl engines: Fix async engines fds were not considered to fix fd limit...
Emeric Brun [Wed, 6 Dec 2017 12:51:49 +0000 (13:51 +0100)] 
BUG/MEDIUM: ssl engines: Fix async engines fds were not considered to fix fd limit automatically.

The number of async fd is computed considering the maxconn, the number
of sides using ssl and the number of engines using async mode.

This patch should be backported on haproxy 1.8

7 years agoBUG/MEDIUM: mworker: also close peers sockets in the master
Willy Tarreau [Tue, 5 Dec 2017 10:14:12 +0000 (11:14 +0100)] 
BUG/MEDIUM: mworker: also close peers sockets in the master

There's a nasty case related to signaling all processes via SIGUSR1.
Since the master process still holds the peers sockets, the old process
trying to connect to the new one to teach it its tables has a risk to
connect to the master instead, which will not do anything, causing the
old process to hang instead of quitting.

This patch ensures we correctly close the peers in the master process
on startup, just like it is done for proxies. Ultimately we would rather
have a complete list of listeners to avoid such issues. But that's a bit
trickier as it would require using unbind_all() and avoiding side effects
the master could cause to other processes (like unlinking unix sockets).

To be backported to 1.8.

7 years agoBUG/MINOR: ssl: support tune.ssl.cachesize 0 again
William Lallemand [Mon, 4 Dec 2017 17:46:39 +0000 (18:46 +0100)] 
BUG/MINOR: ssl: support tune.ssl.cachesize 0 again

Since the split of the shctx and the ssl cache, we lost the ability to
disable the cache with tune.ssl.cachesize 0.

Worst than that, when using this configuration, haproxy segfaults during
the configuration parsing.

Must be backported to 1.8.

7 years agoBUG/MAJOR: hpack: don't pretend large headers fit in empty table
Willy Tarreau [Mon, 4 Dec 2017 16:58:37 +0000 (17:58 +0100)] 
BUG/MAJOR: hpack: don't pretend large headers fit in empty table

In hpack_dht_make_room(), we try to fulfill this rule form RFC7541#4.4 :

 "It is not an error to attempt to add an entry that is larger than the
  maximum size; an attempt to add an entry larger than the maximum size
  causes the table to be emptied of all existing entries and results in
  an empty table."

Unfortunately it is not consistent with the way it's used in
hpack_dht_insert() as this last one will consider a success as a
confirmation it can copy the header into the table, and a failure as
an indexing error. This results in the two following issues :
  - if a client sends too large a header into an empty table, this
    header may overflow the table. Fortunately, most clients send
    small headers like :authority first, and never mark headers that
    don't fit into the table as indexable since it is counter-productive ;

  - if a client sends too large a header into a populated table, the
    operation fails after the table is totally flushed and the request
    is not processed.

This patch fixes the two issues at once :
  - a header not fitting into an empty table is always a sign that it
    will never fit ;
  - not fitting into the table is not an error

Thanks to Yves Lafon for reporting detailed traces demonstrating this
issue. This fix must be backported to 1.8.

7 years agoBUG/MINOR: action: Don't check http capture rules when no id is defined
Christopher Faulet [Mon, 4 Dec 2017 08:45:15 +0000 (09:45 +0100)] 
BUG/MINOR: action: Don't check http capture rules when no id is defined

This is a regression in the commit 29730ba5 ("MINOR: action: Add a functions to
check http capture rules"). We must check the capture id only when an id is
defined.

This patch must be backported in 1.8.

7 years agoBUG/MINOR: h2: use the H2_F_DATA_* macros for DATA frames
Willy Tarreau [Sun, 3 Dec 2017 20:06:59 +0000 (21:06 +0100)] 
BUG/MINOR: h2: use the H2_F_DATA_* macros for DATA frames

A typo resulted in H2_F_HEADERS_* being used there, but it's harmless
as they are equal. Better fix the confusion though.

Should be backported to 1.8.

7 years agoBUG/MEDIUM: h2: do not accept upper case letters in request header names
Willy Tarreau [Sun, 3 Dec 2017 19:28:13 +0000 (20:28 +0100)] 
BUG/MEDIUM: h2: do not accept upper case letters in request header names

This is explicitly forbidden by 7540#8.1.2, and may be used to bypass
some of the other filters, so they must be blocked early. It removes
another issue reported by h2spec.

To backport to 1.8.

7 years agoBUG/MEDIUM: h2: remove connection-specific headers from request
Willy Tarreau [Sun, 3 Dec 2017 19:15:34 +0000 (20:15 +0100)] 
BUG/MEDIUM: h2: remove connection-specific headers from request

h2spec rightfully outlines that we used not to reject these ones, and
they may cause trouble if presented, especially "upgrade".

Must be backported to 1.8.

7 years agoBUG/MINOR: h2: reject response pseudo-headers from requests
Willy Tarreau [Sun, 3 Dec 2017 19:13:54 +0000 (20:13 +0100)] 
BUG/MINOR: h2: reject response pseudo-headers from requests

At the moment there's only ":status". Let's block it early when parsing
the request. Otherwise it would be blocked by the HTTP/1 code anyway.
This silences another h2spec issue.

To backport to 1.8.

7 years agoBUG/MINOR: h2: properly check PRIORITY frames
Willy Tarreau [Sun, 3 Dec 2017 18:46:19 +0000 (19:46 +0100)] 
BUG/MINOR: h2: properly check PRIORITY frames

We don't use them right now but it's better to ensure they're properly
checked. This removes another 3 warnings in h2spec.

To backport to 1.8.

7 years agoBUG/MINOR: h2: reject incorrect stream dependencies on HEADERS frame
Willy Tarreau [Sun, 3 Dec 2017 18:24:50 +0000 (19:24 +0100)] 
BUG/MINOR: h2: reject incorrect stream dependencies on HEADERS frame

We currently don't use stream dependencies, but as reported by h2spec,
the spec requires that we reject streams that depend on themselves in
HEADERS frames.

To backport to 1.8.

7 years agoBUG/MINOR: h2: do not accept SETTINGS_ENABLE_PUSH other than 0 or 1
Willy Tarreau [Sun, 3 Dec 2017 18:02:28 +0000 (19:02 +0100)] 
BUG/MINOR: h2: do not accept SETTINGS_ENABLE_PUSH other than 0 or 1

We don't use yet it but for correctness, let's enforce the check.

To backport to 1.8.

7 years agoBUG/MEDIUM: h2: enforce the per-connection stream limit
Willy Tarreau [Sun, 3 Dec 2017 17:56:02 +0000 (18:56 +0100)] 
BUG/MEDIUM: h2: enforce the per-connection stream limit

h2spec reports that we unfortunately didn't enforce the per-connection
stream limit that we advertise. It's important to ensure it's never
crossed otherwise it's cheap for a client to create many streams. This
requires the addition of a stream count. The h2c struct could be cleaned
up a bit, just like the h2_detach() function where an "if" block doesn't
make sense anymore since it's always true.

To backport to 1.8.

7 years agoBUG/MINOR: h2: the TE header if present may only contain trailers
Willy Tarreau [Sun, 3 Dec 2017 17:41:31 +0000 (18:41 +0100)] 
BUG/MINOR: h2: the TE header if present may only contain trailers

h2spec reports this issue which has no side effect for now, but is
better cleared.

To backport to 1.8.

7 years agoBUG/MINOR: h2: fix a typo causing PING/ACK to be responded to
Willy Tarreau [Sun, 3 Dec 2017 17:15:56 +0000 (18:15 +0100)] 
BUG/MINOR: h2: fix a typo causing PING/ACK to be responded to

The ACK flag was tested on the frame type instead of the frame flag.

To backport to 1.8.

7 years agoBUG/MINOR: h2: ":path" must not be empty
Willy Tarreau [Sun, 3 Dec 2017 10:51:31 +0000 (11:51 +0100)] 
BUG/MINOR: h2: ":path" must not be empty

As reported by h2spec, the h2->h1 gateway doesn't verify that ":path"
is not empty. This is harmless since the H1 parser will reject such a
request, but better fix it anyway.

To backport to 1.8.

7 years agoBUG/MINOR: h2: try to abort closed streams as soon as possible
Willy Tarreau [Sun, 3 Dec 2017 09:42:59 +0000 (10:42 +0100)] 
BUG/MINOR: h2: try to abort closed streams as soon as possible

The purpose here is to be able to signal receipt of RST_STREAM to
streams when they start to provide a response so that the response
can be aborted ASAP. Given that RST_STREAM immediately switches the
stream to the CLOSED state, we must check for CLOSED in addition to
the existing ERROR check.

To be backported to 1.8.

7 years agoBUG/MINOR: h2: immediately close if receiving GOAWAY after the last stream
Willy Tarreau [Sun, 3 Dec 2017 09:27:47 +0000 (10:27 +0100)] 
BUG/MINOR: h2: immediately close if receiving GOAWAY after the last stream

The h2spec test suite reveals that a GOAWAY frame received after the
last stream doesn't cause an immediate close, because we count on the
last stream to quit to do so. By simply setting the last_sid to the
received value in case it was not set, we can ensure to properly close
an idle connection during h2_wake().

To be backported to 1.8.

7 years agoBUG/MAJOR: h2: correctly check the request length when building an H1 request
Willy Tarreau [Sun, 3 Dec 2017 08:44:50 +0000 (09:44 +0100)] 
BUG/MAJOR: h2: correctly check the request length when building an H1 request

Due to a typo in the request maximum length calculation, we count the
request path twice instead of counting it added to the method's length.
This has two effects, the first one being that a path cannot be larger
than half a buffer, and the second being that the method's length isn't
properly checked. Due to the way the temporary buffers are used internally,
it is quite difficult to meet this condition. In practice, the only
situation where this can cause a problem is when exactly one of either
the method or the path are compressed and the other ones is sent as a
literal.

Thanks to Yves Lafon for providing useful traces exhibiting this issue.

To be backported to 1.8.

7 years agoBUG/MINOR: hpack: dynamic table size updates are only allowed before headers
Willy Tarreau [Sun, 3 Dec 2017 17:09:21 +0000 (18:09 +0100)] 
BUG/MINOR: hpack: dynamic table size updates are only allowed before headers

h2spec reports that we used to support a dynamic table size update
anywhere in the header block but it's only allowed before other
headers (cf RFC7541#4.2.1). In practice we don't use these for now
since we only use literals in responses.

To backport to 1.8.

7 years agoBUG/MINOR: hpack: reject invalid header index
Willy Tarreau [Sun, 3 Dec 2017 11:12:17 +0000 (12:12 +0100)] 
BUG/MINOR: hpack: reject invalid header index

If the hpack decoder sees an invalid header index, it emits value
"### ERR ###" that was used during debugging instead of rejecting the
block. This is harmless, and was detected by h2spec.

To backport to 1.8.

7 years agoBUG/MINOR: hpack: must reject huffman literals padded with more than 7 bits
Willy Tarreau [Sun, 3 Dec 2017 11:00:36 +0000 (12:00 +0100)] 
BUG/MINOR: hpack: must reject huffman literals padded with more than 7 bits

h2spec reported that we didn't check that no more than 7 bits of padding
were left after decoding an huffman-encoded literal. This is harmless but
better fix it now.

To backport to 1.8.

7 years agoBUG/MINOR: hpack: fix debugging output of pseudo header names
Willy Tarreau [Sun, 3 Dec 2017 08:43:38 +0000 (09:43 +0100)] 
BUG/MINOR: hpack: fix debugging output of pseudo header names

When a pseudo header is used, name.ptr is NULL and we must replace it
with hpack_idx_to_name(). This only affects code built with DEBUG_HPACK.

To be backported to 1.8.

7 years agoBUG/MEDIUM: checks: Be sure we have a mux if we created a cs.
Olivier Houchard [Fri, 1 Dec 2017 21:04:05 +0000 (22:04 +0100)] 
BUG/MEDIUM: checks: Be sure we have a mux if we created a cs.

In connect_conn_chk(), there were one case we could return with a new
conn_stream created, but no mux attached. With no mux, cs_destroy() would
segfault. Fix that by setting the mux before we can fail.

This should be backported to 1.8.

7 years agoBUILD: Fix LDFLAGS vs. LIBS re linking order in various makefiles
Christian Ruppert [Thu, 30 Nov 2017 09:11:36 +0000 (10:11 +0100)] 
BUILD: Fix LDFLAGS vs. LIBS re linking order in various makefiles

Libraries should always be listed last. Should be backported to 1.8.

Signed-off-by: Christian Ruppert <idl0r@qasl.de>
7 years agoBUG/MAJOR: thread: Be sure to request a sync between threads only once at a time
Christopher Faulet [Sat, 2 Dec 2017 08:53:24 +0000 (09:53 +0100)] 
BUG/MAJOR: thread: Be sure to request a sync between threads only once at a time

The first thread requesting a synchronization is responsible to write in the
"sync" pipe to notify all others. But we must write only once in the pipe
between two synchronizations to have exactly one character in the pipe. It is
important because we only read 1 character in return when the last thread exits
from the sync-point.

Here there is a bug. If two threads request a synchronization, only the first
writes in the pipe. But, if the same thread requests several times a
synchronization before entering in the sync-point (because, for instance, it
detects many servers down), it writes as many as characters in the pipe. And
only one of them will be read. Repeating this bug many times will block HAProxy
on the write because the pipe is full.

To fix the bug, we just check if the current thread has already requested a
synchronization before trying to notify all others.

The patch must be backported in 1.8

7 years agoMINOR: threads: Fix pthread_setaffinity_np on FreeBSD.
Olivier Houchard [Fri, 1 Dec 2017 17:19:43 +0000 (18:19 +0100)] 
MINOR: threads: Fix pthread_setaffinity_np on FreeBSD.

As with the call to cpuset_setaffinity(), FreeBSD expects the argument to
pthread_setaffinity_np() to be a cpuset_t, not an unsigned long, so the call
was silently failing.

This should probably be backported to 1.8.

7 years agoBUG/MINOR: mworker: detach from tty when in daemon mode
PiBa-NL [Tue, 28 Nov 2017 22:26:08 +0000 (23:26 +0100)] 
BUG/MINOR: mworker: detach from tty when in daemon mode

This allows a calling script to show the first startup output and
know when to stop reading from stdout so haproxy can daemonize.

To be backpored to 1.8.

7 years agoBUG/MINOR: mworker: fix validity check for the pipe FDs
PiBa-NL [Tue, 28 Nov 2017 22:22:14 +0000 (23:22 +0100)] 
BUG/MINOR: mworker: fix validity check for the pipe FDs

Check if master-worker pipe getenv succeeded, also allow pipe fd 0 as
valid. On FreeBSD in quiet mode the stdin/stdout/stderr are closed
which lets the mworker_pipe to use fd 0 and fd 1. Additionally exit()
upon failure to create or get the master-worker pipe.

This needs to be backported to 1.8.

7 years agoMINOR: config: report when "monitor fail" rules are misplaced
Willy Tarreau [Fri, 1 Dec 2017 17:25:08 +0000 (18:25 +0100)] 
MINOR: config: report when "monitor fail" rules are misplaced

"monitor-uri" may rely on "monitor fail" rules, which are processed
very early, immediately after the HTTP request is parsed and before
any http rulesets. It's not reported by the config parser when this
ruleset is misplaces, causing some configurations not to work like
users would expect. Let's just add the warning for a misplaced rule.

7 years agoBUILD/MINOR: haproxy: compiling config cpu parsing handling when needed
David Carlier [Fri, 1 Dec 2017 09:14:02 +0000 (09:14 +0000)] 
BUILD/MINOR: haproxy: compiling config cpu parsing handling when needed

parse_cpu_set is only relevant where there is cpu affinity,
avoiding in the process compilation warning as well.

7 years agoBUG/MAJOR: thread/peers: fix deadlock on peers sync.
Emeric Brun [Fri, 1 Dec 2017 10:37:36 +0000 (11:37 +0100)] 
BUG/MAJOR: thread/peers: fix deadlock on peers sync.

Table lock was not released on an error path (if there is no
enough room to write table switch message).

[wt: needs to be backported to 1.8]

7 years agoBUG/MEDIUM: peers: fix some track counter rules dont register entries for sync.
Emeric Brun [Wed, 29 Nov 2017 15:15:07 +0000 (16:15 +0100)] 
BUG/MEDIUM: peers: fix some track counter rules dont register entries for sync.

This BUG was introduced with:
'MEDIUM: threads/stick-tables: handle multithreads on stick tables'

The API was reviewed to handle stick table entry updates
asynchronously and the caller must now call a 'stkable_touch_*'
function each time the content of an entry is modified to
register the entry to be synced.

There was missing call to stktable_touch_* resulting in
not propagated entries to remote peers (or local one during reload)

7 years agoBUG/MEDIUM: h2: don't report an error after parsing a 100-continue response
Willy Tarreau [Wed, 29 Nov 2017 14:41:32 +0000 (15:41 +0100)] 
BUG/MEDIUM: h2: don't report an error after parsing a 100-continue response

Yves Lafon reported a breakage with 100-continue. In fact the problem
is caused when an 1xx is the last response in the buffer (which commonly
is the case). We loop back immediately into the parser with what remains
of the input buffer (ie: nothing), while it is not expected to be called
with an empty response, so it fails.

Let's simply get back to the caller to decide whether or not more data
are expected to be sent.

This fix needs to be backported to 1.8.

7 years agoBUG/MEDIUM: threads/peers: decrement, not increment jobs on quitting
Willy Tarreau [Wed, 29 Nov 2017 13:49:30 +0000 (14:49 +0100)] 
BUG/MEDIUM: threads/peers: decrement, not increment jobs on quitting

Commit 8d8aa0d ("MEDIUM: threads/listeners: Make listeners thread-safe")
mistakenly placed HA_ATOMIC_ADD(job, 1) to replace a job--, so it maintains
the job count too high preventing the process from cleanly exiting on
reload.

This needs to be backported to 1.8.

7 years agoBUG/MINOR: ssl: CO_FL_EARLY_DATA removal is managed by stream
Emmanuel Hocdet [Mon, 27 Nov 2017 15:14:40 +0000 (16:14 +0100)] 
BUG/MINOR: ssl: CO_FL_EARLY_DATA removal is managed by stream

Manage BoringSSL early_data as it is with openssl 1.1.1.

7 years agoBUILD/MINOR: Makefile : enabling USE_CPU_AFFINITY
David Carlier [Wed, 29 Nov 2017 11:05:12 +0000 (11:05 +0000)] 
BUILD/MINOR: Makefile : enabling USE_CPU_AFFINITY

FreeBSD can handle cpuset matters just fine, we can hence enable it
by default as linux2628 TARGET.

7 years agoBUILD/MINOR: haproxy : FreeBSD/cpu affinity needs pthread_np header
David Carlier [Wed, 29 Nov 2017 11:02:32 +0000 (11:02 +0000)] 
BUILD/MINOR: haproxy : FreeBSD/cpu affinity needs pthread_np header

for pthread_*_np calls, pthread_np.h is needed under FreeBSD.

7 years agoBUG/MEDIUM: stream: fix session leak on applet-initiated connections
Willy Tarreau [Wed, 29 Nov 2017 13:05:38 +0000 (14:05 +0100)] 
BUG/MEDIUM: stream: fix session leak on applet-initiated connections

Commit 3e13cba ("MEDIUM: session: make use of the connection's destroy
callback") ensured that connections could be autonomous to destroy the
session they initiated, but it didn't take care of doing the same for
applets. Such applets are used for peers, Lua and SPOE outgoing
connections. In this case, once the stream ends, it closes everything
and nothing takes care of releasing the session. The problem is not
immediately obvious since the only visible effect is that older
processes will not quit on reload after having leaked one such session.

For now we check in stream_free() if the session's origin is the applet
we're releasing, and then free the session as well. Something more
uniform should probably be done once we manage to unify applets and
connections a bit more.

This fix needs to be backported to 1.8. Thanks to Emmanuel Hocdet for
reporting the problem.

7 years agoBUILD: checks: don't include server.h
Willy Tarreau [Wed, 29 Nov 2017 09:52:29 +0000 (10:52 +0100)] 
BUILD: checks: don't include server.h

server.h needs checks.h since it references the struct check, but depending
on the include order it will fail if check.h is included first due to this
one including server.h in turn while it doesn't need it.