Attention is currently required from: its_Giaan, ordex, plaisthos.
cron2 has posted comments on this change. (
http://gerrit.openvpn.net/c/openvpn/+/434?usp=email )
Change subject: Adapt socket handling to support listening on multiple sockets
......................................................................
Patch Set 10: Code-Review-2
(18 comments)
Patchset:
PS10:
Getting there, and except for the `bind_dev` these are minor nitpicks in the
source (but worth fixing if we do another round anyway).
In the review, I seem to have overlooked something, though - applying this on
top of current master breaks `--fragment` servers - on the first connecting
client, *boom*
```
2024-12-29 23:34:56 us=142019 Initialization Sequence Completed
2024-12-29 23:35:05 us=857683 Connection Attempt MULTI: multi_create_instance
called
2024-12-29 23:35:05 us=857852 2001:608:0:814::f000:21 Re-using SSL/TLS context
2024-12-29 23:35:05 us=857903 2001:608:0:814::f000:21 WARNING: if you use
--mssfix and --fragment, you should use the "mtu" flag for both or none of of
them.
2024-12-29 23:35:05 us=858101 2001:608:0:814::f000:21 Control Channel MTU parms
[ mss_fix:0 max_frag:0 tun_mtu:1250 tun_max_mtu:0 headroom:126 payload:1600
tailroom:126 ET:0 ]
Program received signal SIGSEGV, Segmentation fault.
0x000055555558121e in get_link_socket_info (c=0x5555556c6450)
at /home/gert/t_server.git/src/openvpn/forward.h:324
324 return &c->c2.link_sockets[0]->info;
(gdb) where
#0 0x000055555558121e in get_link_socket_info (c=0x5555556c6450)
at /home/gert/t_server.git/src/openvpn/forward.h:324
#1 do_init_fragment (c=0x5555556c6450) at init.c:3801
#2 init_instance (c=c@entry=0x5555556c6450, env=<optimized out>,
flags=flags@entry=10) at init.c:4736
#3 0x0000555555581e91 in inherit_context_child (
dest=dest@entry=0x5555556c6450, src=src@entry=0x7fffffffc848,
ls=ls@entry=0x55555569fb00) at init.c:4977
#4 0x00005555555999e0 in multi_create_instance (m=m@entry=0x7fffffffc780,
real=real@entry=0x7fffffffc3c0, ls=ls@entry=0x55555569fb00) at multi.c:777
#5 0x00005555555928b2 in multi_get_create_instance_udp (m=0x7fffffffc780,
floated=floated@entry=0x7fffffffc6cf, ls=ls@entry=0x55555569fb00)
at mudp.c:260
#6 0x000055555559a4dd in multi_process_incoming_link (
m=m@entry=0x7fffffffc780, instance=instance@entry=0x0,
mpp_flags=mpp_flags@entry=5, ls=ls@entry=0x55555569fb00) at multi.c:3356
#7 0x00005555555935f2 in multi_process_io_udp (m=0x7fffffffc780,
sock=0x55555569fb00) at mudp.c:387
#8 tunnel_server_udp (top=top@entry=0x7fffffffda10) at mudp.c:523
#9 0x000055555559b2c4 in tunnel_server (top=top@entry=0x7fffffffda10)
at multi.c:4168
(gdb) print &c->c2.link_sockets[0]->info
Cannot access memory at address 0x0
(gdb) print &c->c2.link_sockets[0]
$1 = (struct link_socket **) 0x0
```
so it definitely looks like "socket not initialized", though I have no idea why.
File src/openvpn/init.c:
http://gerrit.openvpn.net/c/openvpn/+/434/comment/721e4eca_fe894283 :
PS9, Line 4054: {
The `if()` should be outside and the `for()` inside - the condition does not
depend on `i`.
http://gerrit.openvpn.net/c/openvpn/+/434/comment/1a384971_29102103 :
PS9, Line 4062: {
same thing.
http://gerrit.openvpn.net/c/openvpn/+/434/comment/89309b77_8f3abbb8 :
PS9, Line 4786: get_link_socket_info(c));
Should this be a per-socket thing? IPv4 vs. IPv6 will change overhead so the
`frame` calculation will be wrong for all sockets != 0.
Maybe a comment to that extent ("for multiple listening sockets, this will not
be correct for these cases, but everything but TLS should no longer be used
anyway")?
http://gerrit.openvpn.net/c/openvpn/+/434/comment/a405a93a_e360204a :
PS9, Line 4991: &dest->gc);
if line length permits, can this be unwrapped?
File src/openvpn/mtcp.c:
http://gerrit.openvpn.net/c/openvpn/+/434/comment/040f4e94_88362f91 :
PS9, Line 271:
&c->c2.link_sockets[i]->ev_arg);
the diff looks as if there are tabs at the beginning of this line?
File src/openvpn/mudp.c:
http://gerrit.openvpn.net/c/openvpn/+/434/comment/0490e7ea_822b07fb :
PS9, Line 388: sock);
please unwrap
File src/openvpn/multi.c:
http://gerrit.openvpn.net/c/openvpn/+/434/comment/7963d5d0_1f026e7c :
PS9, Line 3336: {
I think this could be wrapped before `const unsigned int mpp_flags`
File src/openvpn/socket.c:
http://gerrit.openvpn.net/c/openvpn/+/434/comment/c450a732_4bec3d43 :
PS9, Line 1037:
another blank line
http://gerrit.openvpn.net/c/openvpn/+/434/comment/a223f45e_43302346 :
PS9, Line 1875: sock->bind_dev = o->bind_dev;
where did that go to? another rebase accident? *red flag*
http://gerrit.openvpn.net/c/openvpn/+/434/comment/03786af0_25ca4cba :
PS9, Line 1019: socket_descriptor_t sd;
we shouldn't delete blank lines unrelated to actual changes (minor nit)
http://gerrit.openvpn.net/c/openvpn/+/434/comment/9c9ce7e2_01384c14 :
PS9, Line 1854: int mode)
please unwrap
http://gerrit.openvpn.net/c/openvpn/+/434/comment/b9a7826e_aead3ffe :
PS9, Line 1863: sock->remote_port = o->ce.remote_port;
not sure this change (remove the local variables `remote_host` and
`remote_port`) is a good thing as part of this patchset. It's
cleanup/refactoring, and unrelated, but makes this diff larger than it should
be.
So I'd prefer to not do this set of changes.
http://gerrit.openvpn.net/c/openvpn/+/434/comment/bff14e2e_61aafce1 :
PS9, Line 1919: sock->proxy_dest_port = o->ce.remote_port;
this
http://gerrit.openvpn.net/c/openvpn/+/434/comment/ac638e3f_3c683fd8 :
PS9, Line 1924:
spurious blank line
http://gerrit.openvpn.net/c/openvpn/+/434/comment/8bdd62db_a3c5abdd :
PS9, Line 1931: sock->proxy_dest_port = o->ce.remote_port;
this
http://gerrit.openvpn.net/c/openvpn/+/434/comment/c27c8614_67874b24 :
PS9, Line 1936: sock->remote_port = o->ce.remote_port;
and this
http://gerrit.openvpn.net/c/openvpn/+/434/comment/731d7cd9_eba0b68a :
PS9, Line 2301: /* Always restore the saved signal
--register/throw_signal will handle priority */
you lost a ` ` here after `--` (this is not a command line option `--register`)
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/434?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ia0a889e800f0b36aed770ee36e31afeec5df6084
Gerrit-Change-Number: 434
Gerrit-PatchSet: 10
Gerrit-Owner: ordex <[email protected]>
Gerrit-Reviewer: cron2 <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: its_Giaan <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: its_Giaan <[email protected]>
Gerrit-Attention: ordex <[email protected]>
Gerrit-Comment-Date: Sun, 29 Dec 2024 22:40:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
