Hi,
I should have added that as written the fault is with get_dco_peer stats().
It takes a pointer to "struct context" and dereferences a couple of layers
below it. The caller has no idea what it wants to see set in c. So, until
that is changed on all platforms (right now windows and linux), too much to
ask the caller to check that a valid dco handle is available.
Selva
On Mon, Mar 27, 2023 at 4:42 PM Selva Nair <selva.n...@gmail.com> wrote:
>
>
> On Mon, Mar 27, 2023 at 4:30 PM Antonio Quartulli <a...@unstable.cc> wrote:
>
>> Hi,
>>
>> On 27/03/2023 19:12, selva.n...@gmail.com wrote:
>> > From: Selva Nair <selva.n...@gmail.com>
>> >
>> > We persist peer-stats when restarting, but an early restart
>> > before open_tun results in a segfault in dco_get_peer_stats().
>> > To reproduce, trigger a TLS handshake error due to lack of common
>> > protocols, for example.
>> >
>> > Fix by checking that tuntap is defined before dereferencing it.
>> >
>> > Signed-off-by: Selva Nair <selva.n...@gmail.com>
>>
>> Nice catch! Thanks a lot!
>>
>> > ---
>> > I'm not entirely sure this is the right place to fix this.
>> > Or is it the caller at fault exercising dco_get_peer_stats()
>> > when tuntap is not set?
>>
>> Indeed that was my assumption.
>>
>> Does somebody have the stacktrace?
>> I wanted to check where this call is coming from exactly.
>>
>
> I didn't save the stacktrace.. The call comes through
>
> persist_client_stats(c); (openvpn.c ~ line 100)
>
> which gets called on exiting the main eventloop for restart. Without it we
> used to lose
> the statistics on restart.
>
> Imho it'd be reasonable if the caller could check if we have a device
>> before invoking any DCO API.
>>
>
> Sure, I would think that the caller of dco_get_peer_stats() should just
> pass the dco handle to it
> and get the stats back. It's debatable who should check whether
> the dco handle is valid or not.
>
> But this may not be the time and place for a refactor. As dco is young, I
> think it can mature in 2.7..
>
> Selva
>
>
>
>>
>> Cheers,
>>
>> >
>> > src/openvpn/dco_linux.c | 5 +++++
>> > 1 file changed, 5 insertions(+)
>> >
>> > diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
>> > index 317f9dc0..41540c0f 100644
>> > --- a/src/openvpn/dco_linux.c
>> > +++ b/src/openvpn/dco_linux.c
>> > @@ -975,6 +975,11 @@ dco_get_peer_stats(struct context *c)
>> > uint32_t peer_id = c->c2.tls_multi->dco_peer_id;
>> > msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id);
>> >
>> > + if (!c->c1.tuntap)
>> > + {
>> > + return 0;
>> > + }
>> > +
>> > dco_context_t *dco = &c->c1.tuntap->dco;
>> > struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco,
>> OVPN_CMD_GET_PEER);
>> > struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_GET_PEER);
>>
>> --
>> Antonio Quartulli
>>
>
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
