From: Radoslaw Zarzynski Date: Sun, 6 Oct 2024 11:13:57 +0000 (+0000) Subject: dencoding: check struct_v against DECODE_START(v, ...) at compile-time X-Git-Tag: v20.3.0~263^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=fbecd5d238b5cf4a53508082a4fb8c1b80c143a9;p=ceph.git 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 --- diff --git a/src/common/versioned_variant.h b/src/common/versioned_variant.h index 124c58839169..5405222473d7 100644 --- a/src/common/versioned_variant.h +++ b/src/common/versioned_variant.h @@ -203,7 +203,7 @@ void decode(std::variant& 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(index, [&v, &p] (auto I) { // default-construct the type at index I and call its decoder decode(v.template emplace(), p); diff --git a/src/include/denc.h b/src/include/denc.h index 65b41b447981..d945e2646896 100644 --- a/src/include/denc.h +++ b/src/include/denc.h @@ -1790,6 +1790,42 @@ inline std::enable_if_t 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 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 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 f53a04837ece..fc8825c6492c 100644 --- a/src/include/encoding.h +++ b/src/include/encoding.h @@ -1493,7 +1493,22 @@ decode(std::array& 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& 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 d76a15e557ce..da0bc2288194 100644 --- 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 7bddb58b48fe..361551c6e370 100644 --- 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 75cd3fbadefa..ea8b78bb0f69 100644 --- 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 c432fe4beb3b..4dc929bd0092 100644 --- 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 be34e8d4f7cc..0b8a7731c00c 100644 --- 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 fc830a642595..a4ba431e2273 100644 --- 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 11dca182acaf..12cd3b450c41 100644 --- 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 ae0636f7d492..704a2aabd2da 100644 --- 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 ee071f1aa713..81fe9e3d4378 100644 --- 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 8dde420b8d95..6be2270e4240 100644 --- 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 af0ce9ce0fa3..73890c708b72 100644 --- 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 cc6a7e51a1d1..0cdba1365ca2 100644 --- 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 f8a2ae3ee7ba..daa6e9ed46da 100644 --- 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 176ada952042..e96a6aa28c38 100644 --- 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 41c015692a90..e7df454b9f8d 100644 --- 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);