Hi Willy,
Thanks again for the merge, very glad that it made its way into 2.9.
> I don't see how this is possible:
>
>list_for_each_entry(srv_tlv, >pp_tlvs, list) {
>if (srv_tlv == NULL)
> break;
> For srv_tlv to be NULL, it would mean that you visited (NULL)->list at some
> point, and apparently pp_tlvs is always manipulated with
> LIST_APPEND() only, so I don't see how you could end up with something like
> last->next = (NULL)->list. Are you sure it was not a side effect of a
> debugging session with some temporary code in it ?
> I'd be interested in knowing if you can reproduce it so that we can find the
> root cause (and hopefully address it).
I finally got back to this again. You should be able to reproduce it quite
easily. After removing the 'break' above, you can run the regression tests.
Then, tests which (presumably) use the server settings copy function fail or
timeout.
Probably an even better method is to use the following configuration
'haproxy.cfg':
global
defaults
log global
timeout connect 500ms
timeout client 5000ms
timeout server 5ms
backend dummy
mode tcp
default-server send-proxy-v2 set-proxy-v2-tlv-fmt(0xE1) %[fc_pp_tlv(0xE1)]
server dummy_server 127.0.0.1:2319
frontend dummy
log stdout local0
mode tcp
bind *:2320 accept-proxy
default_backend dummy
Run it with:
gdb --args ./haproxy '-f' './haproxy.cfg'
It should crash immediately with the following seg fault:
Thread 1 "haproxy" received signal SIGSEGV, Segmentation fault.
srv_settings_cpy (srv=srv@entry=0xbed7e0, src=src@entry=0xbebce0,
srv_tmpl=srv_tmpl@entry=0) at src/server.c:2526
2526new_srv_tlv->fmt_string = strdup(srv_tlv->fmt_string);
Overall, I am not too confident with the order of invocations in the server.
Maybe we have some initializations mixed up and unintuitive some dependencies.
> Another point, in make_proxy_line_v2() I'm seeing that you've placed
> everything under "if (strm)". I understand why, it's because you didn't want
> to call build_logline() without a stream. That made > me think "hmmm we
> already have the ability to do that", and I found it, you can use
> sess_build_logline() instead, that takes the session separately, and supports
> an optional stream. That would be useful for health checks for example, since
> they don't have streams. But there's a catch: in function
> make_proxy_line_v2() we don't have the session at the moment, and
> build_logline() extracts it from the stream. Normally during a session
> establishment, the session is present in connection->owner, thus
> remote->owner in make_proxy_line_v2(). I suggest that you give this a
> try to confirm that it works for you, this way we'd be sure that health
> checks can also send the ppv2 fields. But that's not critical given that
> there's no bad behavior right now, it's just that fields will be ignored, so
> this can be done in a subsequent patch.
I looked into it again. I did not see an obvious way to retrieve the current or
pass the session as parameter.
I mean, the easy option, as you said, would be to use remote->owner, which
would require replacing if (stream) with if (remote) to prevent the NULL
dereference.
But we still want to send out TLVs when there is no remote. So maybe I could
change it such that it uses remote->owner when available and otherwise, falls
back to using the session from the stream?
Maybe something along the lines of
struct session *sess = (remote != NULL) ? remote->owner : strm->sess;
replace->data = sess_build_logline(sess, strm, replace->area,
replace->size, _tlv->fmt);
?
It seems to achieve the behavior you described regarding the health checks,
right? If that's alright, I will send the corresponding patch.
Best,
Alexander
-Original Message-
From: Willy Tarreau
Sent: Saturday, November 4, 2023 5:02 AM
To: Stephan, Alexander
Cc: haproxy@formilux.org
Subject: Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated
proxy-v2-options
Alexander,
I now merged your patch with the SMP_VAL_ change, after verifying that the
reg-test is still OK. Thus 2.9-dev9 will contain it. Thanks!
Willy
