openvpn icon indicating copy to clipboard operation
openvpn copied to clipboard

fix(ssl): init peer_id when init tls_multi

Open pushan01 opened this issue 2 years ago • 3 comments

When openvpn running in UDP server mode, if ssl connections reach the max clients, the next connection will fail in multi_create_instance and the half connection will be close in multi_close_instance, which may lead array m->instances[0] covered unexpectedly and make the first connection interrupt, this patch fix this problem by init peer_id with MAX_PEER_ID in tls_multi_init. Thanks.

pushan01 avatar Oct 19 '23 15:10 pushan01

I encountered a similar issue when setting max-clients to 1. When a second client attempts to connect, the first client (with peer_id 0) that is already connected stops functioning. The issue arises in src/openvpn/multi.c at the line:

m->instances[mi->context.c2.tls_multi->peer_id] = NULL;

in multi_close_instance. If this function is reached through the err: label in multi_create_instance this line should not be executed. In all other cases it should be run (if the surrounding if sentence validates to true). We solved it by adding another bool to multi_close_instance, true if the line should run and false if it should not. I don't know if this is the best solution.

AlbertVeli avatar Oct 22 '24 14:10 AlbertVeli

Just for reference, the original bug was fixed via

commit 3e30504d86f0fe5556acc0cb8e6975c5b2277661 Author: yatta [email protected] Date: Fri Oct 20 01:12:13 2023 +0800

fix(ssl): init peer_id when init tls_multi

(in master, commit 6dffbf6a2 in release/2.6)

cron2 avatar Oct 22 '24 14:10 cron2

Yes, this is a nicer solution to initiate peer_id to MAX_PEER_ID, then the if sentence before removing the connecting mi from m will work as intended. But it didn't solve my case, peer_id needs to be initiated in one more place, in multi.c multi_create_instance right after the line mi->context.c2.tls_multi->multi_state = CAS_PENDING; add mi->context.c2.tls_multi->peer_id = MAX_PEER_ID; This solves my problem in v2.5.11. But the same problem seems to be in the latest 2.6.x too when I glance at the source. You may want to add this line in your pull request too.

Thanks, finding this pull request made the solution much cleaner.

AlbertVeli avatar Oct 23 '24 07:10 AlbertVeli