Re: [PATCH] BUG/MINOR: Error out when a `server` has an `AF_UNSPEC` address
On Mon, Jan 04, 2021 at 12:58:25AM +0100, Tim Duesterhus wrote:
> I am adding Thayne as CC as it was your commit that uncovered the issue and
> the
> crash happened in a function you wrote. Maybe you might want to add some
> additional checks somewhere?
Oops, my bad. I actually meant to put a NULL check in there, but must have
forgotten.
On 1/5/21 3:37 AM, Willy Tarreau wrote:
> Unfortunately we cannot do that or it destroys usage of the DNS or
> even any runtime address assignment over the CLI. For example if you
> write:
>
> server foo foo init-addr none
>
> I guess it won't work anymore since that's placed just after the address
> parsing. This means however that there probably is something more
> problematic in the referencing of address-less servers. Either servers
> are added/updated each time their address changes (to follow DNS) and
> then we can simply skip their registration when they're address-less.
> Or the servers are only registered at boot time, and the synchronization
> will fail after any address update. What do you think, Thayne ?
I'm partial to skipping registration when they are address-less.
Here's a patch that I think should fix this:
-- >8 --
Subject: [PATCH] Fix a segfault in srv_set_addr_desc
Check to make sure the address key is non-null before using it for
comparison or inserting it into the tree.
---
src/server.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/server.c b/src/server.c
index 50c6da131..d1aa51dc8 100644
--- a/src/server.c
+++ b/src/server.c
@@ -204,7 +204,7 @@ static void srv_set_addr_desc(struct server *s)
key = sa2str(>addr, s->svc_port, s->flags & SRV_F_MAPPORTS);
if (s->addr_node.key) {
- if (strcmp(key, s->addr_node.key) == 0) {
+ if (key && strcmp(key, s->addr_node.key) == 0) {
free(key);
return;
}
@@ -218,9 +218,11 @@ static void srv_set_addr_desc(struct server *s)
s->addr_node.key = key;
- HA_RWLOCK_WRLOCK(PROXY_LOCK, >lock);
- ebis_insert(>used_server_addr, >addr_node);
- HA_RWLOCK_WRUNLOCK(PROXY_LOCK, >lock);
+ if (s->addr_node.key) {
+ HA_RWLOCK_WRLOCK(PROXY_LOCK, >lock);
+ ebis_insert(>used_server_addr, >addr_node);
+ HA_RWLOCK_WRUNLOCK(PROXY_LOCK, >lock);
+ }
}
/*
--
2.30.0
[PR] Fixed null pointer dereference in srv_cleanup_connections()
Dear list! Author: Peter Skarpetis Number of patches: 1 This is an automated relay of the Github pull request: Fixed null pointer dereference in srv_cleanup_connections() Patch title(s): Fixed null pointer dereference in srv_cleanup_connections() Link: https://github.com/haproxy/haproxy/pull/1031 Edit locally: wget https://github.com/haproxy/haproxy/pull/1031.patch && vi 1031.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/1031.patch | git am - Description: haproxy_srv_cleanup_connections_crash.cfg causes a null pointer dereference in srv_cleanup_connections . Configuration file works in 2.0.19 branch but crashes in all subsequent versions including the dev branch. I did not track down the cause, I just added the null pointer check to stop the crashing. Crash can be reproduced with the following command: ./haproxy -c -f haproxy_srv_cleanup_connections_crash.cfg haproxy_srv_cleanup_connections_crash.cfg can be grabbed from the gist below: https://gist.github.com/peterska/769f41562f6b045df59df 2294b2c20f0#file-haproxy_srv_cleanup_connections_crash-cfg configuration file has been edited enough to cause the crash while removing all references to certificates and CA authorities. It is not a production config file. Instructions: This github pull request will be closed automatically; patch should be reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is invited to comment, even the patch's author. Please keep the author and list CCed in replies. Please note that in absence of any response this pull request will be lost.
stable-bot: Bugfixes waiting for a release 2.3 (23), 2.2 (17), 2.1 (26), 2.0 (24)
Hi, This is a friendly bot that watches fixes pending for the next haproxy-stable release! One such e-mail is sent periodically once patches are waiting in the last maintenance branch, and an ideal release date is computed based on the severity of these fixes and their merge date. Responses to this mail must be sent to the mailing list. Last release 2.3.2 was issued on 2020-11-28. There are currently 23 patches in the queue cut down this way: - 2 MAJOR, first one merged on 2020-12-14 - 6 MEDIUM, first one merged on 2020-12-14 - 15 MINOR, first one merged on 2020-12-14 Thus the computed ideal release date for 2.3.3 would be 2020-12-28, which was one week ago. Last release 2.2.6 was issued on 2020-11-30. There are currently 17 patches in the queue cut down this way: - 2 MAJOR, first one merged on 2020-12-14 - 3 MEDIUM, first one merged on 2020-12-14 - 12 MINOR, first one merged on 2020-12-14 Thus the computed ideal release date for 2.2.7 would be 2020-12-28, which was one week ago. Last release 2.1.10 was issued on 2020-11-05. There are currently 26 patches in the queue cut down this way: - 4 MAJOR, first one merged on 2020-11-13 - 5 MEDIUM, first one merged on 2020-11-13 - 17 MINOR, first one merged on 2020-11-06 Thus the computed ideal release date for 2.1.11 would be 2020-12-11, which was three weeks ago. Last release 2.0.19 was issued on 2020-11-06. There are currently 24 patches in the queue cut down this way: - 4 MAJOR, first one merged on 2020-11-13 - 5 MEDIUM, first one merged on 2020-11-13 - 15 MINOR, first one merged on 2020-11-13 Thus the computed ideal release date for 2.0.20 would be 2020-12-11, which was three weeks ago. The current list of patches in the queue is: - 2.0, 2.1 - MAJOR : peers: fix partial message decoding - 2.0, 2.1 - MAJOR : filters: Always keep all offsets up to date during data filtering - 2.0, 2.1, 2.2, 2.3- MAJOR : spoa/python: Fixing return None - 2.0, 2.1 - MAJOR : spoe: Be sure to remove all references on a released spoe applet - 2.2, 2.3 - MAJOR : ring: tcp forward on ring can break the reader counter. - 2.3 - MEDIUM : local log format regression. - 2.3 - MEDIUM : lists: Lock the element while we check if it is in a list. - 2.3 - MEDIUM : task: close a possible data race condition on a tasklet's list link - 2.0, 2.1 - MEDIUM : peers: fix decoding of multi-byte length in stick-table messages - 2.0, 2.1, 2.2, 2.3- MEDIUM : spoa/python: Fixing references to None - 2.0, 2.1 - MEDIUM : filters: Forward all filtered data at the end of http filtering - 2.0, 2.1, 2.2, 2.3- MEDIUM : lb-leastconn: Reposition a server using the right eweight - 2.0, 2.1, 2.2, 2.3- MEDIUM : spoa/python: Fixing PyObject_Call positional arguments - 2.3 - MINOR : listener: use sockaddr_in6 for IPv6 - 2.0, 2.1, 2.2, 2.3- MINOR : spoa/python: Cleanup ipaddress objects if initialization fails - 2.0, 2.1 - MINOR : pattern: a sample marked as const could be written - 2.0, 2.1, 2.2, 2.3- MINOR : lua: Some lua init operation are processed unsafe - 2.0, 2.1 - MINOR : lua: set buffer size during map lookups - 2.0, 2.1, 2.2, 2.3- MINOR : tools: Reject size format not starting by a digit - 2.0, 2.1, 2.2, 2.3- MINOR : lua: warn when registering action, conv, sf, cli or applet multiple times - 2.0, 2.1, 2.2, 2.3- MINOR : spoa/python: Cleanup references for failed Module Addobject operations - 2.0, 2.1 - MINOR : http-fetch: Fix calls w/o parentheses of the cookie sample fetches - 2.0, 2.1, 2.2, 2.3- MINOR : lua: Post init register function are not executed beyond the first one - 2.3 - MINOR : mux-h2/stats: not all GOAWAY frames are errors - 2.1, 2.2, 2.3 - MINOR : lua: missing "\n" in error message - 2.2, 2.3 - MINOR : tcpcheck: Don't rearm the check timeout on each read - 2.0, 2.1 - MINOR : peers: Missing TX cache entries reset. - 2.0, 2.1, 2.2, 2.3- MINOR : tools: make parse_time_err() more strict on the timer validity - 2.0, 2.1 - MINOR : http-ana: Don't wait for the body of CONNECT requests - 2.0, 2.1 - MINOR : http-fetch: Extract cookie value even when no cookie name - 2.2, 2.3 - MINOR : http-check: Use right condition to consider HTX message as full - 2.3 - MINOR : mux-h2/stats: make stream/connection proto errors more accurate - 2.0, 2.1, 2.2, 2.3- MINOR : lua: lua-load doesn't check its
Re: [PATCH] CI: build popular contrib tools
we can try to set up a filter "build only if contrib folder was changed". not sure such a filter exists natively in github actions вт, 5 янв. 2021 г. в 21:45, Willy Tarreau : > On Tue, Jan 05, 2021 at 05:36:30PM +0100, Tim Düsterhus wrote: > (...) > > The patch in this state is okay for me, you can take it. > > Perfect, now merged, thanks for the explanation! > Willy >
Re: [PATCH] BUILD: Print a warning when building without USE_OPENSSL=1
On Tue, Jan 05, 2021 at 06:17:56PM +0100, Tim Duesterhus wrote: > haproxy: $(OPTIONS_OBJS) $(OBJS) > $(cmd_LD) $(LDFLAGS) -o $@ $^ $(LDOPTS) > +ifeq ($(USE_OPENSSL),) > + $(warning Building without TLS support. Add USE_OPENSSL to add support > for TLS encrypted connections.) > +endif > + I'm not fond of this one. It tells you after a long build that it's probably not what you want, and is particularly annoying for anyone. I'd rather do it before the build, and only if USE_OPENSSL wasn't forced either way. Again there's no rush on this, and if we figure a nice way to print options before starting the build, we could adopt it to dump several ones instead of just SSL. Willy
Re: [PATCH] DOC: Improve the message printed when running `make` w/o `TARGET`
On Tue, Jan 05, 2021 at 06:10:41PM +0100, Tim Duesterhus wrote: > Rephrase the message to no longer talk about something that "is no longer > supported", but about what actually *is* supported. Thanks, I like this output better than the previous one and just merged it. That doesn't mean I'm still totally convinced about SSL itself but seeing LUA, ZLIB and PCRE there makes me think that even though SSL is a common dependency, it's not that much special in itself. Let's try with this as a starter, and in parallel let's see what other users think of this. There's no rush anyway, we still have a few months before 2.4 to change our minds multiple times based on feedback :-) Willy
Re: Question about USE_OPENSSL build option
Sorry, previous mail left too early :-) On Tue, Jan 5, 2021 at 5:59 PM Willy Tarreau wrote: > Note, they still have to enter the target operating system so minimal > reading is necessary. But this can be addressed in the makefile's help > message which is their first contact indicating them what target to use. > (we could even suggest what the current target looks like for some of > them). I also agree that's one potential thing which could be nice to address to make it easier. -- William
Re: Question about USE_OPENSSL build option
On Tue, Jan 5, 2021 at 5:59 PM Willy Tarreau wrote: > > I used to think most people use `use_openssl=1` and wondered why it > > was not the default, but I recently discovered a large setup not > > making use of tls. The market is however strongly moving towards end > > to end encryption so I would say it makes sense to have use_openssl=1 > > by default. > > At least not to have to type it anymore ? correct. > Note, they still have to enter the target operating system so minimal > reading is necessary. But this can be addressed in the makefile's help > message which is their first contact indicating them what target to use. > (we could even suggest what the current target looks like for some of > them). > > > A developer/maintainer knows how to deactivate it for test purposes to > > reply to Tim's comment even if it is longer to type. > > It's true as well. Nowadays I have a myriad of build scripts which all > build with various options combinations, for various platforms, with > various debugging options etc, so the typing time on the developer's > machine is not a big deal: > > $ ls make-*|wc -l > 84 > > So I don't find myself often adding USE_OPENSSL=1 by hand. But on the > other hand I also trapped myself into forgetting it when building by > hand for the same reason. > > Maybe if we figure a nice way to print some options before building, > it could then be nice to recap the main options used so that users > still have a chance to press Ctrl-C and change them. This would still > alleviate the need to read docs and provide indications about other > possible options (PCRE, ZLIB, etc). > > Willy -- William
[PATCH] BUILD: Print a warning when building without USE_OPENSSL=1
Add a warning to the `haproxy` target when building without USE_OPENSSL.
---
Makefile | 4
1 file changed, 4 insertions(+)
diff --git a/Makefile b/Makefile
index 4e6c834ed..e60b7d3c8 100644
--- a/Makefile
+++ b/Makefile
@@ -912,6 +912,10 @@ endif
haproxy: $(OPTIONS_OBJS) $(OBJS)
$(cmd_LD) $(LDFLAGS) -o $@ $^ $(LDOPTS)
+ifeq ($(USE_OPENSSL),)
+ $(warning Building without TLS support. Add USE_OPENSSL to add support
for TLS encrypted connections.)
+endif
+
objsize: haproxy
$(Q)objdump -t $^|grep ' g '|grep -F '.text'|awk '{print $$5 FS
$$6}'|sort
--
2.29.0
[PATCH] speling fixes
hello,
another spelcheck fixes.
Ilya
From 6f6d56943c6bf41a2b33cc572c9eab679dcdb5e1 Mon Sep 17 00:00:00 2001
From: Ilya Shipitsin
Date: Tue, 5 Jan 2021 22:10:46 +0500
Subject: [PATCH] CLEANUP: assorted typo fixes in the code and comments
This is 14th iteration of typo fixes
---
include/haproxy/http_ana-t.h | 2 +-
include/haproxy/quic_cc-t.h | 2 +-
include/haproxy/quic_tls.h | 2 +-
include/haproxy/xprt_quic-t.h| 4 +-
include/haproxy/xprt_quic.h | 14 +++
include/import/xxhash.h | 4 +-
reg-tests/cache/vary.vtc | 2 +-
reg-tests/compression/lua_validation.vtc | 2 +-
src/quic_frame.c | 48
src/ssl_sock.c | 2 +-
src/xprt_quic.c | 40 ++--
11 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/include/haproxy/http_ana-t.h b/include/haproxy/http_ana-t.h
index c7d03ec81..63085a5b8 100644
--- a/include/haproxy/http_ana-t.h
+++ b/include/haproxy/http_ana-t.h
@@ -93,7 +93,7 @@
/* Maximum length of the cache secondary key (sum of all the possible parts of
* the secondary key). The actual keys might be smaller for some
* request/response pairs, because they depend on the responses' optional Vary
- * header. The differents sizes can be found in the vary_information object (see
+ * header. The different sizes can be found in the vary_information object (see
* cache.c).*/
#define HTTP_CACHE_SEC_KEY_LEN (sizeof(int)+sizeof(int)+sizeof(int))
diff --git a/include/haproxy/quic_cc-t.h b/include/haproxy/quic_cc-t.h
index 2c11011f4..5089d665e 100644
--- a/include/haproxy/quic_cc-t.h
+++ b/include/haproxy/quic_cc-t.h
@@ -84,7 +84,7 @@ union quic_cc_algo_state {
};
struct quic_cc {
- /* is there ony for debugging purpose. */
+ /* is there only for debugging purpose. */
struct quic_conn *qc;
struct quic_cc_algo *algo;
union quic_cc_algo_state algo_state;
diff --git a/include/haproxy/quic_tls.h b/include/haproxy/quic_tls.h
index 3d2160134..2db41dd22 100644
--- a/include/haproxy/quic_tls.h
+++ b/include/haproxy/quic_tls.h
@@ -220,7 +220,7 @@ static inline const char *ssl_error_str(int err)
return "WANT_CLIENT_HELLO_CB";
#endif
default:
- return "UNKNWON";
+ return "UNKNOWN";
}
}
diff --git a/include/haproxy/xprt_quic-t.h b/include/haproxy/xprt_quic-t.h
index 6cf9fae3b..2b89e4a8a 100644
--- a/include/haproxy/xprt_quic-t.h
+++ b/include/haproxy/xprt_quic-t.h
@@ -146,7 +146,7 @@ enum quic_pkt_type {
#define QUIC_PACKET_KEY_PHASE_BIT0x04 /* (protected) */
/*
- * Tranport level error codes.
+ * Transport level error codes.
*/
#define NO_ERROR 0x00
#define INTERNAL_ERROR 0x01
@@ -577,7 +577,7 @@ struct quic_conn {
size_t enc_params_len;
/*
- * Original Destination Connection ID (comming with first client Initial packets).
+ * Original Destination Connection ID (coming with first client Initial packets).
* Used only by servers.
*/
struct ebmb_node odcid_node;
diff --git a/include/haproxy/xprt_quic.h b/include/haproxy/xprt_quic.h
index 17b325431..ca49097f4 100644
--- a/include/haproxy/xprt_quic.h
+++ b/include/haproxy/xprt_quic.h
@@ -54,7 +54,7 @@ static inline size_t sizeof_quic_cid(const struct quic_cid *cid)
}
/* Copy QUIC CID to .
- * This is the responsability of the caller to check there is enough room in
+ * This is the responsibility of the caller to check there is enough room in
* to copy .
* Always succeeds.
*/
@@ -145,7 +145,7 @@ static inline void quic_connection_id_to_frm_cpy(struct quic_frame *dst,
/* Allocate a new CID with as sequence number and attach it to
* ebtree.
- * Returns the new CID if succedded, NULL if not.
+ * Returns the new CID if succeeded, NULL if not.
*/
static inline struct quic_connection_id *new_quic_cid(struct eb_root *root,
int seq_num)
@@ -213,7 +213,7 @@ static inline int quic_read_uint32(uint32_t *val,
/* Write a 32-bits integer to a buffer with as address.
* Make point to the data after this 32-buts value if succeeded.
- * Note that thes 32-bits integers are networkg bytes ordered.
+ * Note that these 32-bits integers are networkg bytes ordered.
* Returns 0 if failed (not enough room in the buffer), 1 if succeeded.
*/
static inline int quic_write_uint32(unsigned char **buf,
@@ -465,7 +465,7 @@ static inline size_t quic_packet_number_length(int64_t pn,
/* Encode packet number with as length in byte into a buffer with
* as current copy address and as pointer to one past the end of
- * this buffer. This is the responsability of the caller to check there is
+ * this buffer. This is the responsibility of the caller to check there is
* enough room in the buffer to copy bytes.
* Never fails.
*/
@@ -542,7 +542,7 @@ static inline void
[PATCH] DOC: Improve the message printed when running `make` w/o `TARGET`
Rephrase the message to no longer talk about something that "is no longer supported", but about what actually *is* supported. Adjustments include: - Removal of rare targets to make it easier to find the proper one. - Reformatting to be easier to read (more newlines) - Explanation of common non-default feature flags. --- Makefile | 36 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index 0ea3ef6b8..4e6c834ed 100644 --- a/Makefile +++ b/Makefile @@ -790,23 +790,35 @@ endif ifeq ($(TARGET),) all: + @echo "Building HAProxy without specifying a TARGET is not supported." @echo - @echo "Due to too many reports of suboptimized setups, building without" - @echo "specifying the target is no longer supported. Please specify the" - @echo "target OS in the TARGET variable, in the following form:" + @echo "Usage:" @echo - @echo " $ make TARGET=xxx" + @echo "$ make help # To print a full explanation." + @echo "$ make TARGET=xxx USE_=1 # To build HAProxy." @echo - @echo "Please choose the target among the following supported list :" + @echo "The most commonly used targets are:" @echo - @echo " linux-glibc, linux-glibc-legacy, linux-musl, solaris, freebsd, dragonfly, " - @echo " openbsd, netbsd, cygwin, haiku, aix51, aix52, aix72-gcc, osx, generic, " - @echo " custom" + @echo "linux-glibc- Modern Linux with glibc" + @echo "linux-musl - Modern Linux with musl" + @echo "freebsd- FreeBSD" + @echo "openbsd- OpenBSD" + @echo "netbsd - NetBSD" + @echo "osx- macOS" + @echo "solaris- Solaris" @echo - @echo "Use \"generic\" if you don't want any optimization, \"custom\" if you" - @echo "want to precisely tweak every option, or choose the target which" - @echo "matches your OS the most in order to gain the maximum performance" - @echo "out of it. Please check the Makefile in case of doubts." + @echo "Choose the target which matches your OS the most in order to" + @echo "gain the maximum performance out of it." + @echo + @echo "Common features you might want to include in your build are:" + @echo + @echo "USE_OPENSSL=1 - Support for TLS encrypted connections" + @echo "USE_ZLIB=1- Support for HTTP response compression" + @echo "USE_PCRE=1- Support for PCRE regular expressions" + @echo "USE_LUA=1 - Support for dynamic processing using Lua" + @echo + @echo "Use 'make help' to print a full explanation of supported targets" + @echo "and features." @echo @exit 1 else -- 2.29.0
Re: Question about USE_OPENSSL build option
On Tue, Jan 05, 2021 at 05:44:27PM +0100, William Dauchy wrote: > Hi Willy, > > On Tue, Jan 5, 2021 at 5:23 PM Willy Tarreau wrote: > > as I suspected in issue #1020, another user got trapped not enabling > > SSL when building from sources (probably for the first time, as it > > happens to everyone to build haproxy for the first time). > > > > Given that haproxy's main target is HTTP and that these days it often > > comes with SSL (and it doesn't seem like it's going to revert soon), > > I was wondering if it would be a good idea for 2.4 and onwards to preset > > USE_OPENSSL=1 by default. At least users who face build errors will have > > a glance at the README and figure how to disable it if they don't want > > it. But providing a successful build which misses some essential features > > doesn't sound like a very good long-term solution to me. > > > > I'm interested in any opinion here. > > I used to think most people use `use_openssl=1` and wondered why it > was not the default, but I recently discovered a large setup not > making use of tls. The market is however strongly moving towards end > to end encryption so I would say it makes sense to have use_openssl=1 > by default. At least not to have to type it anymore ? > People like things which work out of the box without > reading any doc. So I'm quite a supporter of that change. Note, they still have to enter the target operating system so minimal reading is necessary. But this can be addressed in the makefile's help message which is their first contact indicating them what target to use. (we could even suggest what the current target looks like for some of them). > A developer/maintainer knows how to deactivate it for test purposes to > reply to Tim's comment even if it is longer to type. It's true as well. Nowadays I have a myriad of build scripts which all build with various options combinations, for various platforms, with various debugging options etc, so the typing time on the developer's machine is not a big deal: $ ls make-*|wc -l 84 So I don't find myself often adding USE_OPENSSL=1 by hand. But on the other hand I also trapped myself into forgetting it when building by hand for the same reason. Maybe if we figure a nice way to print some options before building, it could then be nice to recap the main options used so that users still have a chance to press Ctrl-C and change them. This would still alleviate the need to read docs and provide indications about other possible options (PCRE, ZLIB, etc). Willy
Re: Question about USE_OPENSSL build option
On Tue, Jan 05, 2021 at 05:34:46PM +0100, Tim Düsterhus wrote: > Willy, > > Am 05.01.21 um 17:22 schrieb Willy Tarreau: > > Given that haproxy's main target is HTTP and that these days it often > > comes with SSL (and it doesn't seem like it's going to revert soon), > > I was wondering if it would be a good idea for 2.4 and onwards to preset > > USE_OPENSSL=1 by default. At least users who face build errors will have > > a glance at the README and figure how to disable it if they don't want > > it. But providing a successful build which misses some essential features > > doesn't sound like a very good long-term solution to me. > > > > I'm interested in any opinion here. > > > > This would be a -1 from my side. For development and testing I usually > build with a simple `make -j4 all TARGET=linux-glibc` to keep build > times low. > > I suspect that the vast majority of users consume distro packages > anyway. Users that compile themselves can usually be expected to read > the `INSTALL` file. I tend to agree on this point. However the scenario probably is: $ make (... blabbering about all suppored targets ...) $ make TARGET=mytarget Thus the help message from the makefile should at least suggest that plenty of other options exist (and give hints about common ones). > I would be fine with a warning if `USE_OPENSSL` is not explicitly > provided, though. I hadn't thought about this one. It's true that it could be nice to see something like "note: USE_OPENSSL not specified, building without SSL support". It's not necessarily trivial to fit into the makefile for the "all" target however, as we can't run actions before the dependencies are met, so it would probably be a dirty hack before the target definition. But it could be worth trying. Thanks! Willy
Re: [PATCH] CI: build popular contrib tools
On Tue, Jan 05, 2021 at 05:36:30PM +0100, Tim Düsterhus wrote: (...) > The patch in this state is okay for me, you can take it. Perfect, now merged, thanks for the explanation! Willy
Re: Question about USE_OPENSSL build option
Hi Willy, On Tue, Jan 5, 2021 at 5:23 PM Willy Tarreau wrote: > as I suspected in issue #1020, another user got trapped not enabling > SSL when building from sources (probably for the first time, as it > happens to everyone to build haproxy for the first time). > > Given that haproxy's main target is HTTP and that these days it often > comes with SSL (and it doesn't seem like it's going to revert soon), > I was wondering if it would be a good idea for 2.4 and onwards to preset > USE_OPENSSL=1 by default. At least users who face build errors will have > a glance at the README and figure how to disable it if they don't want > it. But providing a successful build which misses some essential features > doesn't sound like a very good long-term solution to me. > > I'm interested in any opinion here. I used to think most people use `use_openssl=1` and wondered why it was not the default, but I recently discovered a large setup not making use of tls. The market is however strongly moving towards end to end encryption so I would say it makes sense to have use_openssl=1 by default. People like things which work out of the box without reading any doc. So I'm quite a supporter of that change. A developer/maintainer knows how to deactivate it for test purposes to reply to Tim's comment even if it is longer to type. -- William
Re: [PATCH] CI: build popular contrib tools
Willy, Am 05.01.21 um 17:17 schrieb Willy Tarreau: Looks good to me in general. However for this one it would probably be sufficient to run it on schedule once a day / week or so. The contrib stuff is not that important. >>> >>> I'm ok with running them on push. > > Does this mean this is what the current patch implements ? > In any case, I just need to know if I have to merge it or not, > nothing more. The current patch already runs them on `push`. I suggested that `schedule` might be sufficient for the less important contrib/ stuff, which Ilya declined. The patch in this state is okay for me, you can take it. Best regards Tim Düsterhus
Re: Question about USE_OPENSSL build option
Willy, Am 05.01.21 um 17:22 schrieb Willy Tarreau: > Given that haproxy's main target is HTTP and that these days it often > comes with SSL (and it doesn't seem like it's going to revert soon), > I was wondering if it would be a good idea for 2.4 and onwards to preset > USE_OPENSSL=1 by default. At least users who face build errors will have > a glance at the README and figure how to disable it if they don't want > it. But providing a successful build which misses some essential features > doesn't sound like a very good long-term solution to me. > > I'm interested in any opinion here. > This would be a -1 from my side. For development and testing I usually build with a simple `make -j4 all TARGET=linux-glibc` to keep build times low. I suspect that the vast majority of users consume distro packages anyway. Users that compile themselves can usually be expected to read the `INSTALL` file. I would be fine with a warning if `USE_OPENSSL` is not explicitly provided, though. Best regards Tim Düsterhus
Re: [PATCH] CLEANUP: spoe: fix typo on `var_check_arg` comment
On Tue, Jan 05, 2021 at 11:14:58AM +0100, William Dauchy wrote: > there was an extra `s` added to the `var_check_arg` function Merged, thank you William. Willy
Question about USE_OPENSSL build option
Hi all, as I suspected in issue #1020, another user got trapped not enabling SSL when building from sources (probably for the first time, as it happens to everyone to build haproxy for the first time). Given that haproxy's main target is HTTP and that these days it often comes with SSL (and it doesn't seem like it's going to revert soon), I was wondering if it would be a good idea for 2.4 and onwards to preset USE_OPENSSL=1 by default. At least users who face build errors will have a glance at the README and figure how to disable it if they don't want it. But providing a successful build which misses some essential features doesn't sound like a very good long-term solution to me. I'm interested in any opinion here. Thanks, Willy
Re: [PATCH] CI: build popular contrib tools
Hi Ilya, On Tue, Jan 05, 2021 at 08:11:00PM +0500, ??? wrote: > ping I thought you were going to change something regarding this below: > >> Looks good to me in general. However for this one it would probably be > >> sufficient to run it on schedule once a day / week or so. The contrib > >> stuff is not that important. > >> > > > > I'm ok with running them on push. Does this mean this is what the current patch implements ? In any case, I just need to know if I have to merge it or not, nothing more. Thanks, Willy
Re: [PATCH 1/2] CLEANUP: Reduce scope of `header_name` in http_action_store_cache()
On Sat, Jan 02, 2021 at 10:47:16PM +0100, Tim Duesterhus wrote: > This variable is only needed deeply nested in a single location and clang's > static analyzer complains about a dead initialization. Reduce the scope to > satisfy clang and the human that reads the function. On Sat, Jan 02, 2021 at 10:47:17PM +0100, Tim Duesterhus wrote: > This is only required to process the `age` header. Thanks Tim, pushed in master. -- William Lallemand
Re: [PATCH] CI: build popular contrib tools
ping On Wed, Dec 30, 2020, 5:50 PM Илья Шипицин wrote: > > > On Wed, Dec 30, 2020, 4:23 PM Tim Düsterhus wrote: > >> Ilya, >> >> Am 30.12.20 um 11:28 schrieb Илья Шипицин: >> > all tools are built in separate job. >> >> Looks good to me in general. However for this one it would probably be >> sufficient to run it on schedule once a day / week or so. The contrib >> stuff is not that important. >> > > I'm ok with running them on push. > > > >> Best regards >> Tim Düsterhus >> >
Re: [PATCH v2] MINOR: converter: adding support for url_enc
Hi William,
A few comments below.
On Tue, Jan 05, 2021 at 12:08:58PM +0100, William Dauchy wrote:
> diff --git a/src/http_conv.c b/src/http_conv.c
> index 4afa6a2fd..0fded0cc7 100644
> --- a/src/http_conv.c
> +++ b/src/http_conv.c
> @@ -268,6 +268,83 @@ static int sample_conv_url_dec(const struct arg *args,
> struct sample *smp, void
> return 1;
> }
>
> +/* Check url-encode type */
> +enum encode_type {
> + ENC_QUERY = 0,
> +};
Please leave a blank line here between the end of the enum and the function.
> +static int sample_conv_url_enc_check(struct arg *arg, struct sample_conv
> *conv,
> + const char *file, int line, char **err)
> +{
> + enum encode_type enc_type;
> +
> + enc_type = ENC_QUERY;
> + if (!arg) {
> + memprintf(err, "Unexpected empty arg list");
> + return 0;
> + }
As much as I remember, check functions are never called with a NULL args
list, it's always an array, so you can drop this test.
> + if (arg->type != ARGT_STR) {
> + memprintf(err, "Unexpected arg type for encoding");
> + return 0;
> + }
The expression parser guarantees that you can't have anything else, because
you mentioned your converter takes a mandatory string argument. Thus this
test can be removed as well.
> +
> + if (arg && arg->type == ARGT_STR) {
This test is redundant with to the two previous ones. And since we know
they can never happen, you can simply remove the test and leave the code
below out of the block.
> + if (strcmp(arg->data.str.area, "") == 0)
> + enc_type = ENC_QUERY;
> + else if (strcmp(arg->data.str.area, "query") == 0)
> + enc_type = ENC_QUERY;
> + else {
> + memprintf(err, "Unexpected encode type. "
> + "Allowed value is 'query'");
> + return 0;
> + }
> + }
> +
> + chunk_destroy(>data.str);
I didn't remember we needed to do this, I had to recheck :-)
> + arg->type = ARGT_SINT;
> + arg->data.sint = enc_type;
> + return 1;
> +}
> +
> +/* This fetch url-encode any input string. Only support query string for now
> */
> +static int sample_conv_url_enc(const struct arg *args, struct sample *smp,
> void
> + *private)
> +{
> + enum encode_type enc_type;
> + struct buffer *trash = get_trash_chunk();
> + long url_encode_map[(256 / 8) / sizeof(long)];
> + char *ret;
> + int i;
> +
> + enc_type = ENC_QUERY;
> + if (args)
> + enc_type = args->data.sint;
> +
> + /* Add final \0 required by encode_string() */
> + smp->data.u.str.area[smp->data.u.str.data] = '\0';
> +
> + memset(url_encode_map, 0, sizeof(url_encode_map));
> + if (enc_type == ENC_QUERY) {
> + /* use rfc3986 to determine list of characters to keep
> unchanged for
> + * query string */
> + for (i = 0; i < 256; i++) {
> + if (!((i >= 'a' && i <= 'z') || (i >= 'A' && i <= 'Z')
> + || (i >= '0' && i <= '9') ||
> + i == '-' || i == '.' || i == '_' || i == '~'))
> + ha_bit_set(i, url_encode_map);
> + }
No, this is not a good idea, you're doing this in the stack for every
single call. Better put it outside, as a global variable, and initalize
it in an initcall function. I'm even seeing that we have similarly named
variables in log.c, maybe you can call your variable query_encode_map()
or url_enc_query_map() or anything else that makes sense to you.
> + } else {
> + return 0;
> + }
> + ret = encode_string(trash->area, trash->area + trash->size, '%',
> + url_encode_map, smp->data.u.str.area);
> + if (ret == NULL || *ret != '\0')
> + return 0;
> + trash->data = strlen(trash->area);
>From what I'm seeing from the encode_string() function, it returns the
pointer to the trailing zero so that you don't have to run a costly
strlen() over it, thus you can simply write:
trash->data = ret - trash->area;
> + smp->data.u.str = *trash;
> + return 1;
> +}
Thanks!
Willy
[PATCH v2] MINOR: converter: adding support for url_enc
add base support for url encode following RFC3986, supporting `query`
type only.
- add test checking url_enc/url_dec/url_enc
- update documentation
- leave the door open for future changes
this should resolve github issue #941
Signed-off-by: William Dauchy
---
changed in v2:
- adding support for encode type argument validation
---
doc/configuration.txt | 6 +++
reg-tests/converter/url_enc.vtc | 43 ++
src/http_conv.c | 78 +
3 files changed, 127 insertions(+)
create mode 100644 reg-tests/converter/url_enc.vtc
diff --git a/doc/configuration.txt b/doc/configuration.txt
index e5e854623..bdacfcd6d 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -16240,6 +16240,12 @@ url_dec([])
space (' '). Otherwise this will only happen after a question mark indicating
a query string ('?').
+url_enc([])
+ Takes a string provided as input and returns the encoded version as output.
+ The input and the output are of type string. By default the type of encoding
+ is meant for `query` type. There is no other type supported for now but the
+ optional argument is here for future changes.
+
ungrpc(,[])
This extracts the protocol buffers message field in raw mode of an input
binary
sample representation of a gRPC message with as field number
diff --git a/reg-tests/converter/url_enc.vtc b/reg-tests/converter/url_enc.vtc
new file mode 100644
index 0..a3f70ade9
--- /dev/null
+++ b/reg-tests/converter/url_enc.vtc
@@ -0,0 +1,43 @@
+varnishtest "url_enc converter test"
+
+#REQUIRE_VERSION=2.4
+
+feature ignore_unknown_macro
+
+server s1 {
+ rxreq
+ txresp
+} -repeat 2 -start
+
+haproxy h1 -conf {
+defaults
+ mode http
+ timeout connect 1s
+ timeout client 1s
+ timeout server 1s
+
+frontend fe
+ bind "fd@${fe}"
+
+ http-request set-var(txn.url0) "str(foo=bar+42 42 )"
+ http-request set-var(txn.url1) "var(txn.url0),url_enc"
+ http-request set-var(txn.url2) "var(txn.url1),url_dec"
+ http-request set-var(txn.url3) "var(txn.url2),url_enc(query)"
+ http-response set-header url_enc0 "%[var(txn.url1)]"
+ http-response set-header url_dec "%[var(txn.url2)]"
+ http-response set-header url_enc1 "%[var(txn.url3)]"
+
+ default_backend be
+
+backend be
+ server s1 ${s1_addr}:${s1_port}
+} -start
+
+client c1 -connect ${h1_fe_sock} {
+ txreq -url "/"
+ rxresp
+ expect resp.http.url_enc0 == "foo%3Dbar%2B42%2042%20"
+ expect resp.http.url_dec == "foo=bar+42 42 "
+ expect resp.http.url_enc1 == "foo%3Dbar%2B42%2042%20"
+ expect resp.status == 200
+} -run
diff --git a/src/http_conv.c b/src/http_conv.c
index 4afa6a2fd..0fded0cc7 100644
--- a/src/http_conv.c
+++ b/src/http_conv.c
@@ -268,6 +268,83 @@ static int sample_conv_url_dec(const struct arg *args,
struct sample *smp, void
return 1;
}
+/* Check url-encode type */
+enum encode_type {
+ ENC_QUERY = 0,
+};
+static int sample_conv_url_enc_check(struct arg *arg, struct sample_conv *conv,
+const char *file, int line, char **err)
+{
+ enum encode_type enc_type;
+
+ enc_type = ENC_QUERY;
+ if (!arg) {
+ memprintf(err, "Unexpected empty arg list");
+ return 0;
+ }
+
+ if (arg->type != ARGT_STR) {
+ memprintf(err, "Unexpected arg type for encoding");
+ return 0;
+ }
+
+ if (arg && arg->type == ARGT_STR) {
+ if (strcmp(arg->data.str.area, "") == 0)
+ enc_type = ENC_QUERY;
+ else if (strcmp(arg->data.str.area, "query") == 0)
+ enc_type = ENC_QUERY;
+ else {
+ memprintf(err, "Unexpected encode type. "
+ "Allowed value is 'query'");
+ return 0;
+ }
+ }
+
+ chunk_destroy(>data.str);
+ arg->type = ARGT_SINT;
+ arg->data.sint = enc_type;
+ return 1;
+}
+
+/* This fetch url-encode any input string. Only support query string for now */
+static int sample_conv_url_enc(const struct arg *args, struct sample *smp, void
+ *private)
+{
+ enum encode_type enc_type;
+ struct buffer *trash = get_trash_chunk();
+ long url_encode_map[(256 / 8) / sizeof(long)];
+ char *ret;
+ int i;
+
+ enc_type = ENC_QUERY;
+ if (args)
+ enc_type = args->data.sint;
+
+ /* Add final \0 required by encode_string() */
+ smp->data.u.str.area[smp->data.u.str.data] = '\0';
+
+ memset(url_encode_map, 0, sizeof(url_encode_map));
+ if (enc_type == ENC_QUERY) {
+ /* use rfc3986 to determine list of characters to keep
unchanged for
+* query string */
+ for (i = 0; i < 256; i++) {
+
Re: [PATCH] MINOR: converter: adding support for url_enc
Hi Tim, Thanks for the review. On Tue, Dec 22, 2020 at 5:57 PM Tim Düsterhus wrote: > > +url_enc([]) > > + Takes a string provided as input and returns the encoded version as > > output. > > + The input and the output are of type string. By default the type of > > encoding > > + is meant for `query` type. There is no other type supported for now but > > the > > + optional argument is here for future changes. > > Can you add a function that actually validates that the `enc_type` is > either exactly 'query' or not provided at all to make sure it catches > typos? It goes where the NULL is in `sample_conv_kw_list`. now fixed in v2. > > + /* Add final \0 required by encode_string() */ > > + smp->data.u.str.area[smp->data.u.str.data] = '\0'; > > I'm not sure, but is this guaranteed to have enough space for that NUL? yes, that's also what we do in other places (see url_dec for example) -- William
Re: [PATCH] BUG/MINOR: cfgparse: Fail if the strdup() for `rule->be.name` for `use_backend` fails
On Sun, Jan 03, 2021 at 10:54:43PM +0100, Tim Duesterhus wrote: > This patch fixes GitHub issue #1024. Merged, thanks! Willy
Re: [PATCH] BUG/MINOR: Error out when a `server` has an `AF_UNSPEC` address
Hi Tim, On Mon, Jan 04, 2021 at 12:58:25AM +0100, Tim Duesterhus wrote: > Willy, > Thayne, > > find the patch below. > > I am adding Thayne as CC as it was your commit that uncovered the issue and > the > crash happened in a function you wrote. Maybe you might want to add some > additional checks somewhere? > > Best regards > Tim Düsterhus > > Apply with `git am --scissors` to automatically cut the commit message. > > -- >8 -- > GitHub Issue #1026 reported a crash during configuration check for the > following example config: > > backend 0 > server 0 0 > server 0 0 > > HAProxy crashed in srv_set_addr_desc() due to a NULL pointer dereference > caused > by `sa2str` returning NULL for an `AF_UNSPEC` address (`0`). That's embarrassing. > Fix this issue by erroring out early if the parsed address results in an > `AF_UNSPEC` family. Unfortunately we cannot do that or it destroys usage of the DNS or even any runtime address assignment over the CLI. For example if you write: server foo foo init-addr none I guess it won't work anymore since that's placed just after the address parsing. This means however that there probably is something more problematic in the referencing of address-less servers. Either servers are added/updated each time their address changes (to follow DNS) and then we can simply skip their registration when they're address-less. Or the servers are only registered at boot time, and the synchronization will fail after any address update. What do you think, Thayne ? Willy
[PATCH] CLEANUP: spoe: fix typo on `var_check_arg` comment
there was an extra `s` added to the `var_check_arg` function
Signed-off-by: William Dauchy
---
src/flt_spoe.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index 45370fee6..bfd6f4fbf 100644
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -4178,7 +4178,7 @@ parse_spoe_flt(char **args, int *cur_arg, struct proxy
*px,
arg.type = ARGT_STR;
arg.data.str.area = trash.area;
arg.data.str.data = trash.data;
- arg.data.str.size = 0; /* Set it to 0 to not release it in
vars_check_args() */
+ arg.data.str.size = 0; /* Set it to 0 to not release it in
vars_check_arg() */
if (!vars_check_arg(, err)) {
memprintf(err, "SPOE agent '%s': failed to register
variable %s.%s (%s)",
curagent->id, curagent->var_pfx,
curagent->var_on_error, *err);
@@ -4195,7 +4195,7 @@ parse_spoe_flt(char **args, int *cur_arg, struct proxy
*px,
arg.type = ARGT_STR;
arg.data.str.area = trash.area;
arg.data.str.data = trash.data;
- arg.data.str.size = 0; /* Set it to 0 to not release it in
vars_check_args() */
+ arg.data.str.size = 0; /* Set it to 0 to not release it in
vars_check_arg() */
if (!vars_check_arg(, err)) {
memprintf(err, "SPOE agent '%s': failed to register
variable %s.%s (%s)",
curagent->id, curagent->var_pfx,
curagent->var_t_process, *err);
@@ -4212,7 +4212,7 @@ parse_spoe_flt(char **args, int *cur_arg, struct proxy
*px,
arg.type = ARGT_STR;
arg.data.str.area = trash.area;
arg.data.str.data = trash.data;
- arg.data.str.size = 0; /* Set it to 0 to not release it in
vars_check_args() */
+ arg.data.str.size = 0; /* Set it to 0 to not release it in
vars_check_arg() */
if (!vars_check_arg(, err)) {
memprintf(err, "SPOE agent '%s': failed to register
variable %s.%s (%s)",
curagent->id, curagent->var_pfx,
curagent->var_t_process, *err);
@@ -4418,7 +4418,7 @@ parse_spoe_flt(char **args, int *cur_arg, struct proxy
*px,
arg.type = ARGT_STR;
arg.data.str.area = trash.area;
arg.data.str.data = trash.data;
- arg.data.str.size = 0; /* Set it to 0 to not release it in
vars_check_args() */
+ arg.data.str.size = 0; /* Set it to 0 to not release it in
vars_check_arg() */
if (!vars_check_arg(, err)) {
memprintf(err, "SPOE agent '%s': failed to register
variable %s.%s (%s)",
curagent->id, curagent->var_pfx, vph->name,
*err);
--
2.29.2
Bid Writing, Major Donors and Volunteering Workshops
NFP WORKSHOPS 18 Blake Street, York YO1 8QG 01133 280988 Affordable Training Courses for Charities, Schools & Public Sector Organisations This email has been sent to haproxy@formilux.org CLICK TO UNSUBSCRIBE FROM LIST Alternatively send a blank e-mail to unsubscr...@nfpmail2001.co.uk quoting haproxy@formilux.org in the subject line. Unsubscribe requests will take effect within seven days. Bid Writing: The Basics Online via ZOOM COST £95 TOPICS COVERED Do you know the most common reasons for rejection? Are you gathering the right evidence? Are you making the right arguments? Are you using the right terminology? Are your numbers right? Are you learning from rejections? Are you assembling the right documents? Do you know how to create a clear and concise standard funding bid? Are you communicating with people or just excluding them? Do you know your own organisation well enough? Are you thinking through your projects carefully enough? Do you know enough about your competitors? Are you answering the questions funders will ask themselves about your application? Are you submitting applications correctly? PARTICIPANTS Staff members, volunteers, trustees or board members of charities, schools, not for profits or public sector organisations who intend to submit grant funding applications to charitable grant making trusts and foundations. People who provide advice to these organisations are also welcome. Bid Writing: Advanced Online via ZOOM COST £95 TOPICS COVERED Are you applying to the right trusts? Are you applying to enough trusts? Are you asking for the right amount of money? Are you applying in the right ways? Are your projects the most fundable projects? Are you carrying out trust fundraising in a professional way? Are you delegating enough work? Are you highly productive or just very busy? Are you looking for trusts in all the right places? How do you compare with your competitors for funding? Is the rest of your fundraising hampering your bids to trusts? Do you understand what trusts are ideally looking for? PARTICIPANTS Staff members, volunteers, trustees or board members of charities, schools, not for profits or public sector organisations who intend to submit grant funding applications to charitable grant making trusts and foundations. People who provide advice to these organisations are also welcome. Dates & Booking Links BID WRITING: THE BASICS Mon 11 Jan 2021 10.00 to 12.30Booking Link Mon 25 Jan 2021 10.00 to 12.30Booking Link Mon 08 Feb 2021 10.00 to 12.30Booking Link Mon 22 Feb 2021 10.00 to 12.30Booking Link BID WRITING: ADVANCED Tue 12 Jan 2021 10.00 to 12.30Booking Link Tue 26 Jan 2021 10.00 to 12.30Booking Link Tue 09 Feb 2021 10.00 to 12.30Booking Link Tue 23 Feb 2021 10.00 to 12.30Booking Link Recruiting and Managing Volunteers Online via ZOOM COST £195 TOPICS COVERED Where do you find volunteers? How do you find the right volunteers? How do you attract volunteers? How do you run volunteer recruitment events? How do you interview volunteers? How do you train volunteers? How do you motivate volunteers? How do you involve volunteers? How do you recognise volunteer? How do you recognise problems with volunteers? How do you learn from volunteer problems? How do you retain volunteers? How do you manage volunteers? What about volunteers and your own staff? What about younger, older and employee volunteers? PARTICIPANTS Staff members, volunteers, trustees or board members of charities, schools, not for profits or public sector organisations who intend to recruit volunteers into their organisation and then manage those volunteers. People who provide advice to these organisations are also welcome. Dates & Booking Links RECRUITING AND MANAGING VOLUNTEERS Wed 13 Jan 2021 10.00 to 16.00Booking Link Wed 10 Mar 2021 10.00 to 16.00Booking Link Major Donor Fundraising Online via ZOOM COST £95 TOPICS COVERED Major Donor Characteristics, Motivations and Requirements. Researching and Screening Major Donors. Encouraging, Involving and Retaining Major Donors. Building Relationships with Major Donors. Major Donor Events and Activities. Setting Up Major Donor Clubs.Asking For Major Gifts. Looking After and Reporting Back to Major Donors. Delivering on Major Donor Expectations. Showing Your Appreciation to Major Donors. Fundraising Budgets and Committees. PARTICIPANTS Staff members, volunteers, trustees or board members of charities, schools, not for profits or public sector organisations who intend to carry out Major Donor Fundraising. People who provide advice to these organisations are also welcome. Dates & Booking Links MAJOR DONOR FUNDRAISING Wed 10 Feb 2021 10.00 to 12.30Booking Link Wed 14 Apr 2021 10.00 to 12.30Booking Link FEEDBACK FROM PAST ATTENDEES AT LIVE WORKSHOPS I must say I was really impressed with the course and the content. My knowledge and confidence has increased hugely. I got a
Re: [PATCH 2/3] CLEANUP: ssl: Remove useless loop in tlskeys_list_get_next()
Hi Tim, FWIW, I've just merged your two cleanups as they were independent on the gcc-specific part. Thanks, Willy
