Race condition when instantiating Packet subclasses in multiple threads

[alirez]

Brief description

When Packet subclasses are used in multiple threads, there is a potential for a race condition.

Environment

• Scapy version: 2.4.3rc1.dev77
• Python version: 3.7.3rc1
• Operating System: Linux (Debian buster)

How to reproduce

This issue will only be triggered with a very specific timing, and is hard to reproduce. But in theory it may present itself any time you have multiple threads concurrently instantiating any of the Packet subclasses. To increase the chances, this script can be used:

import threading
from scapy.all import UDP, IP, TCP


def f(b):
    b.wait()
    p = (IP(src='1.1.1.1', dst='2.2.2.2', flags=5, ttl=100, id=1, proto=17,
            len=100, chksum=1)/('a' * 10000),
         TCP(sport=1000, dport=2000, window=1000, seq=1000, ack=1000,
             chksum=1, flags=0)/('a' * 10000),
         UDP(sport=1000, dport=2000)/('a' * 10000))
    return p


ts = list()
n = 1000
b = threading.Barrier(parties=n)
for i in range(n):
    ts.append(threading.Thread(target=f,
                               args=(b,)))

for t in ts:
    t.start()

for t in ts:
    t.join()

Running this code in a loop will likely trigger the exception.

for i in `seq 100`; do echo $i; python scapy-race-poc.py; done

Actual result

The exception raised looks like the following. It shows that certain field names don't exist in self.fieldtype.

Exception in thread Thread-95:
Traceback (most recent call last):
  File "/usr/lib/python3.7/threading.py", line 917, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.7/threading.py", line 865, in run
    self._target(*self._args, **self._kwargs)
  File "/tmp/scapy-thread-race-poc.py", line 14, in f
    chksum=1, flags=0),
  File "/tmp/scapy/scapy/base_classes.py", line 258, in __call__
    i.__init__(*args, **kargs)
  File "/tmp/scapy/scapy/packet.py", line 156, in __init__
    self.fields[fname] = self.get_field(fname).any2i(self, value)
  File "/tmp/scapy/scapy/packet.py", line 256, in get_field
    return self.fieldtype[fld]
KeyError: 'flags'

Possible fixes/workarounds

This issue seems to be related to the field caching mechanism used in the Packet class.

Using a lock makes the issue disappear. The following is just a proof of concept. It needs more testing and review to ensure it won't cause other issues. Perhaps the locking can be made more granular.

diff --git a/scapy/packet.py b/scapy/packet.py
index 52932466..a6badc92 100644
--- a/scapy/packet.py
+++ b/scapy/packet.py
@@ -27,6 +27,7 @@ from scapy.utils import import_hexcap, tex_escape, colgen, issubtype, \
 from scapy.error import Scapy_Exception, log_runtime, warning
 from scapy.extlib import PYX
 import scapy.modules.six as six
+import threading
 
 try:
     import pyx
@@ -80,6 +81,7 @@ class Packet(six.with_metaclass(Packet_metaclass, BasePacket,
     class_default_fields = dict()
     class_default_fields_ref = dict()
     class_fieldtype = dict()
+    field_cache_lock = threading.RLock()
 
     @classmethod
     def from_hexcap(cls):
@@ -172,7 +174,8 @@ class Packet(six.with_metaclass(Packet_metaclass, BasePacket,
         if self.class_dont_cache.get(self.__class__, False):
             self.do_init_fields(self.fields_desc)
         else:
-            self.do_init_cached_fields()
+            with self.field_cache_lock:
+                self.do_init_cached_fields()
 
     def do_init_fields(self, flist):
         """

Workarounds I've been able to use to avoid this issue:

• Instantiating the subclasses once in the main thread, before other threads are started. That will populate the cache.

• Disabling the cache for each subclass used by threads. e.g.:

UDP.class_dont_cache[IP] = True

[guedou]

Could you try your fix with the following script #642 and report the before/after values for comparison?

[alirez]

Ran it my laptop with several other applications running. So not the perfect benchmarking environment.

• Before:

$ git checkout master 
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
$ for i in `seq 3`; do python /tmp/bench.py; done
Build - 17.38s
Dissect - 6.83s
Build & dissect - 25.94s
Build - 17.58s
Dissect - 6.96s
Build & dissect - 25.46s
Build - 17.14s
Dissect - 7.29s
Build & dissect - 25.38s

• After:

$ git checkout thread-safety 
Switched to branch 'thread-safety'
$ for i in `seq 3`; do python /tmp/bench.py; done
Build - 17.73s
Dissect - 7.22s
Build & dissect - 25.83s
Build - 17.65s
Dissect - 6.93s
Build & dissect - 25.64s
Build - 17.78s
Dissect - 6.98s
Build & dissect - 25.91s

[guedou]

Thanks for your quick answer! I wonder if your fix should be integrated or not =/ Maybe @gpotter2 and @p-l- have another opinion.

[gpotter2]

Wouldn’t it be possible to edit prepare_cached_fields in order to set the whole cache in one shot (with a temporary dictionary), to avoid the issue ?

Let's not start to add Locks within Scapy :/ that'll kill some perf at larger scales / faster rates. You could also be responsible for the Rlock when you builds your packets, rather than within Scapy :-)

If the one-shot dict doesn't work, it's a wontfix for me

[aoshiken]

Any news about this issue?
I'm going to start a project with Scapy and threads and I prefere to be prepared... :)

[guedou]

I cannot reproduce the issue on different Linux setups =/

@gpotter2 any hint to fix this without locks ?

[gpotter2]

That's easy to fix. The issue happens only on the first instanciation (first call of prepare_fields..), where it would set the packet in a "ready" state too early.

Will provide a patch

[guedou]

@alirez could you check that PR #2102 fixes your issue.

[guedou]

I finally managed to reproduce the issue by assigning 2000 to n.