fix operator>=(bufferlist& l, bufferlist& r)
diff mbox

Message ID 1361005875-768-1-git-send-email-loic@dachary.org
State New, archived
Headers show

Commit Message

Loic Dachary Feb. 16, 2013, 9:11 a.m. UTC
bufferlist a;
  a.append("A");
  bufferlist ab;
  ab.append("AB");

a >= ab failed, throwing an instance of 'ceph::buffer::end_of_buffer'
because it tried to access a[1]. All comparison operators should be
tested using a lexicographic sort like strcmp or memcmp (-1, 0, 1).
In the meantime, the missing test is added:

  if (l.length() == p && r.length() > p) return false;

A set of unit tests demonstrating the problem and covering all comparison
operators are added to show that the proposed fix works as expected.

http://tracker.ceph.com/issues/4157 refs #4157

Signed-off-by: Loic Dachary <loic@dachary.org>
---
 src/include/buffer.h   |    1 +
 src/test/bufferlist.cc |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

Comments

Sage Weil Feb. 16, 2013, 10:52 p.m. UTC | #1
Applied.  Thanks, Loic!

On Sat, 16 Feb 2013, Loic Dachary wrote:

>   bufferlist a;
>   a.append("A");
>   bufferlist ab;
>   ab.append("AB");
> 
> a >= ab failed, throwing an instance of 'ceph::buffer::end_of_buffer'
> because it tried to access a[1]. All comparison operators should be
> tested using a lexicographic sort like strcmp or memcmp (-1, 0, 1).
> In the meantime, the missing test is added:
> 
>   if (l.length() == p && r.length() > p) return false;
> 
> A set of unit tests demonstrating the problem and covering all comparison
> operators are added to show that the proposed fix works as expected.
> 
> http://tracker.ceph.com/issues/4157 refs #4157
> 
> Signed-off-by: Loic Dachary <loic@dachary.org>
> ---
>  src/include/buffer.h   |    1 +
>  src/test/bufferlist.cc |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/src/include/buffer.h b/src/include/buffer.h
> index 4f87ed7..b84e7f4 100644
> --- a/src/include/buffer.h
> +++ b/src/include/buffer.h
> @@ -467,6 +467,7 @@ inline bool operator>=(bufferlist& l, bufferlist& r) {
>    for (unsigned p = 0; ; p++) {
>      if (l.length() > p && r.length() == p) return true;
>      if (r.length() == p && l.length() == p) return true;
> +    if (l.length() == p && r.length() > p) return false;
>      if (l[p] > r[p]) return true;
>      if (l[p] < r[p]) return false;
>    }
> diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
> index 91e37e6..71c2e79 100644
> --- a/src/test/bufferlist.cc
> +++ b/src/test/bufferlist.cc
> @@ -57,6 +57,53 @@ TEST(BufferList, zero) {
>    }
>  }
>  
> +TEST(BufferList, compare) {
> +  bufferlist a;
> +  a.append("A");
> +  bufferlist ab;
> +  ab.append("AB");
> +  bufferlist ac;
> +  ac.append("AC");
> +  //
> +  // bool operator>(bufferlist& l, bufferlist& r)
> +  //
> +  ASSERT_FALSE(a > ab);
> +  ASSERT_TRUE(ab > a);
> +  ASSERT_TRUE(ac > ab);
> +  ASSERT_FALSE(ab > ac);
> +  ASSERT_FALSE(ab > ab);
> +  //
> +  // bool operator>=(bufferlist& l, bufferlist& r)
> +  //
> +  ASSERT_FALSE(a >= ab);
> +  ASSERT_TRUE(ab >= a);
> +  ASSERT_TRUE(ac >= ab);
> +  ASSERT_FALSE(ab >= ac);
> +  ASSERT_TRUE(ab >= ab);
> +  //
> +  // bool operator<(bufferlist& l, bufferlist& r)
> +  //
> +  ASSERT_TRUE(a < ab);
> +  ASSERT_FALSE(ab < a);
> +  ASSERT_FALSE(ac < ab);
> +  ASSERT_TRUE(ab < ac);
> +  ASSERT_FALSE(ab < ab);
> +  //
> +  // bool operator<=(bufferlist& l, bufferlist& r)
> +  //
> +  ASSERT_TRUE(a <= ab);
> +  ASSERT_FALSE(ab <= a);
> +  ASSERT_FALSE(ac <= ab);
> +  ASSERT_TRUE(ab <= ac);
> +  ASSERT_TRUE(ab <= ab);
> +  //
> +  // bool operator==(bufferlist &l, bufferlist &r)
> +  //
> +  ASSERT_FALSE(a == ab);
> +  ASSERT_FALSE(ac == ab);
> +  ASSERT_TRUE(ab == ab);
> +}
> +
>  TEST(BufferList, EmptyAppend) {
>    bufferlist bl;
>    bufferptr ptr;
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/src/include/buffer.h b/src/include/buffer.h
index 4f87ed7..b84e7f4 100644
--- a/src/include/buffer.h
+++ b/src/include/buffer.h
@@ -467,6 +467,7 @@  inline bool operator>=(bufferlist& l, bufferlist& r) {
   for (unsigned p = 0; ; p++) {
     if (l.length() > p && r.length() == p) return true;
     if (r.length() == p && l.length() == p) return true;
+    if (l.length() == p && r.length() > p) return false;
     if (l[p] > r[p]) return true;
     if (l[p] < r[p]) return false;
   }
diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index 91e37e6..71c2e79 100644
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -57,6 +57,53 @@  TEST(BufferList, zero) {
   }
 }
 
+TEST(BufferList, compare) {
+  bufferlist a;
+  a.append("A");
+  bufferlist ab;
+  ab.append("AB");
+  bufferlist ac;
+  ac.append("AC");
+  //
+  // bool operator>(bufferlist& l, bufferlist& r)
+  //
+  ASSERT_FALSE(a > ab);
+  ASSERT_TRUE(ab > a);
+  ASSERT_TRUE(ac > ab);
+  ASSERT_FALSE(ab > ac);
+  ASSERT_FALSE(ab > ab);
+  //
+  // bool operator>=(bufferlist& l, bufferlist& r)
+  //
+  ASSERT_FALSE(a >= ab);
+  ASSERT_TRUE(ab >= a);
+  ASSERT_TRUE(ac >= ab);
+  ASSERT_FALSE(ab >= ac);
+  ASSERT_TRUE(ab >= ab);
+  //
+  // bool operator<(bufferlist& l, bufferlist& r)
+  //
+  ASSERT_TRUE(a < ab);
+  ASSERT_FALSE(ab < a);
+  ASSERT_FALSE(ac < ab);
+  ASSERT_TRUE(ab < ac);
+  ASSERT_FALSE(ab < ab);
+  //
+  // bool operator<=(bufferlist& l, bufferlist& r)
+  //
+  ASSERT_TRUE(a <= ab);
+  ASSERT_FALSE(ab <= a);
+  ASSERT_FALSE(ac <= ab);
+  ASSERT_TRUE(ab <= ac);
+  ASSERT_TRUE(ab <= ab);
+  //
+  // bool operator==(bufferlist &l, bufferlist &r)
+  //
+  ASSERT_FALSE(a == ab);
+  ASSERT_FALSE(ac == ab);
+  ASSERT_TRUE(ab == ab);
+}
+
 TEST(BufferList, EmptyAppend) {
   bufferlist bl;
   bufferptr ptr;