Reassembly: pages not released to pageCache correctly due to mistake in doubly-linked list traverse
- Dominant language
- Go
- Stars
- 6.8k
- Forks
- 1.2k
- PR merge metrics
- No merged PRs in 30d
Description
In the `reassembly` package, a `pageCache` is used manage free pages pool for reusing the memory objects across sessions. When a session is finishing/closing, each page the session borrowed from the pool by the `next()` method should be returned/released to this pool by the `replace()` method. These pages formed a doubly linked list when they were used within a session.
In [reassembly/tcpassembly.go#L1198](https://github.com/google/gopacket/blob/master/reassembly/tcpassembly.go#L1198), `Assembler` 's `closeHalfConnection()` function is implemented like this:
for p := half.first; p != nil; p = p.next {
// FIXME: it should be already empty
a.pc.replace(p)
half.pages--
}
Meanwhile, in the `replace()` method in [reassembly/memory.go#L109](https://github.com/google/gopacket/blob/master/reassembly/memory.go#L109):
p.next = nil
As a result, in the `closeHalfConnection()` loop, when the `half.first` being released, its `next` will become `nil`, combining with the iteration of using `p.next` directly, result in the doubly-linked list broken; the rest of pages in this list won't be iterated anymore and lose chance of releasing to `pageCache`'s `free` pool.
This could be simply fixed by below implementation:
var next *page = nil
for p := half.first; p != nil; p = next {
// FIXME: it should be already empty
next = p.next
a.pc.replace(p)
half.pages--
}
The issue could be easily verified with a TCP session that contains several HTTP transactions while each transaction need reassembly. You will see for the session, the `next()` will be invoked much more times than `replace()` -- `replace()` will only invoke for one time even if `half.next.next != nil`. By the same traffic, you can verify that the above proposed fix would work well.
Contributor guide
Assessment
This issue has not been assessed yet.