Bug 2367

Summary: BlockAckManager does not remove iterators to freed items
Product: ns-3 Reporter: Alexander Krotov <krotov>
Component: wifiAssignee: sebastien.deronne
Status: RESOLVED FIXED    
Severity: normal CC: ns-bugs, tomh
Priority: P3    
Version: ns-3.25   
Hardware: All   
OS: All   
Attachments: Proposed patch

Description Alexander Krotov 2016-04-07 11:35:42 UTC
Created attachment 2374 [details]
Proposed patch

When packets are acknowledged, retransmissions are not removed. In most cases they don't exist at all or are removed later by BlockAckManager::CleanupBuffers, but it means CleanupBuffers accessed memory of already deleted packets. Valgrind reports use-after-free in some of my experiments and sometimes they crash because of this. Attached is a patch to fix this problem by removing retransmissions for all acknowledged packets.
Comment 1 sebastien.deronne 2016-04-16 03:34:18 UTC
I was expecting that BlockAckManager::RemovePacket was doing the job.

Could you attach your test case or traces showing that packets are not always removed?

Maybe the problem is that we do not properly call BlockAckManager::RemovePacket at some  places.
Comment 2 sebastien.deronne 2016-04-20 16:20:43 UTC
Could you please clarify a bit this bug?
For me it is not clear whether something is wrong or not.
Comment 3 sebastien.deronne 2016-06-09 04:44:37 UTC
Still no feedback received.
I'll reject this bug unless I get a reply before the upcoming release.
Comment 4 sebastien.deronne 2016-07-04 15:47:39 UTC
Waiting example from Ioannis
Comment 5 sebastien.deronne 2016-10-09 03:57:43 UTC
Still test case needed...
In the meantime, I will test the proposed patch to check whether this hurts anything, or whether this indeed adds some protection somehow.
Comment 6 sebastien.deronne 2017-02-19 05:47:38 UTC
I made some tests with this patch and I do not see any problem. Since I do not have any further objection to this patch which seems fixing an issue for Alexander, I propose to deliver it to the mainstream.
Comment 7 Tom Henderson 2017-02-19 09:06:30 UTC
(In reply to sebastien.deronne from comment #6)
> I made some tests with this patch and I do not see any problem. Since I do
> not have any further objection to this patch which seems fixing an issue for
> Alexander, I propose to deliver it to the mainstream.

+1; sometimes bugs like these are hard to reproduce with a mainline code test
Comment 8 sebastien.deronne 2017-02-20 13:12:48 UTC
pushed in changeset 12707:d4c60d43eb4e