[PR] BUG/MEDIUM: lua: fix invalid return types in hlua_http_msg_get_body
Dear list! Author: vishnu Number of patches: 1 This is an automated relay of the Github pull request: BUG/MEDIUM: lua: fix invalid return types in hlua_http_msg_get_body Patch title(s): BUG/MEDIUM: lua: fix invalid return types in hlua_http_msg_get_body Link: https://github.com/haproxy/haproxy/pull/1427 Edit locally: wget https://github.com/haproxy/haproxy/pull/1427.patch && vi 1427.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/1427.patch | git am - Description: hlua_http_msg_get_body must return either a Lua string or nil. For some HTTPMessage objects, HTX_BLK_EOT blocks are also present in the HTX buffer along with HTX_BLK_DATA blocks. In such cases, _hlua_http_msg_dup will start copying data into a luaL_Buffer until it encounters an HTX_BLK_EOT. But then instead of pushing neither the luaL_Buffer nor `nil` to the Lua stack, the function will return immediately. The end result will be that the caller of the HTTPMessage.get_body() method from a Lua filter will see whatever object was on top of the stack as return value. It may be either a userdata object if HTTPMessage.get_body was called with only two arguments, or the third argument itself if called with three arguments. Hence HTTPMessage.get_body would return either nil, or HTTPMessage body as Lua string, or a userdata objects, or number. This fix ensure that HTTPMessage.get_body() will always return either a string or nil. Fixes #1426 . 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.
[PATCH] CLEANUP: hlua: Remove obsolete branch in `hlua_alloc()`
This branch is no longer required, because the `!nsize` case is handled for any value of `ptr` now. see 22586524e32f14c44239063088a38ccea8abc9b7 see a5efdff93c36f75345a2a18f18bffee9b602bc7b --- src/hlua.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/hlua.c b/src/hlua.c index ac61a3171..0e12614af 100644 --- a/src/hlua.c +++ b/src/hlua.c @@ -11463,9 +11463,6 @@ static void *hlua_alloc(void *ud, void *ptr, size_t osize, size_t nsize) struct hlua_mem_allocator *zone = ud; size_t limit, old, new; - if (unlikely(!ptr && !nsize)) - return NULL; - /* a limit of ~0 means unlimited and boot complete, so there's no need * for accounting anymore. */ -- 2.33.1
[PATCH] DEV: coccinelle: Add realloc_leak.cocci
This coccinelle patch finds locations where the return value of `realloc()` is assigned to the pointer passed to `realloc()`. This calls will leak memory if `realloc()` returns `NULL`. --- dev/coccinelle/realloc_leak.cocci | 6 ++ 1 file changed, 6 insertions(+) create mode 100644 dev/coccinelle/realloc_leak.cocci diff --git a/dev/coccinelle/realloc_leak.cocci b/dev/coccinelle/realloc_leak.cocci new file mode 100644 index 0..c201b808c --- /dev/null +++ b/dev/coccinelle/realloc_leak.cocci @@ -0,0 +1,6 @@ +@@ +expression E; +expression F; +@@ + +* E = realloc(E, F); -- 2.33.1
Re: [PATCH] BUILD/MINOR: atomics: mac arm64 build fix
On Sat, 23 Oct 2021 at 16:50, Willy Tarreau wrote:
>
> Hi David,
>
> On Sat, Oct 23, 2021 at 02:51:59PM +0100, David CARLIER wrote:
> > Hi,
> > Hopefully not too late for the 2.5 release :-)
>
> No worries, and fixes can be merged later anyway. I have some questions
> below.
>
> > From b9c083252bdabf2d0bbfffa1383453cdfd94ab13 Mon Sep 17 00:00:00 2001
> > From: David CARLIER
> > Date: Sat, 23 Oct 2021 14:43:42 +0100
> > Subject: [PATCH] BUILD/MINOR: atomics mac arm64 build fix
> >
> > the inline assembly is invalid for this platform thus falling back
> > to the builtin instead.
> >
> > ---
> > include/haproxy/atomic.h | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/haproxy/atomic.h b/include/haproxy/atomic.h
> > index 3198b381a..29a06c57b 100644
> > --- a/include/haproxy/atomic.h
> > +++ b/include/haproxy/atomic.h
> > @@ -698,7 +698,7 @@ __ha_barrier_atomic_full(void)
> > */
> > #define __ha_cpu_relax() ({ asm volatile("isb" ::: "memory"); 1; })
> >
> > -#if defined(__ARM_FEATURE_ATOMICS) // ARMv8.1-A atomics
> > +#if defined(__ARM_FEATURE_ATOMICS) && !defined(__APPLE__) // ARMv8.1-A
> > atomics
>
> Hmmm that's not expected at all, what error are you getting ? We're
> not doing anything special, so if the __ARM_FEATURE_ATOMICS macro
> is defined, we ought to have them.
Note this works ok with gcc but not clang. But clang is the main
compiler, gcc is secondary on this platform (less well supported).
I tried to convert to llvm asm-ism but seems applying direct
contiguous data values (ie cmp) to registers seems unsupported and
requires more instructions/operands thus
it is safer to just fallback to the builtin which just does that
anyway as you can see :
`
0x100087a3c <+28>: movx3, x5
0x100087a40 <+32>: casp x2, x3, x6, x7, [x8]
0x100087a44 <+36>: eorx8, x3, x5
0x100087a48 <+40>: eorx9, x2, x4
0x100087a4c <+44>: orrx8, x9, x8
0x100087a50 <+48>: cbzx8, 0x100087a58 ; <+56> at
atomic.h:757:2
`
>
> > /* returns 0 on failure, non-zero on success */
> > static forceinline int __ha_cas_dw(void *target, void *compare, const void
> > *set)
> > @@ -738,7 +738,7 @@ static forceinline int __ha_cas_dw(void *target, void
> > *compare, const void *set)
> > return ret;
> > }
> >
> > -#elif defined(__SIZEOF_INT128__) &&
> > defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16) // no ARMv8.1-A atomics but
> > 128-bit atomics
> > +#elif defined(__SIZEOF_INT128__) &&
> > (defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16) || defined(__APPLE__)) // no
> > ARMv8.1-A atomics but 128-bit atomics
>
> I feel uncomfortable with adding this "||" in the expression, as we ought
> to condition this instructions to this. Are you sure this was needed ? Did
> you check the resulting code to make sure a CASP instruction was emitted ?
>
> Something that would be nice is to dump all defines:
>
> gcc -dM -E - < /dev/null
>
> (or use clang instead of gcc).
mac m1 supports rightfully the 128 bits type.
`
#define __SIZEOF_INT128__ 16
`
I just recall on arm64 __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16 is not even
present and indeed when I look up on both on my raspberry and my mac
m1 that is confirmed.
>
> > /* According to
> > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
> > * we can use atomics on __int128. The availability of CAS is defined
> > there:
> > @@ -752,7 +752,7 @@ static forceinline int __ha_cas_dw(void *target, void
> > *compare, const void *set)
> > /* returns 0 on failure, non-zero on success */
> > static __inline int __ha_cas_dw(void *target, void *compare, const void
> > *set)
> > {
> > - return __atomic_compare_exchange((__int128*)target,
> > (__int128*)compare, (const __int128*)set,
> > + return __atomic_compare_exchange((__int128*)target,
> > (__int128*)compare, (__int128*)set,
>
> It's not correct to remove the const there, first because this value is
> not expected to be changed by the emitted instructions, and second because
> we're violating the function's contract which promises not to modify this
> argument. Are you sure it's not just a leftover of your debugging session ?
>
> Thanks,
> Willy
Re: Suggestion
On Thu, Oct 21, 2021 at 11:24:22AM +0200, Steve Hand wrote: > I wasted a day yesterday with this config. This all seemed to work > randomly, sometimes routing to default backend, sometimes routing to the acl > backend. > > The problem was I had 'mode tcp' in global and had to add 'mode http' to the > frontend, otherwise, the acl can't see the headers. > > Whilst I understand now why this was the case I was baffled for many hours > because it just worked but apparently randomly. > > Do you think a warning - "Use of acl rules in tcp mode won't match header > rules" ? Or something would be useful? It can definitely match them and some users make use of that, these just require that you wait for the data to actually be there, because as TCP rules will execute as soon as the connection arrives, the request might very well not be there yet, hence the use of the tcp-request inspect-delay rule. With that said, use of TCP rules for HTTP processing ought to be avoided whenever possible, as it's both difficult to use and often limited (e.g. it makes no sense to recode an HTTP/2 request to HTTP/1 just for the sake of analyzing its contents at the TCP layer). In the past it used to happen when users were combining TCP and HTTP on a same frontend. Nowadays we can upgrade the connection from TCP to HTTP so that HTTP rules are usable even in a TCP frontend, limiting the need for TCP rules to TCP only. With newer versions (starting with 2.5) the doc starts to encourage to migrate to http-request rules for HTTP, and to use tcp-request session for lower layer stuff. My hope is that over the long term we stop seeing that many tcp-request rules for L7 processing. Cheers, Willy
Re: [PATCH] BUILD/MINOR: atomics: mac arm64 build fix
Hi David,
On Sat, Oct 23, 2021 at 02:51:59PM +0100, David CARLIER wrote:
> Hi,
> Hopefully not too late for the 2.5 release :-)
No worries, and fixes can be merged later anyway. I have some questions
below.
> From b9c083252bdabf2d0bbfffa1383453cdfd94ab13 Mon Sep 17 00:00:00 2001
> From: David CARLIER
> Date: Sat, 23 Oct 2021 14:43:42 +0100
> Subject: [PATCH] BUILD/MINOR: atomics mac arm64 build fix
>
> the inline assembly is invalid for this platform thus falling back
> to the builtin instead.
>
> ---
> include/haproxy/atomic.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/haproxy/atomic.h b/include/haproxy/atomic.h
> index 3198b381a..29a06c57b 100644
> --- a/include/haproxy/atomic.h
> +++ b/include/haproxy/atomic.h
> @@ -698,7 +698,7 @@ __ha_barrier_atomic_full(void)
> */
> #define __ha_cpu_relax() ({ asm volatile("isb" ::: "memory"); 1; })
>
> -#if defined(__ARM_FEATURE_ATOMICS) // ARMv8.1-A atomics
> +#if defined(__ARM_FEATURE_ATOMICS) && !defined(__APPLE__) // ARMv8.1-A
> atomics
Hmmm that's not expected at all, what error are you getting ? We're
not doing anything special, so if the __ARM_FEATURE_ATOMICS macro
is defined, we ought to have them.
> /* returns 0 on failure, non-zero on success */
> static forceinline int __ha_cas_dw(void *target, void *compare, const void
> *set)
> @@ -738,7 +738,7 @@ static forceinline int __ha_cas_dw(void *target, void
> *compare, const void *set)
> return ret;
> }
>
> -#elif defined(__SIZEOF_INT128__) &&
> defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16) // no ARMv8.1-A atomics but
> 128-bit atomics
> +#elif defined(__SIZEOF_INT128__) &&
> (defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16) || defined(__APPLE__)) // no
> ARMv8.1-A atomics but 128-bit atomics
I feel uncomfortable with adding this "||" in the expression, as we ought
to condition this instructions to this. Are you sure this was needed ? Did
you check the resulting code to make sure a CASP instruction was emitted ?
Something that would be nice is to dump all defines:
gcc -dM -E - < /dev/null
(or use clang instead of gcc).
> /* According to
> https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
> * we can use atomics on __int128. The availability of CAS is defined there:
> @@ -752,7 +752,7 @@ static forceinline int __ha_cas_dw(void *target, void
> *compare, const void *set)
> /* returns 0 on failure, non-zero on success */
> static __inline int __ha_cas_dw(void *target, void *compare, const void *set)
> {
> - return __atomic_compare_exchange((__int128*)target, (__int128*)compare,
> (const __int128*)set,
> + return __atomic_compare_exchange((__int128*)target, (__int128*)compare,
> (__int128*)set,
It's not correct to remove the const there, first because this value is
not expected to be changed by the emitted instructions, and second because
we're violating the function's contract which promises not to modify this
argument. Are you sure it's not just a leftover of your debugging session ?
Thanks,
Willy
[PATCH] BUILD/MINOR: atomics: mac arm64 build fix
Hi, Hopefully not too late for the 2.5 release :-) Kind regards. 0001-BUILD-MINOR-atomics-mac-arm64-build-fix.patch Description: Binary data
