Fix buger/goreplay#1095

[monrax]

The copySlice function inside tcp_packet.go wouldn't copy elements from the from slices to the to slice, even when the to slice has "space" to spare due to its cap being larger than the totalLen:

func copySlice(to []byte, skip int, from ...[]byte) ([]byte, int) {
    var totalLen int
    for _, s := range from {
        totalLen += len(s)
    }
    totalLen += skip

    if cap(to) < totalLen {
        diff := totalLen - cap(to)
        to = append(to, make([]byte, diff)...)
    }

    for _, s := range from {
        skip += copy(to[skip:], s)
    }

    return to, skip
}

This is caused because Go's copy function used in copySlice will copy a number of elements "which will be the minimum of len(src) and len(dst).". For the built-in copy function to copy elements into a slice, the destination slots must be initialized for this slice, not just allocated in the underlying array. In other words, len must be used instead of cap to allow copy to work properly.

This PR closes #1095, which is an example of the effects of this issue: when mirroring packets using VXLAN, the raw payloads obtained using the vxlan engine can be large due to encapsulation. This has the effect of giving the tmp slice a large cap when PacketData() is called inside tcp_message.go. Then, the to slice is never resized in copySlice and only the first packet is read, without the rest (e.g. no response body, only headers are read).

[buger]

What a find!

I never thought about this nuance with copy 🙈

Created small playground to test it https://go.dev/play/p/g8DZRqwg_rw

Original intent was to ensure that slice memory gets re-use, and we extend it only when its capacity is not enough. After thinking for a while, your fix performance wise should be very similar, because append should internally do the slice growth if it is required.

[monrax]

Yeah, took me a little while to understand what was going on after @BridgeWind pointed me in the right direction.

I'm looking at the tests and there's one that's failing: the TestMessageTimeoutReached test in tcp_test.go. I believe this test issue is not related to this fix, since Data() is never called in TestMessageTimeoutReached. Instead, it is more of a race condition —if you comment out the time.Sleep(time.Second * 2) line, the test passes without any issues.

Update 1: I think the issue with the test is the duration passed to the NewMessageParser constructor. Looks like in those 2 seconds the Emit method of the parser MessageParser is being called, which removes the message at the top of the parser.m stack to put it in the parser.messages channel. Considering that there's only a 10-millisecond window before expiration, I'd say Emit is being called from timer.

Update 2: Yup, that was it. I've changed the expiration to 100 milliseconds and the sleeping time to 20 milliseconds. Please, let me know if this is looks OK to you @buger

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[buger]

👍🏻