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

[common/buf] Add bytespools ownership for buf.Buffer #2336

Merged

Conversation

Vigilans
Copy link
Contributor

V2Ray has a powerful tool that's seldom used (only 2 references in whole project): bytespool.

bytespool has built in 4 pools of size 2048, 8192, 32768 and 131072. The most frequently used buf.Buffer only use the first 2048 byte pool. The other three pools are actually very suitable for payload of variable length that may exceed default 2048 bytes.

bytespool uses Alloc() and Free() pair to manage the bytes buffers. This PR extends buf.Buffer to:

  • introduce a new bytespools ownership, to make use of this two functions to provide a Buffer that can hold more size than 2048
  • provide a new function NewWithSize(size int32), to return a buffer with capacity having at least the given size (Under the hood it will choose one of the four pools to provide a bytes buffer).

@codecov-commenter
Copy link

Codecov Report

Base: 39.21% // Head: 39.19% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (ddb8530) compared to base (83ea1ba).
Patch coverage: 33.33% of modified lines in pull request are covered.

📣 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    #2336      +/-   ##
==========================================
- Coverage   39.21%   39.19%   -0.03%     
==========================================
  Files         617      617              
  Lines       36859    36871      +12     
==========================================
- Hits        14454    14451       -3     
- Misses      20816    20829      +13     
- Partials     1589     1591       +2     
Impacted Files Coverage Δ
common/buf/buffer.go 80.46% <33.33%> (-6.61%) ⬇️
app/router/command/errors.generated.go 0.00% <0.00%> (-100.00%) ⬇️
app/router/command/command.go 36.92% <0.00%> (-3.08%) ⬇️
transport/internet/kcp/sending.go 84.15% <0.00%> (-1.00%) ⬇️
common/log/logger.go 82.35% <0.00%> (+1.47%) ⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Vigilans
Copy link
Contributor Author

@AkinoKaede For your comment in #1919 :

Trojan allows UDP packets up to 4096 bytes, but V2Ray only allows UDP packets up to 2048 bytes, I don't have any opinions about how to deal this problem.

We can construct a scenario that a UDP packet exceeding 2048 bytes would be problematic:

Trojan (Inbound) -> Trojan (Outbound) --- remote ---> Trojan (Inbound) -> Freedom (Outbound)

V2Ray's UDP worker only accepts UDP payload with a 2048 bytes buffer, so for Socks5 and Dokodemo UDP inbound it would not be an issue. However for Trojan inbound this is not the case.

A trojan inbound is a TCP inbound, which can receive more than 2048 bytes of a single UDP payload through TCP stream from another trojan client (not necessarily v2ray trojan client). In turn, v2ray's trojan inbound will write this large payload through dispatched link to the outbound. Let's see how current trojan inbound deals with it:

  1. The author recognizes that a trojan UDP payload may exceed a buf.New() buffer (2048 bytes), so a buf.MultiBuffer is used to store the trojan UDP payload:

    var mb buf.MultiBuffer
    for remain > 0 {
    length := buf.Size
    if remain < length {
    length = remain
    }
    b := buf.New()
    mb = append(mb, b)
    n, err := b.ReadFullFrom(r, int32(length))
    if err != nil {
    buf.ReleaseMulti(mb)
    return nil, newError("failed to read payload").Base(err)
    }
    remain -= int(n)
    }

  2. However, when writing the multi buffer payload to the dispatched link, the buffers in trojan UDP payload is written one by one to the outbound link:

    for _, b := range p.Buffer {
    udpServer.Dispatch(ctx, p.Target, b)
    }

    In V2Ray, a UDP packet payload is represented by a buf.Buffer, not buf.MultiBuffer, so writing the buffer one by one essentially means the large UDP packet is split into multiple small UDP packet to the outbound. At least for freedom outbound it will send the payload in multiple UDP packets, causing misbehavior.

  3. 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.

So how to solve this problem? A good solution would be to respect the semantic that a UDP packet is represented as a buf.Buffer. So, when the UDP packet is larger than 2048 bytes, we just provides a buffer large enough for it.

This functionality is exactly what this PR provides. By using buf.NewWithSize(), we can easily create a managed buffer with capacity big enough for this large UDP packet. So, after this PR merged, it may be used to fix the trojan inbound and outbound UDP processing, as well as possibly your code in packetaddr for trojan.

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 think this change is appropriate. It is ready to be meregd.

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