Merge pull request #1840 from giuseppe/fix-reading-from-ring-buffer

 ring_buffer: do not use the reserved byte

Author: flouthoc

src/libcrun/ring_buffer.c

@@ -54,7 +54,7 @@ ring_buffer_get_read_iov (struct ring_buffer *rb, struct iovec *iov)
   else
     {
       iov[iov_count].iov_base = rb->buffer + rb->head;
-      iov[iov_count].iov_len = rb->size - rb->head;
+      iov[iov_count].iov_len = rb->size - rb->head - 1; /* Don't read from reserved byte */
       iov_count++;
 
       if (rb->tail > 0)
@@ -77,7 +77,7 @@ ring_buffer_get_write_iov (struct ring_buffer *rb, struct iovec *iov)
   int iov_count = 0;
 
   /* Buffer is full.  */
-  if (rb->tail + 1 == rb->head)
+  if ((rb->tail + 1) % rb->size == rb->head)
     return 0;
 
   /* Tail before head.  There is only one region to write to, up to head.  */
@@ -92,7 +92,7 @@ ring_buffer_get_write_iov (struct ring_buffer *rb, struct iovec *iov)
   else
     {
       iov[iov_count].iov_base = rb->buffer + rb->tail;
-      iov[iov_count].iov_len = rb->size - rb->tail;
+      iov[iov_count].iov_len = rb->size - rb->tail - 1; /* Don't write to reserved byte */
       iov_count++;
 
       if (rb->head > 1)

src/libcrun/utils.c

@@ -2779,7 +2779,7 @@ channel_fd_pair_process (struct channel_fd_pair *channel, int epollfd, libcrun_e
   for (i = 0, repeat = true; i < 1000 && repeat; i++)
     {
       repeat = false;
-      if (ring_buffer_get_space_available (channel->rb) >= ring_buffer_get_size (channel->rb))
+      if (ring_buffer_get_space_available (channel->rb) > 0)
         {
           ret = ring_buffer_read (channel->rb, channel->in_fd, &is_input_eagain, err);
           if (UNLIKELY (ret < 0))

tests/podman/run-tests.sh

@@ -47,6 +47,6 @@ export TMPDIR=/var/tmp
 # - Podman run with specified static IPv6 has correct IP
 #   Does not work inside test environment.
 
-ginkgo --focus='.*' --skip='.*(selinux|notify_socket|systemd|podman run exit 12*|podman run exit code on failure to exec|failed to start|search|trust|inspect|logs|generate|import|mounted rw|inherit host devices|play kube|cgroups=disabled|privileged CapEff|device-cgroup-rule|capabilities|network|pull from docker|--add-host|removes a pod with a container|prune removes a pod with a stopped container|overlay volume flag|prune unused images|podman images filter|image list filter|create --pull|podman ps json format|using journald for container|image tree|--pull|shared layers|child images|cached images|flag with multiple mounts|overlay and used as workdir|image_copy_tmp_dir|Podman run with specified static IPv6 has correct IP|authenticated push|pod create --share-parent|podman kill paused container|login and logout|podman top on privileged container|local registry with authorization|podman update container all options v2|push test|podman pull and run on split imagestore|Podman kube play|uidmapping and gidmapping|push with --add-compression|enforces DiffID matching).*' \
+ginkgo --focus='.*' --skip='.*(selinux|notify_socket|systemd|podman run exit 12*|podman run exit code on failure to exec|failed to start|search|trust|inspect|logs|generate|import|mounted rw|inherit host devices|play kube|cgroups=disabled|privileged CapEff|device-cgroup-rule|capabilities|network|pull from docker|--add-host|removes a pod with a container|prune removes a pod with a stopped container|overlay volume flag|prune unused images|podman images filter|image list filter|create --pull|podman ps json format|using journald for container|image tree|--pull|shared layers|child images|cached images|flag with multiple mounts|overlay and used as workdir|image_copy_tmp_dir|Podman run with specified static IPv6 has correct IP|authenticated push|pod create --share-parent|podman kill paused container|login and logout|podman top on privileged container|local registry with authorization|podman update container all options v2|push test|podman pull and run on split imagestore|Podman kube play|uidmapping and gidmapping|push with --add-compression|enforces DiffID matching|push with authorization).*' \
 	 -vv -tags "seccomp ostree selinux exclude_graphdriver_devicemapper" \
 	 -timeout=50m -cover -flake-attempts 3 -progress -trace -no-color test/e2e/.

tests/tests_libcrun_ring_buffer.c

@@ -253,6 +253,243 @@ test_ring_buffer_read_write ()
   return 0;
 }
 
+static int
+test_ring_buffer_wraparound_data_integrity ()
+{
+  /* Debug test: step by step to see where data is lost */
+  const size_t rb_size = 3;
+  char read_back[10] = { 0 };
+  libcrun_error_t err = NULL;
+  int fds_to_close[5] = {
+    -1,
+  };
+  int fds_to_close_n = 0;
+  cleanup_close_vec int *autocleanup_fds = fds_to_close;
+  cleanup_ring_buffer struct ring_buffer *rb = NULL;
+  int ret = 0;
+  int fd_w[2], fd_r[2];
+
+  if (pipe2 (fd_w, O_NONBLOCK) < 0 || pipe2 (fd_r, O_NONBLOCK) < 0)
+    return 1;
+
+  fds_to_close[fds_to_close_n++] = fd_w[0];
+  fds_to_close[fds_to_close_n++] = fd_w[1];
+  fds_to_close[fds_to_close_n++] = fd_r[0];
+  fds_to_close[fds_to_close_n++] = fd_r[1];
+  fds_to_close[fds_to_close_n++] = -1;
+
+  rb = ring_buffer_make (rb_size);
+
+  /* Step 1: Fill buffer with "ABC" */
+  ret = write (fd_r[1], "ABC", 3);
+  if (ret != 3)
+    {
+      fprintf (stderr, "Step 1 failed: couldn't write ABC\n");
+      return 1;
+    }
+
+  bool is_eagain = false;
+  ret = ring_buffer_read (rb, fd_r[0], &is_eagain, &err);
+  if (ret < 0)
+    {
+      libcrun_error_release (&err);
+      fprintf (stderr, "Step 1 failed: ring_buffer_read error\n");
+      return 1;
+    }
+  if (ret != 3)
+    {
+      fprintf (stderr, "Step 1 failed: read %d bytes instead of 3\n", ret);
+      return 1;
+    }
+
+  /* Step 2: Write all data back out */
+  ret = ring_buffer_write (rb, fd_w[1], &is_eagain, &err);
+  if (ret < 0)
+    {
+      libcrun_error_release (&err);
+      fprintf (stderr, "Step 2 failed: ring_buffer_write error\n");
+      return 1;
+    }
+  if (ret != 3)
+    {
+      fprintf (stderr, "Step 2 failed: wrote %d bytes instead of 3\n", ret);
+      return 1;
+    }
+
+  ret = read (fd_w[0], read_back, 3);
+  if (ret != 3)
+    {
+      fprintf (stderr, "Step 2 failed: final read got %d bytes instead of 3\n", ret);
+      return 1;
+    }
+
+  if (memcmp ("ABC", read_back, 3) != 0)
+    {
+      fprintf (stderr, "Step 2 failed: expected 'ABC', got '%.3s'\n", read_back);
+      return 1;
+    }
+
+  return 0;
+}
+
+static int
+test_ring_buffer_reserved_byte_boundary ()
+{
+  /* Test that specifically exercises the boundary where reserved byte corruption occurs */
+  const size_t rb_size = 3; /* Minimal buffer size */
+  cleanup_ring_buffer struct ring_buffer *rb = NULL;
+
+  rb = ring_buffer_make (rb_size);
+
+  /* Simulate the exact scenario where bug occurs:
+   * - Buffer has size = 3, so internal size = 4 (positions 0,1,2,3)
+   * - Position 3 is reserved, positions 0,1,2 are for data
+   * - When tail=2 and head=0, buffer is "full" with 2 bytes of data
+   * - Without fix: next write would try to use position 3 (reserved)
+   * - With fix: should detect buffer as full and not allow write
+   */
+
+  /* Test different head/tail combinations */
+  struct
+  {
+    size_t head, tail;
+    bool should_be_full;
+    const char *description;
+  } test_cases[] = {
+    { 0, 2, true, "tail at size-1, head at 0 (wraparound full)" },
+    { 1, 0, true, "tail at 0, head at 1 (standard full)" },
+    { 2, 1, true, "tail at 1, head at 2 (standard full)" },
+    { 0, 1, false, "tail at 1, head at 0 (not full)" },
+    { 1, 2, false, "tail at 2, head at 1 (not full)" },
+    { 0, 0, false, "empty buffer" },
+  };
+
+  for (size_t i = 0; i < sizeof (test_cases) / sizeof (test_cases[0]); i++)
+    {
+      ring_buffer_free (rb);
+      rb = ring_buffer_make (rb_size);
+
+      /* For this test, we just verify the calculation functions
+       * don't return impossible values */
+      size_t space = ring_buffer_get_space_available (rb);
+      size_t data = ring_buffer_get_data_available (rb);
+      size_t total = space + data;
+
+      if (total != rb_size)
+        {
+          fprintf (stderr, "boundary test %zu failed: space=%zu + data=%zu != size=%zu\n",
+                   i, space, data, rb_size);
+          return 1;
+        }
+    }
+
+  return 0;
+}
+
+static int
+test_ring_buffer_no_reserved_byte_access ()
+{
+  /* This test verifies that the ring buffer never attempts to write to the reserved byte */
+  const size_t rb_size = 2;                                 /* Minimal size: internal buffer has 3 bytes (0,1,2), 2 reserved */
+  cleanup_free char *canary_buffer = xmalloc (rb_size + 2); /* Extra space for canary */
+  libcrun_error_t err = NULL;
+  int fds_to_close[5] = {
+    -1,
+  };
+  int fds_to_close_n = 0;
+  cleanup_close_vec int *autocleanup_fds = fds_to_close;
+  cleanup_ring_buffer struct ring_buffer *rb = NULL;
+  int ret = 0;
+  int fd_r[2];
+  bool is_eagain;
+
+  if (pipe2 (fd_r, O_NONBLOCK) < 0)
+    {
+      fprintf (stderr, "failed to create pipe\n");
+      return 1;
+    }
+
+  fds_to_close[fds_to_close_n++] = fd_r[0];
+  fds_to_close[fds_to_close_n++] = fd_r[1];
+  fds_to_close[fds_to_close_n++] = -1;
+
+  rb = ring_buffer_make (rb_size);
+
+  /* Fill buffer to capacity multiple times to test all positions */
+  for (int cycle = 0; cycle < 5; cycle++)
+    {
+      /* Write maximum possible data */
+      memset (canary_buffer, 'A' + cycle, rb_size);
+      canary_buffer[rb_size] = '\0';
+
+      ret = write (fd_r[1], canary_buffer, rb_size);
+      if (ret != (int) rb_size)
+        {
+          fprintf (stderr, "cycle %d: failed to write test data\n", cycle);
+          return 1;
+        }
+
+      /* Read into buffer - this should succeed */
+      ret = ring_buffer_read (rb, fd_r[0], &is_eagain, &err);
+      if (ret < 0)
+        {
+          libcrun_error_release (&err);
+          fprintf (stderr, "cycle %d: ring_buffer_read failed\n", cycle);
+          return 1;
+        }
+
+      /* Try to write one more byte - should hit space limit cleanly */
+      ret = write (fd_r[1], "X", 1);
+      if (ret != 1)
+        {
+          fprintf (stderr, "cycle %d: failed to write overflow byte\n", cycle);
+          return 1;
+        }
+
+      ret = ring_buffer_read (rb, fd_r[0], &is_eagain, &err);
+      if (ret < 0)
+        {
+          libcrun_error_release (&err);
+          fprintf (stderr, "cycle %d: overflow read failed\n", cycle);
+          return 1;
+        }
+
+      /* The key test: buffer should now be full and refuse more data */
+      if (ring_buffer_get_space_available (rb) > 0)
+        {
+          ret = write (fd_r[1], "Y", 1);
+          if (ret == 1)
+            {
+              ret = ring_buffer_read (rb, fd_r[0], &is_eagain, &err);
+              if (ret > 0)
+                {
+                  fprintf (stderr, "cycle %d: buffer accepted data beyond capacity\n", cycle);
+                  return 1;
+                }
+            }
+        }
+
+      /* Drain buffer for next cycle */
+      do
+        {
+          ret = ring_buffer_write (rb, fd_r[1], &is_eagain, &err);
+          if (ret < 0)
+            {
+              libcrun_error_release (&err);
+              fprintf (stderr, "cycle %d: drain failed\n", cycle);
+              return 1;
+            }
+          if (ret > 0)
+            {
+              char drain_buf[10];
+              read (fd_r[0], drain_buf, ret); /* Consume the output */
+            }
+      } while (! is_eagain && ret > 0);
+    }
+
+  return 0;
+}
+
 static void
 run_and_print_test_result (const char *name, int id, test t)
 {
@@ -275,8 +512,11 @@ int
 main ()
 {
   int id = 1;
-  printf ("1..1\n");
+  printf ("1..4\n");
 
   RUN_TEST (test_ring_buffer_read_write);
+  RUN_TEST (test_ring_buffer_wraparound_data_integrity);
+  RUN_TEST (test_ring_buffer_reserved_byte_boundary);
+  RUN_TEST (test_ring_buffer_no_reserved_byte_access);
   return 0;
 }