search

packetcap / go-pcap

Packet capture library and tools in native go
Apache License 2.0
14 stars 4 forks source link

Avoid race between Close and processMmapPackets #42

Closed milan-zededa closed 1 year ago

milan-zededa commented 1 year ago

When handle is closed, isOpen is atomically set to 0 to stop ReadPacketData from reading more packets. However, processMmapPackets may still continue in the background processing packets remaining in the buffer. Then, as Close unmaps the memory containing the buffer, processMmapPackets may find itself reading from invalid memory regions. The right solution is for Close to wait for an ongoing ReadPacketData to finalize before unmapping the memory. This can be achieved using CAS instructions.

PR addresses the issue reported here: https://github.com/packetcap/go-pcap/issues/41

deitch commented 1 year ago

@milan-zededa this is solid! I saw you wrote a test program here. With this program, it fails with the SIGSEGV on the existing code, and succeeds with your new code?

milan-zededa commented 1 year ago

@milan-zededa this is solid! I saw you wrote a test program here. With this program, it fails with the SIGSEGV on the existing code, and succeeds with your new code?

Yes, correct. On the existing code it fails within few seconds. With patch it was running without issues for hours.

deitch commented 1 year ago

Is there any way to make this a regular go test? I see that it uses an actual interface, which obviously wouldn't work, but could we simulate it? As you showed, the issue is continuing to stream data to the ring buffer after it tries to close. Is that simulatable without terrible headache?

milan-zededa commented 1 year ago

Is that simulatable without terrible headache?

Probably not an easy task. Injecting memory region from unit test is doable but faking content with valid packet data (or even just envelopes) seems like a tons of work (and headache). Plus there is the socket and operations around it that would be difficult to mock.

deitch commented 1 year ago

Probably not an easy task. Injecting memory region from unit test is doable but faking content with valid packet data (or even just envelopes) seems like a tons of work (and headache). Plus there is the socket and operations around it that would be difficult to mock.

Good point. If we could extract the processing from the socket and abstract a lot of it, then we could, but that is not surgery I want us to undertake now. Let's leave it then.

milan-zededa commented 1 year ago

Converting to draft - found one more race condition (less probable).

milan-zededa commented 1 year ago

@deitch @eriknordmark Added one more commit - see the commit description and the content. It turned out that there is another race condition hiding. I will keep my test now running for a very long time.

deitch commented 1 year ago

When it is ready for review, remove it from Draft and comment please.

deitch commented 1 year ago

I approved it. When you are comfortable the testing is complete, comment here, and we can merge it in.

milan-zededa commented 1 year ago

@deitch I tested it thoroughly and it looks good now.

deitch commented 1 year ago

Done. Might want to pull it into eve now