Re: [patch] libstdc++/66327 don't pass null pointers to memcmp

[Jonathan Wakely]

On 30/05/15 09:36 +, Jörg Richter wrote:

How about comparing only the length against 0?
The pointers must be != 0 if length  0.

Like so? Yes, that's probably an improvement, because we need that
length anyway.


Yes, that is what I had in mind.
I think the second case can be handled equally by
precomputing std::min(__len1, __len2).


Yep, that's better too, I've tested and committed this patch. Thanks
for the suggestions.


commit 12ae71c3947824fa096113a418c7fe9e2ba89d19
Author: Jonathan Wakely jwak...@redhat.com
Date:   Fri May 29 23:48:55 2015 +0100

	* include/bits/stl_algobase.h (__equaltrue::equal): Check length
	instead of checking for null pointers.
	(__lexicographical_comparetrue::__lc): Only check shorter length.

diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h
index db065e2..12eb7ec 100644
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -812,11 +812,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 static bool
 equal(const _Tp* __first1, const _Tp* __last1, const _Tp* __first2)
 {
-	  if (__first1 == 0 || __first2 == 0)
-	return __first1 == __last1;
-
-	  return !__builtin_memcmp(__first1, __first2, sizeof(_Tp)
-   * (__last1 - __first1));
+	  if (const size_t __len = (__last1 - __first1))
+	return !__builtin_memcmp(__first1, __first2, sizeof(_Tp) * __len);
+	  return true;
 	}
 };
 
@@ -920,14 +918,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	{
 	  const size_t __len1 = __last1 - __first1;
 	  const size_t __len2 = __last2 - __first2;
-	  if (__len1  __len2)
-	{
-	  if (int __result = __builtin_memcmp(__first1, __first2,
-		  std::min(__len1, __len2)))
-		{
-		  return __result  0;
-		}
-	}
+	  if (const size_t __len = std::min(__len1, __len2))
+	if (int __result = __builtin_memcmp(__first1, __first2, __len))
+	  return __result  0;
 	  return __len1  __len2;
 	}
 };

[patch] libstdc++/66327 don't pass null pointers to memcmp

[Jonathan Wakely]

std::equal((int*)0, (int*)0, p) and std::equal(p, p, (int*)0) are
valid for any input iterator p, and must not pass a null pointer to
memcpy.

Similarly, std::lexicographical_compare((int*)0, (int*)0, p, q) and
std::lexicographical_compare(p, q, (int*)0, (int*)0) are valid for any
input iterators p and q, and must not pass a null pointer to memcpy.

This is a rather brute force fix, but if noone has any better ideas
I'll commit this to the trunk and 4.9 and 5 branches tomorrow. (I
think it should go on the branches, because 4.9 is known to optimise
away null pointer checks after invalid calls to memcmp like this).

I am not adding tests for these as we have no way to use ubsan in the
testsuite at present. I plan to make that possible, and will go back
and add tests using -fsanitize=undefined for all the recent fixes I've
made for ubsan errors.

Tested powerpc64le-linux.
commit 323133a6f623e827c7e70fa6918c7149e8932443
Author: Jonathan Wakely jwak...@redhat.com
Date:   Thu May 28 20:23:22 2015 +0100

	PR libstdc++/66327
	* include/bits/stl_algobase.h (__equaltrue::equal): Do not call
	memcmp with null pointers.
	(__lexicographical_comparetrue::__lc): Do not call memcmp for empty
	ranges.

diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h
index 409ef36..db065e2 100644
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -812,6 +812,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 static bool
 equal(const _Tp* __first1, const _Tp* __last1, const _Tp* __first2)
 {
+	  if (__first1 == 0 || __first2 == 0)
+	return __first1 == __last1;
+
 	  return !__builtin_memcmp(__first1, __first2, sizeof(_Tp)
    * (__last1 - __first1));
 	}
@@ -917,9 +920,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	{
 	  const size_t __len1 = __last1 - __first1;
 	  const size_t __len2 = __last2 - __first2;
-	  const int __result = __builtin_memcmp(__first1, __first2,
-		std::min(__len1, __len2));
-	  return __result != 0 ? __result  0 : __len1  __len2;
+	  if (__len1  __len2)
+	{
+	  if (int __result = __builtin_memcmp(__first1, __first2,
+		  std::min(__len1, __len2)))
+		{
+		  return __result  0;
+		}
+	}
+	  return __len1  __len2;
 	}
 };