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 <rzarzyns@redhat.com>

diff --git a/src/common/versioned_variant.h b/src/common/versioned_variant.h
index 124c588391691289ca7189024d49f19b86494f6b..5405222473d74703c7e345171c4f22e1d91e05a6 100644 (file)
--- a/src/common/versioned_variant.h
+++ b/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);

diff --git a/src/include/denc.h b/src/include/denc.h
index 65b41b4479816d64afbdb3b507bffb33b206bbd0..d945e2646896ec5d92336f7fb45be7c91c371db7 100644 (file)
--- a/src/include/denc.h
+++ b/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);
 
 
 // ----------------------------------------------------------------------

diff --git a/src/include/encoding.h b/src/include/encoding.h
index f53a04837eceb4f966dc529ecf007300f8bd517c..fc8825c6492c9b0e6cd778e2f87abd15d9376ad9 100644 (file)
--- a/src/include/encoding.h
+++ b/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())                               \

diff --git a/src/librbd/journal/Types.cc b/src/librbd/journal/Types.cc
index d76a15e557cecb7f0723878197a84b10401d4a52..da0bc228819471e0647af038514811b31450033d 100644 (file)
--- a/src/librbd/journal/Types.cc
+++ b/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);

diff --git a/src/mds/CInode.cc b/src/mds/CInode.cc
index 7bddb58b48fe13a823e427eced8802126cf9518f..361551c6e370bb541f7227015d3f4cfc6136f119 100644 (file)
--- a/src/mds/CInode.cc
+++ b/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);

diff --git a/src/mds/FSMap.cc b/src/mds/FSMap.cc
index 75cd3fbadefae5fdd96169cfb5e9e8a99b93f709..ea8b78bb0f6908610a4d5536ad64d60987ade742 100644 (file)
--- a/src/mds/FSMap.cc
+++ b/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);

diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
index c432fe4beb3b195c0738e5e5449a7084965cf66a..4dc929bd009236556a2a9696e31d80c41340405a 100644 (file)
--- a/src/mds/MDCache.cc
+++ b/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);

diff --git a/src/messages/MClientReply.h b/src/messages/MClientReply.h
index be34e8d4f7cc2027e3f87e5590062a74abaf770b..0b8a7731c00cd5c3ed399d4067dd9cdaf14e0714 100644 (file)
--- a/src/messages/MClientReply.h
+++ b/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);

diff --git a/src/mon/MgrMap.h b/src/mon/MgrMap.h
index fc830a64259539e747016c4f6365397f2ac24def..a4ba431e22731dd517a75d66064a83485a609ef0 100644 (file)
--- a/src/mon/MgrMap.h
+++ b/src/mon/MgrMap.h
@@ -146,7 +146,7 @@ public:
     }
 
     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);

diff --git a/src/mon/mon_types.h b/src/mon/mon_types.h
index 11dca182acaf3839ccee97c1a2717b5da4adc72b..12cd3b450c41b32ed085b1d87c0d5ef1661b86ec 100644 (file)
--- a/src/mon/mon_types.h
+++ b/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);

diff --git a/src/osd/ECMsgTypes.cc b/src/osd/ECMsgTypes.cc
index ae0636f7d492a3f2b1605fcc58abf691b495e8d9..704a2aabd2da5410a595a0ca3edd92431375e0a7 100644 (file)
--- a/src/osd/ECMsgTypes.cc
+++ b/src/osd/ECMsgTypes.cc
@@ -220,7 +220,7 @@ void ECSubRead::decode(bufferlist::const_iterator &bl)
     decode(to_read, bl);
   }
   decode(attrs_to_read, bl);
