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

Shadowsocks2022 Client Implementation Improvements #2770

Merged
merged 7 commits into from Nov 24, 2023

Conversation

xiaokangwang
Copy link
Contributor

  1. Added UDP Replay Detection
  2. Added UDP Processor State Cache
  3. Added More Detailed Output for Time Difference Error

1. Added UDP Replay Detection
2. Added UDP Processor State Cache
3. Added More Detailed Output for Time Difference Error
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023

Codecov Report

Attention: 143 lines in your changes are missing coverage. Please review.

Comparison is base (62428d8) 36.97% compared to head (2eb6ad8) 36.87%.

Files Patch % Lines
proxy/shadowsocks2022/client_session.go 0.00% 91 Missing ⚠️
proxy/shadowsocks2022/encoding.go 0.00% 24 Missing ⚠️
proxy/shadowsocks2022/udp_aes.go 0.00% 20 Missing ⚠️
common/crypto/auth.go 30.00% 5 Missing and 2 partials ⚠️
proxy/shadowsocks2022/client.go 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2770      +/-   ##
==========================================
- Coverage   36.97%   36.87%   -0.10%     
==========================================
  Files         671      671              
  Lines       39685    39792     +107     
==========================================
  Hits        14673    14673              
- Misses      23391    23497     +106     
- Partials     1621     1622       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of contention on sessionMap. The lock is taken once per packet on the encode path, and twice per packet on the decode path.

proxy/shadowsocks2022/udp_aes.go Outdated Show resolved Hide resolved
1. Replaced Mutex with RWMutex for reduced lock contention
2. Added per server session tracking of decryption cache and anti-replay window
@xiaokangwang
Copy link
Contributor Author

I have:

  1. Replaced Mutex with RWMutex for reduced lock contention
  2. Added per server session tracking of decryption cache and anti-replay window

It is ready for another round of review.

proxy/shadowsocks2022/client_session.go Outdated Show resolved Hide resolved
}
for _, key := range expiredServerSessionID {
delete(c.trackedServerSessionID, key)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to delete k-v pairs from a map while iterating over it. There's no need to cache the keys to delete in a separate slice.

The replay window during a server session change is only 60 seconds. Why does it cache the state for 125 seconds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have adjusted this value to 65 seconds and removed mark and remove for Server Session ID.

1. Adjust server session track time
2. Increase per session buffer to 128
1. Fix client crash when EIH is not enabled
2. Fix client log contains non-human-friendly content
1. Remove mark and remove for trackedSessions
@xiaokangwang
Copy link
Contributor Author

I have got another round of edit. Please have a look and please let me know if there is any other suggestions before you consider this implementation spec compliant.

Copy link
Contributor

@database64128 database64128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing my feedback. LGTM now.

1. Fixed packet size uint16 overflow issue
1. Fixed packet size uint16 overflow issue
@xiaokangwang xiaokangwang merged commit b6da3e8 into v2fly:master Nov 24, 2023
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants