Changeset ebed638 for lib

Timestamp:

09/04/18 14:56:53 (2 years ago)

Author:

Shane Alcock <salcock@…>

Branches:

develop, master, ringdecrementfix, ringperformance

Children:

2201d8c

Parents:

37ee856

Message:

Don't munmap ring rx buffers until last possible moment.

As soon as we munmap the buffers, any packets contained within
those buffers become invalid -- however, the user application may
still have references to those packets and could try to operate
on them without knowing that the memory holding the packet payload
is invalid.

Previously, the buffers were munmapped when the trace was paused,
which was causing crashes and invalid memory accesses.

Instead, we now only munmap the buffer if either the trace is
restarted or destroyed, so the packets will remain valid for
longer. Also, the new "start" iteration counting added in an
earlier commit means we can now recognise when a packet belongs
to an unmapped buffer and therefore return appropriate errors if
a user tries to interact with the packet.

This also fixes the assertion failure when freeing a ring packet
after the ring trace has been paused, i.e. after a call to
trace_pstop().

Location:

lib

Files:

• format_linux_common.c (modified) (1 diff)
• format_linux_ring.c (modified) (4 diffs)

lib/format_linux_common.c

@@ -232 +232 @@
                 close(stream->fd);
         stream->fd = -1;
-        if (stream->rx_ring != MAP_FAILED)
-                munmap(stream->rx_ring,
-                       stream->req.tp_block_size *
-                       stream->req.tp_block_nr);
-        stream->rx_ring = MAP_FAILED;
-        stream->rxring_offset = 0;
         FORMAT_DATA->dev_stats.if_name[0] = 0;
+
+        /* Don't munmap the rx_ring here -- keep it around as long as
+         * possible to ensure that any packets that the user is still
+         * holding references to remain valid.
+         *
+         * Don't worry, linuxring will munmap the rx_ring as soon as
+         * someone either destroys or restarts the trace. At that point,
+         * any remaining packets from the old ring will be recognisable
+         * as invalid.
+         */
 }
 

lib/format_linux_ring.c

@@ -252 +252 @@
         char error[2048];
 
+        /* Unmap any previous ring buffers associated with this stream. */
+        if (stream->rx_ring != MAP_FAILED) {
+                munmap(stream->rx_ring, stream->req.tp_block_size *
+                                stream->req.tp_block_nr);
+                stream->rx_ring = MAP_FAILED;
+                stream->rxring_offset = 0;
+        }
+
+
         /* We set the socket up the same and then convert it to PACKET_MMAP */
         if (linuxcommon_start_input_stream(libtrace, stream) < 0)
@@ -274 +283 @@
         return 0;
 }
+
+static int linuxring_fin_input(libtrace_t *libtrace) {
+        size_t i;
+
+        if (libtrace->format_data) {
+                for (i = 0; i < libtrace_list_get_size(FORMAT_DATA->per_stream); ++i) {
+                        struct linux_per_stream_t *stream;
+                        stream = libtrace_list_get_index(
+                                FORMAT_DATA->per_stream, i)->data;
+                        if (stream->rx_ring != MAP_FAILED) {
+                                munmap(stream->rx_ring,
+                                                stream->req.tp_block_size *
+                                                stream->req.tp_block_nr);
+                        }
+                }
+
+                if (FORMAT_DATA->filter != NULL)
+                        free(FORMAT_DATA->filter);
+
+                if (FORMAT_DATA->per_stream)
+                        libtrace_list_deinit(FORMAT_DATA->per_stream);
+
+                free(libtrace->format_data);
+        }
+
+        return 0;
+}
+
 
 static int linuxring_start_input(libtrace_t *libtrace)
@@ -622 +659 @@
         /* If we own the packet (i.e. it's not a copy), we need to free it */
         if (packet->buf_control == TRACE_CTRL_EXTERNAL) {
-                /* Started should always match the existence of the rx_ring
-                 * in the parallel case still just check the first ring */
-                assert(!!FORMAT_DATA_FIRST->rx_ring ==
-                       !!packet->trace->started);
                 /* If we don't have a ring its already been destroyed */
                 if (FORMAT_DATA_FIRST->rx_ring != MAP_FAILED)
@@ -742 +775 @@
         NULL,                           /* config_output */
         linuxring_start_output,         /* start_ouput */
-        linuxcommon_fin_input,          /* fin_input */
+        linuxring_fin_input,            /* fin_input */
         linuxring_fin_output,           /* fin_output */
         linuxring_read_packet,          /* read_packet */