Repository: oremanj/python-netfilterqueue (language: python)

Issue:
__str__ method is not safe
This code results with segmentation fault

```
from typing import List

import netfilterqueue
from netfilterqueue import NetfilterQueue


packet_storage: List[netfilterqueue.Packet] = []


def print_packets():
    for packet in packet_storage:
        print(packet)


def handle_packet(packet: netfilterqueue.Packet):
    packet.retain()
    packet.accept()
    packet_storage.append(packet)
    print_packets()


q = NetfilterQueue()
q.bind(11, handle_packet)
q.run()
```
Looks like __str__ method is not safe, because there is no check for NULL  of `self.payload`:
```
    def __str__(self):
        cdef iphdr *hdr = <iphdr*>self.payload
        protocol = PROTOCOLS.get(hdr.protocol, "Unknown protocol")
        return "%s packet, %s bytes" % (protocol, self.payload_len)
 ```

Is this ok, can that be fixed? 
 


Discussion / hints:
self.payload is a defined as a pointer and passed to a C lib function (libnetfilterqueue). the payload data is allocated in memory by the C lib and initialized to point to the data. Because of this, once you set a verdict on the packet the payload and all other pointers/references held to memory allocated in the C lib will no longer be valid.

Fix patch:
diff --git a/netfilterqueue/_impl.pyx b/netfilterqueue/_impl.pyx
index d656d5b..a8e1960 100644
--- a/netfilterqueue/_impl.pyx
+++ b/netfilterqueue/_impl.pyx
@@ -57,7 +57,15 @@ cdef class Packet:
         self._given_payload = None
 
     def __str__(self):
-        cdef iphdr *hdr = <iphdr*>self.payload
+        cdef unsigned char *payload = NULL
+        if self._owned_payload:
+            payload = self._owned_payload
+        elif self.payload != NULL:
+            payload = self.payload
+        else:
+            return "%d byte packet, contents unretained" % (self.payload_len,)
+
+        cdef iphdr *hdr = <iphdr*>payload
         protocol = PROTOCOLS.get(hdr.protocol, "Unknown protocol")
         return "%s packet, %s bytes" % (protocol, self.payload_len)
 
diff --git a/setup.py b/setup.py
index 053b802..ae2997f 100644
--- a/setup.py
+++ b/setup.py
@@ -3,7 +3,7 @@
 
 exec(open("netfilterqueue/_version.py", encoding="utf-8").read())
 
-setup_requires = []
+setup_requires = ["wheel"]
 try:
     # Use Cython
     from Cython.Build import cythonize
@@ -21,7 +21,7 @@
     if "egg_info" in sys.argv:
         # We're being run by pip to figure out what we need. Request cython in
         # setup_requires below.
-        setup_requires = ["cython"]
+        setup_requires += ["cython"]
     elif not os.path.exists(
         os.path.join(os.path.dirname(__file__), "netfilterqueue/_impl.c")
     ):


Test patch:
diff --git a/tests/test_basic.py b/tests/test_basic.py
index 8ec7492..99a331b 100644
--- a/tests/test_basic.py
+++ b/tests/test_basic.py
@@ -23,6 +23,7 @@ async def test_comms_without_queue(harness):
 async def test_queue_dropping(harness):
     async def drop(packets, msg):
         async for packet in packets:
+            assert "UDP packet" in str(packet)
             if packet.get_payload()[28:] == msg:
                 packet.drop()
             else:
@@ -190,6 +191,7 @@ async def test_errors(harness):
 async def test_unretained(harness):
     def cb(chan, pkt):
         # Can access payload within callback
+        assert "UDP packet" in str(pkt)
         assert pkt.get_payload()[-3:] in (b"one", b"two")
         chan.send_nowait(pkt)
 
@@ -202,6 +204,7 @@ def cb(chan, pkt):
                 RuntimeError, match="Payload data is no longer available"
             ):
                 p.get_payload()
+            assert "contents unretained" in str(p)
             # Can still issue verdicts though
             if accept:
                 p.accept()