Leaks memory #29
Comments
|
I acknowledge this existing issue and thank you for the report. I believe that the 'change_last_ack' function is also responsible for this issue, where 'ipstack' checks the received acknowledgments and must deallocate the memory. |
|
I currently don't have access to a profiler. Could you please try this patch? diff --git a/src/stream/tcb.rs b/src/stream/tcb.rs
index 0e7383b..a191a05 100644
--- a/src/stream/tcb.rs
+++ b/src/stream/tcb.rs
@@ -178,13 +178,17 @@ impl Tcb {
let distance = ack.wrapping_sub(self.last_ack);
if matches!(self.state, TcpState::Established) {
- if let Some(i) = self.inflight_packets.iter().position(|p| p.contains(ack)) {
- let mut inflight_packet = self.inflight_packets.remove(i);
- let distance = ack.wrapping_sub(inflight_packet.seq);
- if (distance as usize) < inflight_packet.payload.len() {
- inflight_packet.payload.drain(0..distance as usize);
- inflight_packet.seq = ack;
- self.inflight_packets.push(inflight_packet);
+ loop {
+ if let Some(i) = self.inflight_packets.iter().position(|p| p.contains(ack)) {
+ let mut inflight_packet = self.inflight_packets.remove(i);
+ let distance = ack.wrapping_sub(inflight_packet.seq);
+ if (distance as usize) < inflight_packet.payload.len() {
+ inflight_packet.payload.drain(0..distance as usize);
+ inflight_packet.seq = ack;
+ self.inflight_packets.push(inflight_packet);
+ }
+ } else {
+ break;
}
}
} |
|
Still seems to be leaking at the same place. |
|
Unable to reproduce. Could you please try with https://github.com/narrowlink/ipstack/tree/ManualShutdownIssue and let me know if you can reproduce it? |
|
I've applied the patch and am on the ManualShutdownIssue branch on Linux: Here's the dump file: |
|
Re-ran with tun_wintun, making sure to pull and rebuild: Dump file: heaptrack.tun_wintun.43488.zip |
|
The underlying issue for me seems to be that sometimes Here's some debug logs I added to illustrate this issue: I fixed this with this patch: diff --git a/src/stream/tcb.rs b/src/stream/tcb.rs
index 0e7383b..7d7b94f 100644
--- a/src/stream/tcb.rs
+++ b/src/stream/tcb.rs
@@ -186,6 +186,9 @@ impl Tcb {
inflight_packet.seq = ack;
self.inflight_packets.push(inflight_packet);
}
+ if i != 0 {
+ self.inflight_packets.drain(0..i);
+ }
}
}
I don't know if this is the best solution however. |
|
Thanks! diff --git a/src/stream/tcb.rs b/src/stream/tcb.rs
index 0e7383b..ac90820 100644
--- a/src/stream/tcb.rs
+++ b/src/stream/tcb.rs
@@ -176,6 +176,7 @@ impl Tcb {
}
pub(super) fn change_last_ack(&mut self, ack: u32) {
let distance = ack.wrapping_sub(self.last_ack);
+ self.last_ack = self.last_ack.wrapping_add(distance);
if matches!(self.state, TcpState::Established) {
if let Some(i) = self.inflight_packets.iter().position(|p| p.contains(ack)) {
@@ -187,9 +188,11 @@ impl Tcb {
self.inflight_packets.push(inflight_packet);
}
}
+ self.inflight_packets.retain(|p| {
+ let last_byte = p.seq.wrapping_add(p.payload.len() as u32);
+ last_byte.wrapping_sub(self.last_ack) > 0 && self.seq.wrapping_sub(last_byte) > 0 // only keep packets that are not fully acked
+ });
}
-
- self.last_ack = self.last_ack.wrapping_add(distance);
}
pub fn is_send_buffer_full(&self) -> bool {
self.seq.wrapping_sub(self.last_ack) >= MAX_UNACKCould you please try this patch? |
|
Doesn't seem to work. |
|
Oops, the wrong function was used. I just replaced diff --git a/src/stream/tcb.rs b/src/stream/tcb.rs
index 0e7383b..e771123 100644
--- a/src/stream/tcb.rs
+++ b/src/stream/tcb.rs
@@ -176,6 +176,7 @@ impl Tcb {
}
pub(super) fn change_last_ack(&mut self, ack: u32) {
let distance = ack.wrapping_sub(self.last_ack);
+ self.last_ack = self.last_ack.wrapping_add(distance);
if matches!(self.state, TcpState::Established) {
if let Some(i) = self.inflight_packets.iter().position(|p| p.contains(ack)) {
@@ -187,9 +188,11 @@ impl Tcb {
self.inflight_packets.push(inflight_packet);
}
}
+ self.inflight_packets.retain(|p| {
+ let last_byte = p.seq.wrapping_add(p.payload.len() as u32);
+ last_byte.saturating_sub(self.last_ack) > 0 && self.seq.saturating_sub(last_byte) > 0
+ });
}
-
- self.last_ack = self.last_ack.wrapping_add(distance);
}
pub fn is_send_buffer_full(&self) -> bool {
self.seq.wrapping_sub(self.last_ack) >= MAX_UNACK |
|
That fixes it! I'll close my PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Discovered this issue while working on whisper, a program that uses this crate.
The demo leaks memory, as seen in this
heaptrackoutput (I restarted thenccommand a couple times as occasionally it would stop transferring data, don't know if that's another issue):It seems to be in
ipstack::stream::tcb::Tcb::add_inflight_packet:To reproduce:
tun2example (had to remove therequired-features = ["log"]in the Cargo.toml to get it to compile)sudo ip route add 10.0.10.0/32 dev tun0)cat /dev/zero | nc 10.0.10.0 2048 > /dev/nullto send some traffic through the TUN deviceThe text was updated successfully, but these errors were encountered: