dencoding: check struct_v against DECODE_START(v, ...) at compile-time

Typical problem found in recent reviews of dencoding changes is missed
increment of the version number passed to `DECODE_START()` macro in
a decoder.
It's easy to overlook and hard to find as the number is used for rarely
happening explicit breaks of schema compatibility.

This commit brings compile-time checks for `struct_v` in decoders
(and for `denc` also in encoders) to find a situation when it's
compared against version higher than declared using the `DECODE_START(v, ...)`
macro.

Additionally, the patch fixes the currently existing issues of this
genre buried across the enitre code base.

Signed-off-by: Radoslaw Zarzynski <[email protected]>

Author: rzarzynski

src/common/versioned_variant.h

@@ -203,7 +203,7 @@ void decode(std::variant<Ts...>& v, bufferlist::const_iterator& p)
 
   // use struct_v as an index into the variant after converting it into a
   // compile-time index I
-  const uint8_t index = struct_v - converted_max_version;
+  const uint8_t index = struct_v.v - converted_max_version;
   boost::mp11::mp_with_index<sizeof...(Ts)>(index, [&v, &p] (auto I) {
       // default-construct the type at index I and call its decoder
       decode(v.template emplace<I>(), p);

src/include/denc.h

@@ -1790,6 +1790,42 @@ inline std::enable_if_t<traits::supported && !traits::featured> decode_nohead(
     "' v=" + std::to_string(code_v)+ " cannot decode v=" + std::to_string(struct_v) +
     " minimal_decoder=" + std::to_string(struct_compat));
 }
+
+// compile-time  checker for struct_v to detect mismatch of declared
+// decoder version with actually implemented blocks like "struct_v < 100".
+// it addresses the common problem of forgetting to bump the version up
+// in decoder's `DECODE_START` (or `DENC_START`) when adding a new
+// schema revision.
+template <__u8 MaxV>
+struct StructVChecker
+{
+  struct CheckingWrapper {
+    consteval CheckingWrapper(__u8 c) : c(c) {
+      consteval_assert(
+        c <= MaxV,
+	"checking against higher version than declared in DECODE_START");
+    }
+    __u8 c;
+  };
+  // we want the implicit conversion to take place but with lower
+  // rank than the CheckingWrapper-conversion during struct_v cmps.
+  template<class T=void> operator __u8() {
+    return v;
+  }
+  // need the wrapper as the operator cannot be consteval; otherwise
+  // it couldn't have accessed the non-constexpr `v`.
+  auto operator<=>(CheckingWrapper c) {
+    return v <=> c.c;
+  }
+  auto operator==(CheckingWrapper c) {
+    return v == c.c;
+  }
+  auto operator!=(CheckingWrapper c) {
+    return v != c.c;
+  }
+  __u8 v;
+};
+
 #define DENC_HELPERS							\
   /* bound_encode */							\
   static void _denc_start(size_t& p,					\
@@ -1857,39 +1893,39 @@ inline std::enable_if_t<traits::supported && !traits::featured> decode_nohead(
 // Due to -2 compatibility rule we cannot bump up compat until U____ release.
 // SQUID=19 T____=20 U____=21
 
-#define DENC_START(v, compat, p)					\
-  __u8 struct_v = v;							\
+#define DENC_START(_v, compat, p)					\
+  StructVChecker<_v> struct_v{_v};					\
   __u8 struct_compat = compat;						\
   char *_denc_pchar;							\
   uint32_t _denc_u32;							\
   static_assert(CEPH_RELEASE >= (CEPH_RELEASE_SQUID /*19*/ + 2) || compat == 1);	\
-  _denc_start(p, &struct_v, &struct_compat, &_denc_pchar, &_denc_u32);	\
+  _denc_start(p, &struct_v.v, &struct_compat, &_denc_pchar, &_denc_u32);	\
   do {
 
 // For the only type that is with compat 2: unittest.
-#define DENC_START_COMPAT_2(v, compat, p)				\
-  __u8 struct_v = v;							\
+#define DENC_START_COMPAT_2(_v, compat, p)				\
+  StructVChecker<_v> struct_v{_v};					\
   __u8 struct_compat = compat;						\
   char *_denc_pchar;							\
   uint32_t _denc_u32;							\
   static_assert(CEPH_RELEASE >= (CEPH_RELEASE_SQUID /*19*/ + 2) || compat == 2);	\
-  _denc_start(p, &struct_v, &struct_compat, &_denc_pchar, &_denc_u32);	\
+  _denc_start(p, &struct_v.v, &struct_compat, &_denc_pchar, &_denc_u32);	\
   do {
 
 // For osd_reqid_t which cannot be upgraded at all.
 // We used it to communicate with clients and now we cannot safely upgrade.
-#define DENC_START_OSD_REQID(v, compat, p)				\
-  __u8 struct_v = v;							\
+#define DENC_START_OSD_REQID(_v, compat, p)				\
+  StructVChecker<_v> struct_v{_v};					\
   __u8 struct_compat = compat;						\
   char *_denc_pchar;							\
   uint32_t _denc_u32;							\
   static_assert(compat == 2, "osd_reqid_t cannot be upgraded");		\
-  _denc_start(p, &struct_v, &struct_compat, &_denc_pchar, &_denc_u32);	\
+  _denc_start(p, &struct_v.v, &struct_compat, &_denc_pchar, &_denc_u32);	\
   do {
 
 #define DENC_FINISH(p)							\
   } while (false);							\
-  _denc_finish(p, &struct_v, &struct_compat, &_denc_pchar, &_denc_u32);
+  _denc_finish(p, &struct_v.v, &struct_compat, &_denc_pchar, &_denc_u32);
 
 
 // ----------------------------------------------------------------------

src/include/encoding.h

@@ -1493,7 +1493,22 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
  * @param v current version of the encoding that the code supports/encodes
  * @param bl bufferlist::iterator for the encoded data
  */
-#define DECODE_START(v, bl)						\
+#define DECODE_START(_v, bl)						\
+  StructVChecker<_v> struct_v;						\
+  __u8 struct_compat;							\
+  using ::ceph::decode;							\
+  decode(struct_v.v, bl);						\
+  decode(struct_compat, bl);						\
+  if (_v < struct_compat)						\
+    throw ::ceph::buffer::malformed_input(DECODE_ERR_NO_COMPAT(__PRETTY_FUNCTION__, _v, struct_v.v, struct_compat)); \
+  __u32 struct_len;							\
+  decode(struct_len, bl);						\
+  if (struct_len > bl.get_remaining())					\
+    throw ::ceph::buffer::malformed_input(DECODE_ERR_PAST(__PRETTY_FUNCTION__)); \
+  unsigned struct_end = bl.get_off() + struct_len;			\
+  do {
+
+#define DECODE_START_UNCHECKED(v, bl)					\
   __u8 struct_v, struct_compat;						\
   using ::ceph::decode;							\
   decode(struct_v, bl);						\
@@ -1527,22 +1542,22 @@ decode(std::array<T, N>& v, bufferlist::const_iterator& p)
 
 /* BEWARE: any change to this macro MUST be also reflected in the duplicative
  * DECODE_START_LEGACY_COMPAT_LEN! */
-#define __DECODE_START_LEGACY_COMPAT_LEN(v, compatv, lenv, skip_v, bl)	\
+#define __DECODE_START_LEGACY_COMPAT_LEN(_v, compatv, lenv, skip_v, bl)	\
   using ::ceph::decode;							\
-  __u8 struct_v;							\
-  decode(struct_v, bl);						\
-  if (struct_v >= compatv) {						\
+  StructVChecker<_v> struct_v;						\
+  decode(struct_v.v, bl);						\
+  if (struct_v.v >= compatv) {						\
     __u8 struct_compat;							\
     decode(struct_compat, bl);					\
-    if (v < struct_compat)						\
-      throw ::ceph::buffer::malformed_input(DECODE_ERR_NO_COMPAT(__PRETTY_FUNCTION__, v, struct_v, struct_compat)); \
+    if (_v < struct_compat)						\
+      throw ::ceph::buffer::malformed_input(DECODE_ERR_NO_COMPAT(__PRETTY_FUNCTION__, _v, struct_v.v, struct_compat)); \
   } else if (skip_v) {							\
     if (bl.get_remaining() < skip_v)					\
       throw ::ceph::buffer::malformed_input(DECODE_ERR_PAST(__PRETTY_FUNCTION__)); \
     bl +=  skip_v;							\
   }									\
   unsigned struct_end = 0;						\
-  if (struct_v >= lenv) {						\
+  if (struct_v.v >= lenv) {						\
     __u32 struct_len;							\
     decode(struct_len, bl);						\
     if (struct_len > bl.get_remaining())				\

src/librbd/journal/Types.cc

@@ -422,7 +422,7 @@ void EventEntry::encode(bufferlist& bl) const {
 }
 
 void EventEntry::decode(bufferlist::const_iterator& it) {
-  DECODE_START(1, it);
+  DECODE_START(5, it);
 
   uint32_t event_type;
   decode(event_type, it);

src/mds/CInode.cc

@@ -1771,7 +1771,7 @@ void CInode::decode_lock_ilink(bufferlist::const_iterator& p)
 {
   ceph_assert(!is_auth());
   auto _inode = allocate_inode(*get_inode());
-  DECODE_START(1, p);
+  DECODE_START(2, p);
   decode(_inode->version, p);
   utime_t tm;
   decode(tm, p);

src/mds/FSMap.cc

@@ -664,7 +664,7 @@ void FSMap::decode(bufferlist::const_iterator& p)
   struct_version = 0;
   DECODE_START(STRUCT_VERSION, p);
   DECODE_OLDEST(7);
-  struct_version = struct_v;
+  struct_version = struct_v.v;
   decode(epoch, p);
   decode(next_filesystem_id, p);
   decode(legacy_client_fscid, p);

src/mds/MDCache.cc

@@ -11187,7 +11187,7 @@ void MDCache::decode_replica_dir(CDir *&dir, bufferlist::const_iterator& p, CIno
 
 void MDCache::decode_replica_dentry(CDentry *&dn, bufferlist::const_iterator& p, CDir *dir, MDSContext::vec& finished)
 {
-  DECODE_START(1, p);
+  DECODE_START(3, p);
   string name;
   snapid_t last;
   decode(name, p);
@@ -11463,7 +11463,7 @@ void MDCache::encode_remote_dentry_link(CDentry::linkage_t *dnl, bufferlist& bl)
 
 void MDCache::decode_remote_dentry_link(CDir *dir, CDentry *dn, bufferlist::const_iterator& p)
 {
-  DECODE_START(1, p);
+  DECODE_START(2, p);
   inodeno_t ino;
   __u8 d_type;
   decode(ino, p);

src/messages/MClientReply.h

@@ -166,7 +166,7 @@ struct InodeStat {
   void decode(ceph::buffer::list::const_iterator &p, const uint64_t features) {
     using ceph::decode;
     if (features == (uint64_t)-1) {
-      DECODE_START(7, p);
+      DECODE_START(8, p);
       decode(vino.ino, p);
       decode(vino.snapid, p);
       decode(rdev, p);

src/mon/MgrMap.h

@@ -146,7 +146,7 @@ class MgrMap
     }
 
     void decode(ceph::buffer::list::const_iterator &bl) {
-      DECODE_START(1, bl);
+      DECODE_START(2, bl);
       decode(name, bl);
       decode(can_run, bl);
       decode(error_string, bl);

src/mon/mon_types.h

@@ -231,7 +231,7 @@ struct DataStats {
     ENCODE_FINISH(bl);
   }
   void decode(ceph::buffer::list::const_iterator &p) {
-    DECODE_START(1, p);
+    DECODE_START(3, p);
     // we moved from having fields in kb to fields in byte
     if (struct_v > 2) {
       decode(fs_stats.byte_total, p);