From 990dc983ed2634bc764e6c4c1e6d63499eff6d58 Mon Sep 17 00:00:00 2001 From: Max Vollmer Date: Wed, 7 Feb 2018 10:48:39 +0100 Subject: [PATCH 1/2] Issue #1776 Fixed potential crash bug in ObjectCompare, because it didn't follow strict weak ordering. As counter-intuitive as it seems, a comparator must return false for equal values. The C++ standard defines and expects this behavior: true if lhs < rhs, false otherwise. --- code/BlenderIntermediate.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/code/BlenderIntermediate.h b/code/BlenderIntermediate.h index 74184a1f1..f3d34d1b2 100644 --- a/code/BlenderIntermediate.h +++ b/code/BlenderIntermediate.h @@ -122,9 +122,11 @@ namespace Blender { # pragma warning(disable:4351) #endif + // As counter-intuitive as it may seem, a comparator must return false for equal values. + // The C++ standard defines and expects this behavior: true if lhs < rhs, false otherwise. struct ObjectCompare { bool operator() (const Object* left, const Object* right) const { - return ::strncmp(left->id.name, right->id.name, strlen( left->id.name ) ) == 0; + return ::strncmp(left->id.name, right->id.name, strlen( left->id.name ) ) < 0; } }; @@ -143,9 +145,11 @@ namespace Blender { , db(db) {} + // As counter-intuitive as it may seem, a comparator must return false for equal values. + // The C++ standard defines and expects this behavior: true if lhs < rhs, false otherwise. struct ObjectCompare { bool operator() (const Object* left, const Object* right) const { - return ::strncmp( left->id.name, right->id.name, strlen( left->id.name ) ) == 0; + return ::strncmp( left->id.name, right->id.name, strlen( left->id.name ) ) < 0; } }; From b3d48d0e9a5e815c2a4acc7662736bb902a362cd Mon Sep 17 00:00:00 2001 From: Max Vollmer Date: Wed, 7 Feb 2018 11:02:08 +0100 Subject: [PATCH 2/2] Issue #1776: Updated and fixed test in utBlenderIntermediate.cpp for Blender::ObjectCompare --- test/unit/utBlenderIntermediate.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/test/unit/utBlenderIntermediate.cpp b/test/unit/utBlenderIntermediate.cpp index 9146d6d1e..55490e613 100644 --- a/test/unit/utBlenderIntermediate.cpp +++ b/test/unit/utBlenderIntermediate.cpp @@ -57,17 +57,26 @@ class BlenderIntermediateTest : public ::testing::Test { #define NAME_1 "name1" #define NAME_2 "name2" +// Updated this test after fixing #1776: +// A comparator in C++ is used for ordering and must implement strict weak ordering, +// which means it must return false for equal values. +// The C++ standard defines and expects this behavior: true if lhs < rhs, false otherwise. TEST_F( BlenderIntermediateTest,ConversionData_ObjectCompareTest ) { Object obj1, obj2; strncpy( obj1.id.name, NAME_1, sizeof(NAME_1) ); strncpy( obj2.id.name, NAME_2, sizeof(NAME_2) ); - Blender::ObjectCompare cmp_false; - bool res( cmp_false( &obj1, &obj2 ) ); + + Blender::ObjectCompare cmp_true_because_first_is_smaller_than_second; + bool res( cmp_true_because_first_is_smaller_than_second( &obj1, &obj2 ) ); + EXPECT_TRUE( res ); + + Blender::ObjectCompare cmp_false_because_equal; + res = cmp_false_because_equal( &obj1, &obj1 ); EXPECT_FALSE( res ); - Blender::ObjectCompare cmp_true; - res = cmp_true( &obj1, &obj1 ); - EXPECT_TRUE( res ); + Blender::ObjectCompare cmp_false_because_first_is_greater_than_second; + res = cmp_false_because_first_is_greater_than_second( &obj2, &obj1 ); + EXPECT_FALSE( res ); }