Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Hi, On Sun, Jan 21, Tim Düsterhus wrote: > Jarno, > > Am 15.01.2018 um 14:28 schrieb Jarno Huuskonen: > > I'll try to test later (might be couple of days). > > Did you find time, yet? I did some brief testing on saturday and with this silly config frontend test4 bind ipv4@127.0.0.1:8080 bind ipv6@::1:8080 http-request track-sc1 src,ipmask(32,128) table test_be http-request track-sc0 req.hdr_ip(X,1),ipmask(24,64) table test_be http-request set-var(sess.myx) req.hdr_ip(X,1),ipmask(24,64) http-response set-header X-MY %[var(sess.myx)] default_backend test_be backend test_be stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt server wp1 some.ip.addr.ess:80 id 1 and curl -4|-6 -H "X: ipv4.or.ipv6.address" http://localhost:8080/ everything I tested seemed to work (I tested Tim's patch with for size_t outside of loop). -Jarno -- Jarno Huuskonen
[PATCH] BUG/MEDIUM: standard: Fix memory leak in str2ip2()
Hi
attached is a patch that fixes a memory leak in str2ip2. I wasn't sure
about the severity of this bug (it's only 140 Bytes per call for me) and
opted for MEDIUM. Change if you think MAJOR (?) is more warranted for a
memory leak.
Also I wasn't sure how I would structure the code best. I did not want to
copy the freeaddrinfo into every case, because don't repeat yourself, but
I am not sure whether the `success` variable is any better.
Best regards
Tim Düsterhus
Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --
Subject: [PATCH] BUG/MEDIUM: standard: Fix memory leak in str2ip2()
An haproxy compiled with:
> make -j4 all TARGET=linux2628 USE_GETADDRINFO=1
And running with a configuration like this:
defaults
log global
modehttp
option httplog
option dontlognull
timeout connect 5000
timeout client 5
timeout server 5
frontend fe
bind :::8080 v4v6
default_backend be
backend be
server s example.com:80 check
Will leak memory inside `str2ip2()`, because the list `result` is not
properly freed in success cases:
==18875== 140 (76 direct, 64 indirect) bytes in 1 blocks are definitely lost in
loss record 87 of 111
==18875==at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18875==by 0x537A565: gaih_inet (getaddrinfo.c:1223)
==18875==by 0x537DD5D: getaddrinfo (getaddrinfo.c:2425)
==18875==by 0x4868E5: str2ip2 (standard.c:733)
==18875==by 0x43F28B: srv_set_addr_via_libc (server.c:3767)
==18875==by 0x43F50A: srv_iterate_initaddr (server.c:3879)
==18875==by 0x43F50A: srv_init_addr (server.c:3944)
==18875==by 0x475B30: init (haproxy.c:1595)
==18875==by 0x40406D: main (haproxy.c:2479)
The exists as long as the usage of getaddrinfo in that function exists,
it was introduced in commit:
d5f4328efd5f4eaa7c89cad9773124959195430a
v1.5-dev8 is the first tag containing this comment, the fix
should be backported to haproxy 1.5 and newer.
---
src/standard.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/standard.c b/src/standard.c
index acd136c27..4f89a9217 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -722,6 +722,7 @@ struct sockaddr_storage *str2ip2(const char *str, struct
sockaddr_storage *sa, i
#ifdef USE_GETADDRINFO
if (global.tune.options & GTUNE_USE_GAI) {
struct addrinfo hints, *result;
+ int success = 0;
memset(&result, 0, sizeof(result));
memset(&hints, 0, sizeof(hints));
@@ -733,23 +734,30 @@ struct sockaddr_storage *str2ip2(const char *str, struct
sockaddr_storage *sa, i
if (getaddrinfo(str, NULL, &hints, &result) == 0) {
if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
sa->ss_family = result->ai_family;
- else if (sa->ss_family != result->ai_family)
+ else if (sa->ss_family != result->ai_family) {
+ freeaddrinfo(result);
goto fail;
+ }
switch (result->ai_family) {
case AF_INET:
memcpy((struct sockaddr_in *)sa,
result->ai_addr, result->ai_addrlen);
set_host_port(sa, port);
- return sa;
+ success = 1;
+ break;
case AF_INET6:
memcpy((struct sockaddr_in6 *)sa,
result->ai_addr, result->ai_addrlen);
set_host_port(sa, port);
- return sa;
+ success = 1;
+ break;
}
}
if (result)
freeaddrinfo(result);
+
+ if (success)
+ return sa;
}
#endif
/* try to resolve an IPv4/IPv6 hostname */
--
2.15.1
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Jarno, Willy, Am 15.01.2018 um 14:28 schrieb Jarno Huuskonen: > On Sun, Jan 14, Tim Düsterhus wrote: >>> Have you tested that req.hdr_ip / stick tables work w/both masks ? I >>> used something like: >>> http-request track-sc0 req.hdr_ip(X,1),ipmask(24,64) table test_be >>> http-request set-var(sess.myx) req.hdr_ip(X,1),ipmask(24,64) >>> http-response set-header X-MY %[var(sess.myx)] >>> >>> backend test_be >>> stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt >> I just read up on stick tables and think I figured it out: haproxy.cfg > defaults > log global > modehttp > option httplog > option dontlognull > timeout connect 5000 > timeout client 5 > timeout server 5 > > frontend fe > bind :::8080 v4v6 > > http-request track-sc0 src,ipmask(24,64) table be > # Masked IPv4 for IPv4, empty for IPv6 (with and without this commit) > http-response set-header Test %[src,ipmask(24)] > # Correctly masked IP addresses for both IPv4 and IPv6 > http-response set-header Test2 %[src,ipmask(24,:::::)] > # Correctly masked IP addresses for both IPv4 and IPv6 > http-response set-header Test3 %[src,ipmask(24,64)] > http-response set-header Stick %[sc0_conn_cnt(be)] > > default_backend be > > backend be > stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt > server s example.com:80 Calls: > [timwolla@/s/haproxy (master)]http --headers 127.0.0.3:8080 |grep Stick > Stick: 3 > [timwolla@/s/haproxy (master)]http --headers 127.0.0.1:8080 |grep Stick > Stick: 1 > [timwolla@/s/haproxy (master)]http --headers 127.0.0.2:8080 |grep Stick > Stick: 2 > [timwolla@/s/haproxy (master)]http --headers 127.0.0.3:8080 |grep Stick > Stick: 3 > [timwolla@/s/haproxy (master)]http --headers 192.168.178.38:8080 |grep Stick > Stick: 1 > [timwolla@/s/haproxy (master)]http --headers [::1]:8080 |grep Stick > Stick: 1 > [timwolla@/s/haproxy (master)]http --headers [::1]:8080 |grep Stick > Stick: 2 I think this is looking good, no? Best regards Tim Düsterhus
Re: Show: haproxy-auth-request - HTTP access control using subrequests
On Sun, Jan 21, 2018 at 4:17 PM, Tim Düsterhus wrote:
>> Quick question though: does this script actually block HAproxy's
>> event loop while waiting for the response from the backend server?
>
> haproxy's Socket class is documented to be non-blocking, as explained
> here: https://www.arpalert.org/haproxy-lua.html#h204 ("Non blocking
> design"). Most of my article focused on getting the Socket class of
> haproxy to work, instead of using the native Socket class of Lua for
> that reason.
OK, so it seems that "non-blocking" in the context of Lua for HAProxy
it is actually understood "asynchronous", in the sense that if / when
the call would block, the HAProxy event loop takes over and restarts
the Lua code only when the blocking operation can be retried without
blocking.
So for the purpose of developing in Lua, the code seems to be
"blocking" and "synchronous", but behind the scenes HAProxy controls
the Lua VM to transform the Lua code exeguhiot into and
"asynchronous".
Thanks,
Ciprian.
Re: Show: haproxy-auth-request - HTTP access control using subrequests
Ciprian,
Am 21.01.2018 um 13:26 schrieb Ciprian Dorin Craciun:
> Nice little tool!
Thank you.
> Quick question though: does this script actually block HAproxy's
> event loop while waiting for the response from the backend server?
>
> (I've read quickly through your article describing the script, but
> failed to get a clear answer to this question. Moreover looking at
> the script, unless HAProxy does something "funky" with the Lua
> interpreter to give the impression of a synchronous API, I would say
> it does block the HAProxy thread that calls this script.)
haproxy's Socket class is documented to be non-blocking, as explained
here: https://www.arpalert.org/haproxy-lua.html#h204 ("Non blocking
design"). Most of my article focused on getting the Socket class of
haproxy to work, instead of using the native Socket class of Lua for
that reason.
Best regards
Tim Düsterhus
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Willy, Am 21.01.2018 um 14:07 schrieb Willy Tarreau: > There's no problem here because these ones are word-aligned anyway. > There are other such occurrences elsewhere in the code, so don't > worry for this one. Okay, I sent an updated patch that uses the cast to uint32_t. > From what I've seen till now your patches look fine but like you I'm > interested in Jarno's feedback ;-) Could you possibly review pick the following list of patches (all of them bugfixes only; mostly for docs) already? - v2 1/8 -2/8 -3/8 -4/8 Possibly also: -5/8 -6/8 -7/8 which don't really touch live code (apart from 5). It would allow me to clean up a bit of my local repository. Best regards Tim Düsterhus
[PATCH v4 8/8] MEDIUM: sample: Add IPv6 support to the ipmask converter
Add an optional second parameter to the ipmask converter that specifies
the number of bits to mask off IPv6 addresses.
If the second parameter is not given IPv6 addresses fail to mask (resulting
in an empty string), preserving backwards compatibility: Previously
a sample like `src,ipmask(24)` failed to give a result for IPv6 addresses.
This feature can be tested like this:
defaults
log global
modehttp
option httplog
option dontlognull
timeout connect 5000
timeout client 5
timeout server 5
frontend fe
bind :::8080 v4v6
# Masked IPv4 for IPv4, empty for IPv6 (with and without this commit)
http-response set-header Test %[src,ipmask(24)]
# Correctly masked IP addresses for both IPv4 and IPv6
http-response set-header Test2 %[src,ipmask(24,:::::)]
# Correctly masked IP addresses for both IPv4 and IPv6
http-response set-header Test3 %[src,ipmask(24,64)]
default_backend be
backend be
server s example.com:80
---
doc/configuration.txt | 11 +++
src/sample.c | 27 ++-
2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 827506788..eaaf0f71f 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -12870,11 +12870,14 @@ in_table()
the presence of a certain key in a table tracking some elements (e.g. whether
or not a source IP address or an Authorization header was already seen).
-ipmask()
- Apply a mask to an IPv4 address, and use the result for lookups and storage.
+ipmask(, [])
+ Apply a mask to an IP address, and use the result for lookups and storage.
This can be used to make all hosts within a certain mask to share the same
- table entries and as such use the same server. The mask can be passed in
- dotted form (e.g. 255.255.255.0) or in CIDR form (e.g. 24).
+ table entries and as such use the same server. The mask4 can be passed in
+ dotted form (e.g. 255.255.255.0) or in CIDR form (e.g. 24). The mask6 can
+ be passed in quadruplet form (e.g. :::) or in CIDR form (e.g. 64).
+ If no mask6 is given IPv6 addresses will fail to convert for backwards
+ compatibility reasons.
json([])
Escapes the input string and produces an ASCII output string ready to use as
a
diff --git a/src/sample.c b/src/sample.c
index 04cec4fc8..0155b3d69 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -1603,11 +1603,28 @@ static int sample_conv_str2upper(const struct arg
*arg_p, struct sample *smp, vo
return 1;
}
-/* takes the netmask in arg_p */
-static int sample_conv_ipmask(const struct arg *arg_p, struct sample *smp,
void *private)
+/* takes the IPv4 mask in args[0] and an optional IPv6 mask in args[1] */
+static int sample_conv_ipmask(const struct arg *args, struct sample *smp, void
*private)
{
- smp->data.u.ipv4.s_addr &= arg_p->data.ipv4.s_addr;
- smp->data.type = SMP_T_IPV4;
+ /* Attempt to convert to IPv4 to apply the correct mask. */
+ c_ipv62ip(smp);
+
+ if (smp->data.type == SMP_T_IPV4) {
+ smp->data.u.ipv4.s_addr &= args[0].data.ipv4.s_addr;
+ smp->data.type = SMP_T_IPV4;
+ }
+ else if (smp->data.type == SMP_T_IPV6) {
+ /* IPv6 cannot be converted without an IPv6 mask. */
+ if (args[1].type != ARGT_IPV6)
+ return 0;
+
+ *(uint32_t*)&smp->data.u.ipv6.s6_addr[0] &=
*(uint32_t*)&args[1].data.ipv6.s6_addr[0];
+ *(uint32_t*)&smp->data.u.ipv6.s6_addr[4] &=
*(uint32_t*)&args[1].data.ipv6.s6_addr[4];
+ *(uint32_t*)&smp->data.u.ipv6.s6_addr[8] &=
*(uint32_t*)&args[1].data.ipv6.s6_addr[8];
+ *(uint32_t*)&smp->data.u.ipv6.s6_addr[12] &=
*(uint32_t*)&args[1].data.ipv6.s6_addr[12];
+ smp->data.type = SMP_T_IPV6;
+ }
+
return 1;
}
@@ -2797,7 +2814,7 @@ static struct sample_conv_kw_list sample_conv_kws = {ILH,
{
{ "length", sample_conv_length,0,NULL, SMP_T_STR,
SMP_T_SINT },
{ "hex",sample_conv_bin2hex, 0,NULL, SMP_T_BIN,
SMP_T_STR },
{ "hex2i", sample_conv_hex2int, 0,NULL, SMP_T_STR,
SMP_T_SINT },
- { "ipmask", sample_conv_ipmask,ARG1(1,MSK4), NULL, SMP_T_IPV4,
SMP_T_IPV4 },
+ { "ipmask", sample_conv_ipmask,ARG2(1,MSK4,MSK6), NULL, SMP_T_ADDR,
SMP_T_IPV4 },
{ "ltime", sample_conv_ltime, ARG2(1,STR,SINT), NULL, SMP_T_SINT,
SMP_T_STR },
{ "utime", sample_conv_utime, ARG2(1,STR,SINT), NULL, SMP_T_SINT,
SMP_T_STR },
{ "crc32", sample_conv_crc32, ARG1(0,SINT), NULL, SMP_T_BIN,
SMP_T_SINT },
--
2.15.1
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Hi Tim, On Sun, Jan 21, 2018 at 01:24:06PM +0100, Tim Düsterhus wrote: > Jarno, > > Am 15.01.2018 um 14:28 schrieb Jarno Huuskonen: > > I'll try to test later (might be couple of days). > > Did you find time, yet? > > > I'll get a compilation error on centos7: > > src/sample.c: In function 'sample_conv_ipmask': > > src/sample.c:1623:3: error: 'for' loop initial declarations are only > > allowed in C99 mode > >for (size_t i = 0; i < 16; i++) > >^ > > src/sample.c:1623:3: note: use option -std=c99 or -std=gnu99 to compile > > your code > > > > So maybe move size_t i outside of loop or unroll loop, something like Yes, please avoid declaring variables inside the for() loop or in the middle of the code, over time they make code maintenance more painful because when you look for the type of a variable you have to scrutinize the code backwards till the beginning of the function instead of quickly glancing at the beginning of each block. > > + *(uint32_t*)&smp->data.u.ipv6.s6_addr[0] &= > > *(uint32_t*)&arg_p[1].data.ipv6.s6_addr[0]; > > + *(uint32_t*)&smp->data.u.ipv6.s6_addr[4] &= > > *(uint32_t*)&arg_p[1].data.ipv6.s6_addr[4]; > > + *(uint32_t*)&smp->data.u.ipv6.s6_addr[8] &= > > *(uint32_t*)&arg_p[1].data.ipv6.s6_addr[8]; > > + *(uint32_t*)&smp->data.u.ipv6.s6_addr[12] &= > > *(uint32_t*)&arg_p[1].data.ipv6.s6_addr[12]; > > > > Thinking about this for some time: I am not sure whether casting a char > pointer to a uint32_t pointer is safe from a language lawyer perspective > in all cases. I certainly can update the patch when Willy tells me his > preference (just move the declaration up or perform the casting). There's no problem here because these ones are word-aligned anyway. There are other such occurrences elsewhere in the code, so don't worry for this one. >From what I've seen till now your patches look fine but like you I'm interested in Jarno's feedback ;-) Cheers, Willy
Re: Show: haproxy-auth-request - HTTP access control using subrequests
On Fri, Jan 19, 2018 at 9:23 PM, Tim Düsterhus wrote: > https://github.com/TimWolla/haproxy-auth-request > > This Lua script reimplements the idea behind nginx' > ngx_http_auth_request_module in haproxy: It allows you to decide whether > an HTTP request should be allowed or not based on the result of an HTTP > request to a backend service. Nice little tool! Quick question though: does this script actually block HAproxy's event loop while waiting for the response from the backend server? (I've read quickly through your article describing the script, but failed to get a clear answer to this question. Moreover looking at the script, unless HAProxy does something "funky" with the Lua interpreter to give the impression of a synchronous API, I would say it does block the HAProxy thread that calls this script.) Thanks, Ciprian.
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Jarno, Am 15.01.2018 um 14:28 schrieb Jarno Huuskonen: > I'll try to test later (might be couple of days). Did you find time, yet? > I'll get a compilation error on centos7: > src/sample.c: In function ‘sample_conv_ipmask’: > src/sample.c:1623:3: error: ‘for’ loop initial declarations are only allowed > in C99 mode >for (size_t i = 0; i < 16; i++) >^ > src/sample.c:1623:3: note: use option -std=c99 or -std=gnu99 to compile your > code > > So maybe move size_t i outside of loop or unroll loop, something like > + *(uint32_t*)&smp->data.u.ipv6.s6_addr[0] &= > *(uint32_t*)&arg_p[1].data.ipv6.s6_addr[0]; > + *(uint32_t*)&smp->data.u.ipv6.s6_addr[4] &= > *(uint32_t*)&arg_p[1].data.ipv6.s6_addr[4]; > + *(uint32_t*)&smp->data.u.ipv6.s6_addr[8] &= > *(uint32_t*)&arg_p[1].data.ipv6.s6_addr[8]; > + *(uint32_t*)&smp->data.u.ipv6.s6_addr[12] &= > *(uint32_t*)&arg_p[1].data.ipv6.s6_addr[12]; > Thinking about this for some time: I am not sure whether casting a char pointer to a uint32_t pointer is safe from a language lawyer perspective in all cases. I certainly can update the patch when Willy tells me his preference (just move the declaration up or perform the casting). Best regards Tim Düsterhus
