From: Josh Durgin <josh.durgin@inktank.com>
Date: Fri, 7 Feb 2014 02:50:50 +0000 (-0800)
Subject: ReplicatedPG: avoid undefined behavior in xattr comparison
X-Git-Tag: v0.78~154^2~5
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=e550ad79154ead3c276a2cdefc75ced02c43d4c4;p=ceph.git

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 4d4ece4faebf..8b219147e22c 100644
--- 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 198a6c6ca024..3202c658a924 100644
--- 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 c540c697ae2b..90fb8afc078b 100644
--- 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));