Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ssl): init peer_id when init tls_multi #432

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pushan01
Copy link
Contributor

@pushan01 pushan01 commented Oct 19, 2023

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.

When openvpn run in UDP server mode, if ssl connections reach the max clients, the next connection would be failed 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 `tils_multi_init`.
@AlbertVeli
Copy link

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.

@cron2
Copy link
Contributor

cron2 commented Oct 22, 2024

Just for reference, the original bug was fixed via

commit 3e30504
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 6dffbf6 in release/2.6)

@AlbertVeli
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants