Re: [PATCH] Avoid signed overflow in num_get::_M_extract_int (PR libstdc++/67214)

[Jonathan Wakely]

On 22/05/17 11:08 +0100, Jonathan Wakely wrote:

On 20/05/17 15:10 +0800, Xi Ruoyao wrote:

On 2017-05-19 15:38 +0100, Jonathan Wakely wrote:

On 18/05/17 19:10 +0800, Xi Ruoyao wrote:

This UB has been hiding so long...


Indeed! Thanks for the patch.


2017-03-11  Xi Ruoyao  

PR libstdc++/67214
* include/bits/locale_facets.tcc (_M_extract_int):
  Add explicit conversion to avoid signed overflow.
---
 libstdc++-v3/include/bits/locale_facets.tcc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/include/bits/locale_facets.tcc 
b/libstdc++-v3/include/bits/locale_facets.tcc
index 351190c..5f85d15 100644
--- a/libstdc++-v3/include/bits/locale_facets.tcc
+++ b/libstdc++-v3/include/bits/locale_facets.tcc
@@ -470,7 +470,8 @@ _GLIBCXX_BEGIN_NAMESPACE_LDBL
    bool __testoverflow = false;
    const __unsigned_type __max =
      (__negative && __gnu_cxx::__numeric_traits<_ValueT>::__is_signed)
-     ? -__gnu_cxx::__numeric_traits<_ValueT>::__min
+     ? -static_cast<__unsigned_type>(__gnu_cxx::
+    __numeric_traits<_ValueT>::__min)


Do we need to keep the negation, or can we just cast to
__unsigned_type?


For 2's complement we can just cast to __unsigned_type.  But for
clarity and other strange architectures I think we should keep
the negation.


https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html
"GCC only supports two's complement integer types"

I doubt that's ever going to change, but keeping the negation also
doesn't do any harm. I'll test and commit this, thanks.




Here's what I committed, which also adds a typedef for the
__numeric_traits type, to make everything more readable.

Tested powerpc64le-linux. Committed to trunk.


commit effe4603accc56b08eda6453eb1abf752cfaafba
Author: Jonathan Wakely 
Date:   Tue May 23 11:11:41 2017 +0100

PR libstdc++/67214 Avoid signed overflow in num_get::_M_extract_int

2017-05-23  Xi Ruoyao  

	PR libstdc++/67214
	* include/bits/locale_facets.tcc (num_get::_M_extract_int): Add
	explicit conversion to avoid signed overflow.

diff --git a/libstdc++-v3/include/bits/locale_facets.tcc b/libstdc++-v3/include/bits/locale_facets.tcc
index 351190c..d7710e6 100644
--- a/libstdc++-v3/include/bits/locale_facets.tcc
+++ b/libstdc++-v3/include/bits/locale_facets.tcc
@@ -375,10 +375,10 @@ _GLIBCXX_BEGIN_NAMESPACE_LDBL
   _M_extract_int(_InIter __beg, _InIter __end, ios_base& __io,
 		 ios_base::iostate& __err, _ValueT& __v) const
   {
-typedef char_traits<_CharT>			 __traits_type;
+typedef char_traits<_CharT>			__traits_type;
 	using __gnu_cxx::__add_unsigned;
-	typedef typename __add_unsigned<_ValueT>::__type __unsigned_type;
-	typedef __numpunct_cache<_CharT> __cache_type;
+	typedef typename __add_unsigned<_ValueT>::__type__unsigned_type;
+	typedef __numpunct_cache<_CharT>__cache_type;
 	__use_cache<__cache_type> __uc;
 	const locale& __loc = __io._M_getloc();
 	const __cache_type* __lc = __uc(__loc);
@@ -463,15 +463,16 @@ _GLIBCXX_BEGIN_NAMESPACE_LDBL
 			  - __num_base::_S_izero : __base);
 
 	// Extract.
+	typedef __gnu_cxx::__numeric_traits<_ValueT> __num_traits;
 	string __found_grouping;
 	if (__lc->_M_use_grouping)
 	  __found_grouping.reserve(32);
 	bool __testfail = false;
 	bool __testoverflow = false;
 	const __unsigned_type __max =
-	  (__negative && __gnu_cxx::__numeric_traits<_ValueT>::__is_signed)
-	  ? -__gnu_cxx::__numeric_traits<_ValueT>::__min
-	  : __gnu_cxx::__numeric_traits<_ValueT>::__max;
+	  (__negative && __num_traits::__is_signed)
+	  ? -static_cast<__unsigned_type>(__num_traits::__min)
+	  : __num_traits::__max;
 	const __unsigned_type __smax = __max / __base;
 	__unsigned_type __result = 0;
 	int __digit = 0;
@@ -572,11 +573,10 @@ _GLIBCXX_BEGIN_NAMESPACE_LDBL
 	  }
 	else if (__testoverflow)
 	  {
-	if (__negative
-		&& __gnu_cxx::__numeric_traits<_ValueT>::__is_signed)
-	  __v = __gnu_cxx::__numeric_traits<_ValueT>::__min;
+	if (__negative && __num_traits::__is_signed)
+	  __v = __num_traits::__min;
 	else
-	  __v = __gnu_cxx::__numeric_traits<_ValueT>::__max;
+	  __v = __num_traits::__max;
 	__err = ios_base::failbit;
 	  }
 	else

Re: [PATCH] Avoid signed overflow in num_get::_M_extract_int (PR libstdc++/67214)

[Jonathan Wakely]

On 20/05/17 15:10 +0800, Xi Ruoyao wrote:

On 2017-05-19 15:38 +0100, Jonathan Wakely wrote:

On 18/05/17 19:10 +0800, Xi Ruoyao wrote:
> This UB has been hiding so long...

Indeed! Thanks for the patch.

> 2017-03-11  Xi Ruoyao  
>
>PR libstdc++/67214
>* include/bits/locale_facets.tcc (_M_extract_int):
>  Add explicit conversion to avoid signed overflow.
> ---
>  libstdc++-v3/include/bits/locale_facets.tcc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libstdc++-v3/include/bits/locale_facets.tcc 
b/libstdc++-v3/include/bits/locale_facets.tcc
> index 351190c..5f85d15 100644
> --- a/libstdc++-v3/include/bits/locale_facets.tcc
> +++ b/libstdc++-v3/include/bits/locale_facets.tcc
> @@ -470,7 +470,8 @@ _GLIBCXX_BEGIN_NAMESPACE_LDBL
>    bool __testoverflow = false;
>    const __unsigned_type __max =
>      (__negative && __gnu_cxx::__numeric_traits<_ValueT>::__is_signed)
> -    ? -__gnu_cxx::__numeric_traits<_ValueT>::__min
> +    ? -static_cast<__unsigned_type>(__gnu_cxx::
> +   __numeric_traits<_ValueT>::__min)

Do we need to keep the negation, or can we just cast to
__unsigned_type?


For 2's complement we can just cast to __unsigned_type.  But for
clarity and other strange architectures I think we should keep
the negation.


https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html
"GCC only supports two's complement integer types"

I doubt that's ever going to change, but keeping the negation also
doesn't do any harm. I'll test and commit this, thanks.

Re: [PATCH] Avoid signed overflow in num_get::_M_extract_int (PR libstdc++/67214)

[Xi Ruoyao]

