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

nd_light: Fix use after free bug in neighbor solicitation. #664

Merged
merged 2 commits into from Dec 4, 2015

Conversation

Projects
None yet
2 participants
@eugeneia
Member

eugeneia commented Nov 17, 2015

This fixes a “use after free” bug (#643) in nd_light. The bug only surfaced when using the Solarflare driver but is indeed unrelated. I want to highlight a pattern that I think is close to a whole class of bugs:

 function nd_light:stop ()
    packet.free(self._next_hop.packet)
+   self._next_hop.packet = nil
    packet.free(self._sna.packet)
+   self._sna.packet = nil
 end

So what I did here previously (I wrote the bug in the first place) is to free “static” packets when the app is stopped. In itself this is important as to not leak memory. The nd_light app specifically launches a timer that will send self._next_hop.packet periodically. What happens when you start nd_light and stop it right away? The timer gets activated, the packet is freed, the timer runs, attempts to clone a packet which was already freed: wham, segmentation fault.

As a response I added explicit statements unbinding the slots that hold pointers to freed packets. I think this is an important lesson learned: Do not keep invalid pointers around. If you free a packet held in a variable that can be accessed outside the lexical scope (for instance an object slot), null it right away. That way you will get a meaningful error message instead of a segmentation fault.

@lukego

This comment has been minimized.

Show comment
Hide comment
@lukego

lukego Dec 1, 2015

Member

This fixes a bug so I am merging it.

However, I disagree on the plan for handling this class of error. My position is that timers in apps should be implemented locally inside the app and not with a global timer whose lifetime is independent of the app instance.

This is the timer implementation that makes sense to me, potentially with some library support:

local deadline
function push ()
   if deadline == nil or deadline <= engine.now() then
      ... timer code ...
      deadline = (deadline or engine.now()) + conf.interval
   end
   ... other push method app logic ...
end

This is simple and local and has no risk of the timer callback outliving the app.

Or?

Member

lukego commented Dec 1, 2015

This fixes a bug so I am merging it.

However, I disagree on the plan for handling this class of error. My position is that timers in apps should be implemented locally inside the app and not with a global timer whose lifetime is independent of the app instance.

This is the timer implementation that makes sense to me, potentially with some library support:

local deadline
function push ()
   if deadline == nil or deadline <= engine.now() then
      ... timer code ...
      deadline = (deadline or engine.now()) + conf.interval
   end
   ... other push method app logic ...
end

This is simple and local and has no risk of the timer callback outliving the app.

Or?

lukego added a commit to lukego/snabb that referenced this pull request Dec 1, 2015

@eugeneia eugeneia merged commit 0fdafdb into snabbco:master Dec 4, 2015

1 check passed

davos-eugeneia/snabb-nfv-test Linux davos 3.13.0-41-generic x86_64 Intel(R) Xeon(R) CPU E5-2603 v2 @ 1.80GHz / eugeneia/snabb-nfv-test
Details

dpino pushed a commit to dpino/snabb that referenced this pull request Dec 9, 2016

Merge pull request #664 from Igalia/unreadme
Removed README. prefix from docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment