Bugzilla – Bug 833
OnOffApplication with PacketSocket: sniffs all traffic
Last modified: 2010-04-14 16:59:37 EDT
Following up on bug 832, I was looking at my code to figure out why my PacketSocket's receive queue was filling, to see if I was missing some packets. Then I realized, I do not use PacketSocket's for receiving at all. I only use OnOffApplication with a PacketSocket to send some L2 traffic sporadically, but I do not want to receive it (it's just some keepalive frames). The problem is that OnOffApplication calls Bind() on the created socket. Additionally, Bind() (the no-arguments version) with a PacketSocket configures it to receive ALL traffic on the network/interface! Of course the data is eventually dropped, but it is a pointless computing overhead that surely could be avoided... I'm not sure where the bug fix belongs. Perhaps OnOffApplication should not call Bind() on the socket, or perhaps PacketSocket should not register a listener in the Node unless it has a non-null receive callback.
Created attachment 779 [details] patch This patch gets around the problem without needing any refactoring. It just checks if the receive callback is null before queuing the packet in the rx queue. It's not perfect, but...
Created attachment 782 [details] patch I think this is the most elegant patch. Calls ShutDownSend and ShutDownRecv where appropriate. No trace files change with the patch.
(In reply to comment #2) > Created an attachment (id=782) [details] > patch > > I think this is the most elegant patch. Calls ShutDownSend and ShutDownRecv > where appropriate. No trace files change with the patch. I like this one too.
(In reply to comment #2) > Created an attachment (id=782) [details] > patch > > I think this is the most elegant patch. Calls ShutDownSend and ShutDownRecv > where appropriate. No trace files change with the patch. What happens if OnOffApplication uses TCP sockets? It seems like this might not work. For example, after applying this patch and running examples/csma/csma-star.cc, the tcpdumps show a bunch of SYN ACKS coming in to node 0, over and over again.
(In reply to comment #4) > (In reply to comment #2) > > Created an attachment (id=782) [details] [details] > > patch > > > > I think this is the most elegant patch. Calls ShutDownSend and ShutDownRecv > > where appropriate. No trace files change with the patch. > > What happens if OnOffApplication uses TCP sockets? It seems like this might > not work. For example, after applying this patch and running > examples/csma/csma-star.cc, the tcpdumps show a bunch of SYN ACKS coming in to > node 0, over and over again. Maybe it's a bug in TCP's implementation of ShutdownSend|Recv?
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > > Created an attachment (id=782) [details] [details] [details] > > > patch > > > > > > I think this is the most elegant patch. Calls ShutDownSend and ShutDownRecv > > > where appropriate. No trace files change with the patch. > > > > What happens if OnOffApplication uses TCP sockets? It seems like this might > > not work. For example, after applying this patch and running > > examples/csma/csma-star.cc, the tcpdumps show a bunch of SYN ACKS coming in to > > node 0, over and over again. > > Maybe it's a bug in TCP's implementation of ShutdownSend|Recv? I believe I understand why the patch is causing a difference in the csma-star regression, since it uses TCP sockets with OnOffApplication. By calling ShutdownRecv which sets m_shutdownRecv in TcpSocketImpl, packets that make it to TcpSocketImpl::ForwardUp are not processed any further; therefore, ProcessEvent (which updates state) and ProcessPacketAction (which will handle and process the acks among all the other things) are not called. I guess maybe this is something that should be addressed if shutting down recv is only supposed to do so for the app and not the socket. I am still confused about the bug. I'm not saying it is invalid or that the proposed solution is incorrect. I just don't understand it. For example, the SetRecvCallback in OnOffApplication is never set anyway, so I don't know why calling ShutdownRecv is needed. And calling ShutdownSend in PacketSink, I'm not sure what that does for you either, since PacketSink never makes any kind of Send call. I think I must be missing something. Sorry for more questions than answers :)
When you use OnOffApplication with a PacketSocket, this happens: 1- OnOffApplication calls Bind() on the socket, with no arguments; 2- PacketSocket's Bind() implementation calls Node::RegisterProtocolHandler, to receive ALL packets and send them to PacketSocket::ForwardUp; 3- PacketSocket::ForwardUp stores received packets in a "receive queue"; I am just saying that it is strange that an application that supposedly only sends traffic is configuring the socket to sniff traffic and store in a queue. Memory and computation bloat. ShutDownRcv() is one possible solution...
(In reply to comment #7) > When you use OnOffApplication with a PacketSocket, this happens: > > 1- OnOffApplication calls Bind() on the socket, with no arguments; > 2- PacketSocket's Bind() implementation calls Node::RegisterProtocolHandler, > to receive ALL packets and send them to PacketSocket::ForwardUp; > 3- PacketSocket::ForwardUp stores received packets in a "receive queue"; > > I am just saying that it is strange that an application that supposedly only > sends traffic is configuring the socket to sniff traffic and store in a queue. > Memory and computation bloat. > > ShutDownRcv() is one possible solution... Why was my brain reading PacketSocket and thinking PacketSink over and over again? I see what you are going for here. So the remaining issue is just calling ShutdownSend in OnOff when using tcp socket. Will have a look and see if I can come up with a fix, but I agree (finally) patch looks good. +1 after fixing tcp issue.
changeset 151afb65c7e8