On 2017-05-19 15:38 +0100, Jonathan Wakely wrote:
> On 18/05/17 19:10 +0800, Xi Ruoyao wrote:
> > This UB has been hiding so long...
> 
> Indeed! Thanks for the patch.
> 
> > 2017-03-11  Xi Ruoyao  
> > 
> > PR libstdc++/67214
> > * include/bits/locale_facets.tcc (_M_extract_int):
> >   Add explicit conversion to avoid signed overflow.
> > ---
> >  libstdc++-v3/include/bits/locale_facets.tcc | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libstdc++-v3/include/bits/locale_facets.tcc 
> > b/libstdc++-v3/include/bits/locale_facets.tcc
> > index 351190c..5f85d15 100644
> > --- a/libstdc++-v3/include/bits/locale_facets.tcc
> > +++ b/libstdc++-v3/include/bits/locale_facets.tcc
> > @@ -470,7 +470,8 @@ _GLIBCXX_BEGIN_NAMESPACE_LDBL
> >     bool __testoverflow = false;
> >     const __unsigned_type __max =
> >       (__negative && __gnu_cxx::__numeric_traits<_ValueT>::__is_signed)
> > -     ? -__gnu_cxx::__numeric_traits<_ValueT>::__min
> > +     ? -static_cast<__unsigned_type>(__gnu_cxx::
> > +    __numeric_traits<_ValueT>::__min)
> 
> Do we need to keep the negation, or can we just cast to
> __unsigned_type?

For 2's complement we can just cast to __unsigned_type.  But for
clarity and other strange architectures I think we should keep
the negation.
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Avoid signed overflow in num_get::_M_extract_int (PR libstdc++/67214)

[Jonathan Wakely]

On 18/05/17 19:10 +0800, Xi Ruoyao wrote:

This UB has been hiding so long...


Indeed! Thanks for the patch.


2017-03-11  Xi Ruoyao  

PR libstdc++/67214
* include/bits/locale_facets.tcc (_M_extract_int):
  Add explicit conversion to avoid signed overflow.
---
 libstdc++-v3/include/bits/locale_facets.tcc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/include/bits/locale_facets.tcc 
b/libstdc++-v3/include/bits/locale_facets.tcc
index 351190c..5f85d15 100644
--- a/libstdc++-v3/include/bits/locale_facets.tcc
+++ b/libstdc++-v3/include/bits/locale_facets.tcc
@@ -470,7 +470,8 @@ _GLIBCXX_BEGIN_NAMESPACE_LDBL
    bool __testoverflow = false;
    const __unsigned_type __max =
      (__negative && __gnu_cxx::__numeric_traits<_ValueT>::__is_signed)
-     ? -__gnu_cxx::__numeric_traits<_ValueT>::__min
+     ? -static_cast<__unsigned_type>(__gnu_cxx::
+    __numeric_traits<_ValueT>::__min)


Do we need to keep the negation, or can we just cast to
__unsigned_type?

[PATCH] Avoid signed overflow in num_get::_M_extract_int (PR libstdc++/67214)

[Xi Ruoyao]

This UB has been hiding so long...

2017-03-11  Xi Ruoyao  

PR libstdc++/67214
* include/bits/locale_facets.tcc (_M_extract_int):
  Add explicit conversion to avoid signed overflow.
---
 libstdc++-v3/include/bits/locale_facets.tcc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/include/bits/locale_facets.tcc 
b/libstdc++-v3/include/bits/locale_facets.tcc
index 351190c..5f85d15 100644
--- a/libstdc++-v3/include/bits/locale_facets.tcc
+++ b/libstdc++-v3/include/bits/locale_facets.tcc
@@ -470,7 +470,8 @@ _GLIBCXX_BEGIN_NAMESPACE_LDBL
    bool __testoverflow = false;
    const __unsigned_type __max =
      (__negative && __gnu_cxx::__numeric_traits<_ValueT>::__is_signed)
-     ? -__gnu_cxx::__numeric_traits<_ValueT>::__min
+     ? -static_cast<__unsigned_type>(__gnu_cxx::
+    __numeric_traits<_ValueT>::__min)
      : __gnu_cxx::__numeric_traits<_ValueT>::__max;
    const __unsigned_type __smax = __max / __base;
    __unsigned_type __result = 0;
-- 
2.7.1

-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University