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

proxy/trojan: fix writing UDP packet #2446

Merged
merged 2 commits into from
May 23, 2023
Merged

Conversation

dyhkwong
Copy link
Contributor

@dyhkwong dyhkwong commented Apr 1, 2023

#2440 #1795

Every UDP packet should have some headers.

@kslr kslr requested a review from xiaokangwang April 1, 2023 12:27
@Vigilans
Copy link
Contributor

Vigilans commented Apr 3, 2023

Ref #2336 (comment), maybe related?

  1. In fact, the author made attempt to deal with the issue mentioned above in trojan outbound:

    func (w *PacketWriter) WriteMultiBuffer(mb buf.MultiBuffer) error {
    b := make([]byte, maxLength)
    for !mb.IsEmpty() {
    var length int
    mb, length = buf.SplitBytes(mb, b)
    if _, err := w.writePacket(b[:length], w.Target); err != nil {
    buf.ReleaseMulti(mb)
    return err
    }
    }
    return nil
    }

    It 1) Creates a buffer of length 8192 (maxLength = 8192); 2) Read the MultiBuffer 8192 bytes by 8192 bytes, then encode each (at most 8192 bytes) buffer into trojan UDP payloads.

    This in a sense fixes the problem that trojan inbound writes the UDP payload 2048 bytes by 2048 bytes. Since a UDP payload will not exceed 8192 bytes (max 4096 bytes as you mentioned), the split UDP packet in trojan inbound will be reunited in trojan outbound.

    However, even if scoped in trojan inbound -> trojan outbound, this solution is still problematic. How to ensure that the passed mb buf.MultiBuffer only contains payload of 1 UDP packet? In fact this is not guaranteed, so the UDP packet in outbound may contains payload of more than 1 UDP packets from inbound, causing misbehavior.

@dyhkwong
Copy link
Contributor Author

dyhkwong commented Apr 7, 2023

Ref #2336 (comment), maybe related?

Indeed. So, this PR also can't handle UDP payload larger than 2048 bytes. Can this be fixed by modifying Trojan's PacketWriter and PacketReader to make use of buf.NewWithSize(), like packetstream in #2337?

@Vigilans
Copy link
Contributor

Vigilans commented Apr 7, 2023

Ref #2336 (comment), maybe related?

Indeed. So, this PR also can't handle UDP payload larger than 2048 bytes. Can this be fixed by modifying Trojan's PacketWriter and PacketReader to make use of buf.NewWithSize(), like packetstream in #2337?

I think it will fix. I remembered that I was referencing Trojan's PacketWriter and PacketReader to implement packetstream ones, though I did not have the time and usecase to fix and test for trojan.

proxy/trojan/protocol.go Outdated Show resolved Hide resolved
proxy/trojan/protocol_test.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2023

Codecov Report

Patch coverage: 42.85% and no project coverage change.

Comparison is base (9b52628) 39.05% compared to head (5b6e0fd) 39.06%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2446   +/-   ##
=======================================
  Coverage   39.05%   39.06%           
=======================================
  Files         617      617           
  Lines       37036    37028    -8     
=======================================
- Hits        14465    14464    -1     
+ Misses      20981    20970   -11     
- Partials     1590     1594    +4     
Impacted Files Coverage Δ
proxy/trojan/protocol.go 42.22% <42.85%> (-3.24%) ⬇️

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@xiaokangwang xiaokangwang left a comment

Choose a reason for hiding this comment

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

I have summarily approved this merge request. Thanks for your contribution.

It is worth noting that UDP packet requires segmentation is not widely supported and should be considered edge case.

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.

4 participants