ReplicatedPG: avoid undefined behavior in xattr comparison

Reading past the end of a pointer returned by string.data() in c++98
is undefined. While we're fixing this, also allow comparison of xattrs
containing null bytes.

Fixes: #7250
Backport: dumpling
Signed-off-by: Josh Durgin <josh.durgin@inktank.com>

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index 4d4ece4faebfe936f99ac653f2f5afa678cfb4d4..8b219147e22c782440c1e7142c4e007aebeda2ba 100644 (file)
--- a/PendingReleaseNotes
+++ b/PendingReleaseNotes
@@ -12,3 +12,6 @@ v0.78
 
 * The per-op return code in librados' ObjectWriteOperation interface
   is now filled in.
+
+* The librados cmpxattr operation now handles xattrs containing null bytes as
+  data rather than null-terminated strings.

diff --git a/src/osd/ReplicatedPG.cc b/src/osd/ReplicatedPG.cc
index 198a6c6ca024dd1b895a06fe95e99ab9b1c55847..3202c658a924f09ddc43bfdb2f249c7052c98652 100644 (file)
--- a/src/osd/ReplicatedPG.cc
+++ b/src/osd/ReplicatedPG.cc
@@ -2489,30 +2489,23 @@ int ReplicatedPG::do_xattr_cmp_u64(int op, __u64 v1, bufferlist& xattr)
 
 int ReplicatedPG::do_xattr_cmp_str(int op, string& v1s, bufferlist& xattr)
 {
-  const char *v1, *v2;
-  v1 = v1s.data();
-  string v2s;
-  if (xattr.length()) {
-    v2s = string(xattr.c_str(), xattr.length());
-    v2 = v2s.c_str();
-  } else
-    v2 = "";
+  string v2s(xattr.c_str(), xattr.length());
 
-  dout(20) << "do_xattr_cmp_str '" << v1s << "' vs '" << v2 << "' op " << op << dendl;
+  dout(20) << "do_xattr_cmp_str '" << v1s << "' vs '" << v2s << "' op " << op << dendl;
 
   switch (op) {
   case CEPH_OSD_CMPXATTR_OP_EQ:
-    return (strcmp(v1, v2) == 0);
+    return (v1s.compare(v2s) == 0);
   case CEPH_OSD_CMPXATTR_OP_NE:
-    return (strcmp(v1, v2) != 0);
+    return (v1s.compare(v2s) != 0);
   case CEPH_OSD_CMPXATTR_OP_GT:
-    return (strcmp(v1, v2) > 0);
+    return (v1s.compare(v2s) > 0);
   case CEPH_OSD_CMPXATTR_OP_GTE:
-    return (strcmp(v1, v2) >= 0);
+    return (v1s.compare(v2s) >= 0);
   case CEPH_OSD_CMPXATTR_OP_LT:
-    return (strcmp(v1, v2) < 0);
+    return (v1s.compare(v2s) < 0);
   case CEPH_OSD_CMPXATTR_OP_LTE:
-    return (strcmp(v1, v2) <= 0);
+    return (v1s.compare(v2s) <= 0);
   default:
     return -EINVAL;
   }

diff --git a/src/test/librados/c_read_operations.cc b/src/test/librados/c_read_operations.cc
index c540c697ae2bf759858e323c659bcc657513256d..90fb8afc078bf2744aae209ecd7fe47acbc63625 100644 (file)
--- a/src/test/librados/c_read_operations.cc
+++ b/src/test/librados/c_read_operations.cc
@@ -204,16 +204,15 @@ TEST_F(CReadOpsTest, CmpXattr) {
   EXPECT_EQ(-ECANCELED, cmp_xattr(xattr, buf, sizeof(buf), LIBRADOS_CMPXATTR_OP_LT));
   EXPECT_EQ(-ECANCELED, cmp_xattr(xattr, buf, sizeof(buf), LIBRADOS_CMPXATTR_OP_LTE));
 
-  // it compares C strings, so NUL at the beginning is the same
-  // regardless of the following data
-  rados_setxattr(ioctx, obj, xattr, "\0\0", 1);
+  // check that null bytes are compared correctly
+  rados_setxattr(ioctx, obj, xattr, "\0\0", 2);
   buf[0] = '\0';
-  EXPECT_EQ(1, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_EQ));
-  EXPECT_EQ(-ECANCELED, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_NE));
-  EXPECT_EQ(-ECANCELED, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_GT));
+  EXPECT_EQ(-ECANCELED, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_EQ));
+  EXPECT_EQ(1, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_NE));
+  EXPECT_EQ(1, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_GT));
   EXPECT_EQ(1, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_GTE));
   EXPECT_EQ(-ECANCELED, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_LT));
-  EXPECT_EQ(1, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_LTE));
+  EXPECT_EQ(-ECANCELED, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_LTE));
 
   buf[1] = '\0';
   EXPECT_EQ(1, cmp_xattr(xattr, buf, 2, LIBRADOS_CMPXATTR_OP_EQ));