-  if (struct_v > 2 && struct_v > struct_compat) {
+  if (struct_v > 2 && struct_v.v > struct_compat) {
     decode(subchunks, bl);
   } else {
     for (auto &i : to_read) {

diff --git a/src/osd/OSDMap.cc b/src/osd/OSDMap.cc
index ee071f1aa7134adf644b7e315d5e5535a01dbb4f..81fe9e3d4378430c6e70a3e549822755963bfb46 100644 (file)
--- a/src/osd/OSDMap.cc
+++ b/src/osd/OSDMap.cc
@@ -942,7 +942,7 @@ void OSDMap::Incremental::decode(ceph::buffer::list::const_iterator& bl)
   }
 
   {
-    DECODE_START(10, bl); // extended, osd-only data
+    DECODE_START(12, bl); // extended, osd-only data
     decode(new_hb_back_up, bl);
     decode(new_up_thru, bl);
     decode(new_last_clean_interval, bl);
@@ -3637,7 +3637,7 @@ void OSDMap::decode(ceph::buffer::list::const_iterator& bl)
   }
 
   {
-    DECODE_START(10, bl); // extended, osd-only data
+    DECODE_START(12, bl); // extended, osd-only data
     decode(osd_addrs->hb_back_addrs, bl);
     decode(osd_info, bl);
     decode(blocklist, bl);

diff --git a/src/osd/osd_types.cc b/src/osd/osd_types.cc
index 8dde420b8d95930b82186291be7c7e0614d680dd..6be2270e4240a0ccd88b548efedb216a45c4f048 100644 (file)
--- a/src/osd/osd_types.cc
+++ b/src/osd/osd_types.cc
@@ -1499,7 +1499,7 @@ void pool_opts_t::encode(ceph::buffer::list& bl, uint64_t features) const
 
 void pool_opts_t::decode(ceph::buffer::list::const_iterator& bl)
 {
-  DECODE_START(1, bl);
+  DECODE_START(2, bl);
   __u32 n;
   decode(n, bl);
   opts.clear();
@@ -3718,7 +3718,7 @@ void pg_notify_t::encode(ceph::buffer::list &bl) const
 
 void pg_notify_t::decode(ceph::buffer::list::const_iterator &bl)
 {
-  DECODE_START(3, bl);
+  DECODE_START(4, bl);
   decode(query_epoch, bl);
   decode(epoch_sent, bl);
   decode(info, bl);
@@ -4570,7 +4570,7 @@ void ObjectModDesc::visit(Visitor *visitor) const
   auto bp = bl.cbegin();
   try {
     while (!bp.end()) {
-      DECODE_START(max_required_version, bp);
+      DECODE_START_UNCHECKED(max_required_version, bp);
       uint8_t code;
       decode(code, bp);
       switch (code) {

diff --git a/src/rgw/driver/rados/rgw_obj_manifest.h b/src/rgw/driver/rados/rgw_obj_manifest.h
index af0ce9ce0fa37c3ad7c5319c44d1b57a8c5a3d0d..73890c708b72cafb440d2ee2fb63d7ec3af71b1d 100644 (file)
--- a/src/rgw/driver/rados/rgw_obj_manifest.h
+++ b/src/rgw/driver/rados/rgw_obj_manifest.h
@@ -297,7 +297,7 @@ public:
   }
 
   void decode(bufferlist::const_iterator& bl) {
-    DECODE_START_LEGACY_COMPAT_LEN_32(7, 2, 2, bl);
+    DECODE_START_LEGACY_COMPAT_LEN_32(8, 2, 2, bl);
     decode(obj_size, bl);
     decode(objs, bl);
     if (struct_v >= 3) {

diff --git a/src/rgw/rgw_lc.h b/src/rgw/rgw_lc.h
index cc6a7e51a1d14da30ae257fcd62bfb941d52634c..0cdba1365ca20b6b27734b8465aaf7178812336d 100644 (file)
--- a/src/rgw/rgw_lc.h
+++ b/src/rgw/rgw_lc.h
@@ -295,7 +295,7 @@ public:
     ENCODE_FINISH(bl);
   }
   void decode(bufferlist::const_iterator& bl) {
-    DECODE_START(3, bl);
+    DECODE_START(4, bl);
     decode(prefix, bl);
     if (struct_v >= 2) {
       decode(obj_tags, bl);

diff --git a/src/rgw/rgw_meta_sync_status.h b/src/rgw/rgw_meta_sync_status.h
index f8a2ae3ee7ba3b94b6bdef143cc9973756ca86ea..daa6e9ed46daed8c5f8a16e2be09a3569e599ce4 100644 (file)
--- a/src/rgw/rgw_meta_sync_status.h
+++ b/src/rgw/rgw_meta_sync_status.h
@@ -29,7 +29,7 @@ struct rgw_meta_sync_info {
   }
 
   void decode(bufferlist::const_iterator& bl) {
-    DECODE_START(1, bl);
+    DECODE_START(2, bl);
     decode(state, bl);
     decode(num_shards, bl);
     if (struct_v >= 2) {

diff --git a/src/rgw/rgw_pubsub.h b/src/rgw/rgw_pubsub.h
index 176ada952042b3bef69d9d4e870eaab4dfd1d953..e96a6aa28c3866ba6cb70ab64964075ed615a0ad 100644 (file)
--- a/src/rgw/rgw_pubsub.h
+++ b/src/rgw/rgw_pubsub.h
@@ -281,7 +281,7 @@ struct rgw_pubsub_dest {
   }
 
   void decode(bufferlist::const_iterator& bl) {
-    DECODE_START(5, bl);
+    DECODE_START(7, bl);
     std::string dummy;
     decode(dummy, bl);
     decode(dummy, bl);

diff --git a/src/rgw/rgw_zone_types.h b/src/rgw/rgw_zone_types.h
index 41c015692a90c532646ed569e7ac0caca7e3960f..e7df454b9f8d10ab9038d74ea8d98732036396ae 100644 (file)
--- a/src/rgw/rgw_zone_types.h
+++ b/src/rgw/rgw_zone_types.h
@@ -573,7 +573,7 @@ struct RGWZoneGroupPlacementTier {
   }
 
   void decode(bufferlist::const_iterator& bl) {
-    DECODE_START(2, bl);
+    DECODE_START(3, bl);
     decode(tier_type, bl);
     decode(storage_class, bl);
     decode(retain_head_object, bl);