osd: fix UBSan shift overflow in pg_pool_t::calc_pg_masks

UBSan reports: shift exponent 32 is too large for 32-bit type 'int'

When pg_num or pgp_num is 0, pg_num-1 wraps to UINT_MAX (unsigned),
cbits() returns 32, and (1 << 32) on 32-bit int is undefined behavior

Use 1ULL for the shift and guard when cbits >= sizeof(unsigned)*CHAR_BIT
to avoid UB

- add unittest-crimson-pg-pool-shift to exercise the fix

Signed-off-by: Alexander Indenbaum <aindenba@redhat.com>

diff --git a/src/osd/osd_types.cc b/src/osd/osd_types.cc
index 91adfdd40f6c257387c6b0df5f5c6e4b0562d5ad..1f89038f9708be3d846e2a10dd9a6e9207b166d7 100644 (file)
--- a/src/osd/osd_types.cc
+++ b/src/osd/osd_types.cc
@@ -1695,8 +1695,16 @@ void pg_pool_t::convert_to_pg_shards(const vector<int> &from, set<pg_shard_t>* t
 
 void pg_pool_t::calc_pg_masks()
 {
-  pg_num_mask = (1 << cbits(pg_num-1)) - 1;
-  pgp_num_mask = (1 << cbits(pgp_num-1)) - 1;
+  // Use 1ULL to avoid undefined behavior: (1 << 32) is UB for 32-bit int
+  // when cbits returns 32 (e.g. pg_num-1 wraps to UINT_MAX when pg_num==0)
+  unsigned b1 = cbits(pg_num - 1);
+  unsigned b2 = cbits(pgp_num - 1);
+  pg_num_mask = (b1 >= sizeof(unsigned) * CHAR_BIT)
+    ? static_cast<unsigned>(-1)
+    : (static_cast<unsigned>((1ULL << b1) - 1));
+  pgp_num_mask = (b2 >= sizeof(unsigned) * CHAR_BIT)
+    ? static_cast<unsigned>(-1)
+    : (static_cast<unsigned>((1ULL << b2) - 1));
 }
 
 unsigned pg_pool_t::get_pg_num_divisor(pg_t pgid) const

diff --git a/src/test/crimson/CMakeLists.txt b/src/test/crimson/CMakeLists.txt
index 8a8c82ae5e7bd10b4f90c10a9519367d298f9072..48484410a064307e3622ff18065b33490bd50ad6 100644 (file)
--- a/src/test/crimson/CMakeLists.txt
+++ b/src/test/crimson/CMakeLists.txt
@@ -135,3 +135,12 @@ target_link_libraries(
   unittest-crimson-scrub
   crimson-common
   crimson::gtest)
+
+add_executable(unittest-crimson-pg-pool-shift
+  test_pg_pool_shift.cc)
+add_ceph_test(unittest-crimson-pg-pool-shift
+  ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/unittest-crimson-pg-pool-shift
+  --memory 256M --smp 1)
+target_link_libraries(unittest-crimson-pg-pool-shift
+  crimson-common
+  crimson::gtest)

diff --git a/src/test/crimson/test_pg_pool_shift.cc b/src/test/crimson/test_pg_pool_shift.cc
new file mode 100644 (file)
index 0000000..3be24ea
--- /dev/null
+++ b/src/test/crimson/test_pg_pool_shift.cc
@@ -0,0 +1,32 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:nil -*-
+// vim: ts=8 sw=2 sts=2 expandtab
+//
+// Minimal test that triggers calc_pg_masks() with pg_num=0.
+// When pg_num=0, pg_num-1 wraps to UINT_MAX, cbits() returns 32, and
+// the original (1 << 32) on 32-bit int is undefined behavior.
+// UBSan reports: "shift exponent 32 is too large for 32-bit type 'int'"
+// Run with ASan+UBSan: ./unittest-crimson-pg-pool-shift
+
+#include "test/crimson/gtest_seastar.h"
+#include "osd/osd_types.h"
+
+struct pg_pool_shift_test_t : public seastar_test_suite_t {};
+
+TEST_F(pg_pool_shift_test_t, calc_pg_masks_pg_num_zero)
+{
+  run_async([] {
+    pg_pool_t pool;
+    pool.set_pg_num(0);  // pg_num-1 wraps to UINT_MAX -> cbits=32 -> (1<<32) UB without fix
+    // With fix: mask = (unsigned)-1 when cbits >= 32
+    EXPECT_EQ(pool.get_pg_num_mask(), static_cast<unsigned>(-1));
+  });
+}
+
+TEST_F(pg_pool_shift_test_t, calc_pg_masks_pgp_num_zero)
+{
+  run_async([] {
+    pg_pool_t pool;
+    pool.set_pgp_num(0);  // same UB path for pgp_num
+    EXPECT_EQ(pool.get_pgp_num_mask(), static_cast<unsigned>(-1));
+  });
+}