From 7dfe7a2e25410170cf77026f05d856018eeec8a7 Mon Sep 17 00:00:00 2001
From: Andreas Rogge <andreas.rogge@bareos.com>
Date: Wed, 28 Jun 2023 16:36:33 +0200
Subject: [PATCH] stored: fix unintended decrement of block_num

When bsr() is called to initiare a re-read on a tape, it will decrement
ths block_num. As during the re-read the block_num was not incremented,
it shouldn't be decremented.
---
 core/src/stored/backends/unix_tape_device.cc | 56 +++++++++++---------
 core/src/stored/backends/unix_tape_device.h  |  2 +-
 core/src/stored/block.cc                     |  4 +-
 3 files changed, 36 insertions(+), 26 deletions(-)

diff --git a/core/src/stored/backends/unix_tape_device.cc b/core/src/stored/backends/unix_tape_device.cc
index 242416995ea..99c64bb6471 100644
--- a/core/src/stored/backends/unix_tape_device.cc
+++ b/core/src/stored/backends/unix_tape_device.cc
@@ -52,20 +52,16 @@
 #include "generic_tape_device.h"
 #include "unix_tape_device.h"
 
-namespace {
-static constexpr size_t size_increment = 1024 * 1024;
-static constexpr size_t max_size_increment = 16 * size_increment;
-static constexpr size_t next_size_increment(size_t count)
+namespace storagedaemon {
+
+static std::vector<std::size_t> bufsizes(std::size_t count)
 {
-  if (count < size_increment) {
-    return size_increment;
-  } else {
-    return (count / size_increment) * 2 * size_increment;
-  }
+  constexpr std::size_t mb = 1024 * 1024;
+  std::vector<std::size_t> sizes{1 * mb, 2 * mb, 4 * mb, 8 * mb, 16 * mb};
+  sizes.erase(sizes.begin(),
+              std::upper_bound(sizes.begin(), sizes.end(), count));
+  return sizes;
 }
-}  // namespace
-
-namespace storagedaemon {
 
 int unix_tape_device::d_ioctl(int fd, ioctl_req_t request, char* op)
 {
@@ -80,21 +76,33 @@ unix_tape_device::unix_tape_device()
 ssize_t unix_tape_device::d_read(int fd, void* buffer, size_t count)
 {
   ssize_t ret = ::read(fd, buffer, count);
-  /* when the driver fails to `read()` with `ENOMEM`, then the provided buffer
-   * was too small by re-reading with a temporary buffer that is enlarged
-   * step-by-step, we can read the block and returns its first `count` bytes.
+  /* If the driver fails to `read()` with `ENOMEM`, then the provided buffer
+   * was too small. By re-reading with a temporary buffer that is enlarged
+   * step-by-step, we can read the block and return the first `count` bytes.
    * This allows the calling code to read the block header and resize its buffer
    * according to the size recorded in that header.
    */
-  for (size_t bufsize = next_size_increment(count);
-       ret == -1 && errno == ENOMEM && bufsize <= max_size_increment;
-       bufsize = next_size_increment(bufsize)) {
-    std::vector<char> tmpbuf(bufsize);
-    bsr(1);       // go back one block so we can re-read
-    block_num++;  // re-increment the block counter that bsr() just incremented
-    if (auto tmpret = ::read(fd, tmpbuf.data(), tmpbuf.size()); tmpret != -1) {
-      memcpy(buffer, tmpbuf.data(), count);
-      ret = std::min(tmpret, static_cast<ssize_t>(count));
+  if (ret == -1 && errno == ENOMEM && HasCap(CAP_BSR)) {
+    for (auto bufsize : bufsizes(count)) {
+      // first go back one block so we can re-read
+      if (!bsr(1)) {
+        /* when backward-spacing fails for some reason, there's not much we
+         * can do, so we just return the original ENOMEM and hope that the
+         * caller knows the device is in a non well-defined state.
+         */
+        errno = ENOMEM;
+        return -1;
+      }
+      block_num++;  // re-increment the block counter bsr() just decremented
+      std::vector<char> tmpbuf(bufsize);
+      if (auto tmpret = ::read(fd, tmpbuf.data(), tmpbuf.size());
+          tmpret != -1) {
+        memcpy(buffer, tmpbuf.data(), count);
+        ret = std::min(tmpret, static_cast<ssize_t>(count));
+        break;  // successful read
+      } else if (errno != ENOMEM) {
+        break;  // some other error occured
+      }
     }
   }
   return ret;
diff --git a/core/src/stored/backends/unix_tape_device.h b/core/src/stored/backends/unix_tape_device.h
index 4e6fb2ac49c..aa728695889 100644
--- a/core/src/stored/backends/unix_tape_device.h
+++ b/core/src/stored/backends/unix_tape_device.h
@@ -36,7 +36,7 @@ class unix_tape_device : public generic_tape_device {
   ~unix_tape_device() { close(nullptr); };
 
   int d_ioctl(int fd, ioctl_req_t request, char* op) override;
-  virtual ssize_t d_read(int fd, void* buffer, size_t count) override;
+  ssize_t d_read(int fd, void* buffer, size_t count) override;
 };
 
 } /* namespace storagedaemon */
diff --git a/core/src/stored/block.cc b/core/src/stored/block.cc
index 1eb1c24edd9..b784ce27d09 100644
--- a/core/src/stored/block.cc
+++ b/core/src/stored/block.cc
@@ -1076,7 +1076,9 @@ DeviceControlRecord::ReadStatus DeviceControlRecord::ReadBlockFromDev(bool)
     // Attempt to Reposition to re-read the block
     if (dev->IsTape()) {
       Dmsg0(250, "BootStrapRecord for reread; block too big for buffer.\n");
-      if (!dev->bsr(1)) {
+      if (dev->bsr(1)) {
+        dev->block_num++;  // re-increment what bsr() decremented
+      } else {
         Mmsg(dev->errmsg, "%s", dev->bstrerror());
         Jmsg(jcr, M_ERROR, 0, "%s", dev->errmsg);
         block->read_len = 0;