#6229 closed defect (fixed)
Template function(s) for overflow checking
Reported by: | Kurt Schwehr | Owned by: | warmerdam |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | default | Version: | unspecified |
Severity: | normal | Keywords: | |
Cc: |
Description
size_t VSIMemHandle::Read( void * pBuffer, size_t nSize, size_t nCount ) { // FIXME: Integer overflow check should be placed here: size_t nBytesToRead = nSize * nCount;
Here is the start to a set of C++ templated functions to check for overflow. I propose these go in the cpl_conv.h. Comments on the implementation, where the code should go, or the template naming?
// Keep headers at the top. #if defined(__cplusplus) && !defined(CPL_SUPRESS_CPLUSPLUS) #include <limits> #endif // __cplusplus // yada yada #if defined(__cplusplus) && !defined(CPL_SUPRESS_CPLUSPLUS) // Return true if adding the two arguments will overflow their type. // Both arguments and the destination must be of the same type for this to be a valid check. template<typename T> bool CPLAddWillOverflow( T a, T b ) { if (a > b) { if ( ( std::numeric_limits<T>::max() - a ) < b ) return true; return false; } if ( ( std::numeric_limits<T>::max() - b ) < a ) return true; return false; } #endif // __cplusplus
It may be possible to check that both argument types are the same in C++14 and fail to compile if they are not with a static_assert.
Attached is a quick test c++ file.
Attachments (1)
Change History (7)
by , 8 years ago
Attachment: | integer_overflow.cc added |
---|
comment:1 by , 8 years ago
And I haven't looked at what happens with negative values for signed types
comment:2 by , 8 years ago
For unsigned types, this would be a bit expensive (no need to compare a > b ), so perhaps having a check for std::numeric_limits<T>::is_signed to have 2 implementations ? And perhaps add the 2 types as template classes and add a static assert they are the same (if that can be done directly, or otherwise test they have same signedness, bitdeph, integerness/floatingpointness) ?
comment:3 by , 8 years ago
In fact looking more at this issue, I'd suggest to match gcc 5 / clang 3.4 ( http://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins ) builtins similar operations, and root the implementation of them if available
#ifndef __has_builtin #define __has_builtin(x) 0 #endif bool CPL_uadd_overflow(unsigned int a, unsigned int b, unsigned int* pres) { #if __GNUC__ >= 5 || __has_builtin(__builtin_uadd_overflow) return __builtin_uadd_overflow(a,b,pres); #else *pres = a + b; return (a > 0xFFFFFFFFU - b); #endif }
The following with clang 3.4 / gcc 5.2 generates very efficient, branchless code :
0000000000000000 <_Z17CPL_uadd_overflowjjPj>: 0: 01 f7 add %esi,%edi 2: 0f 92 c0 setb %al 5: 89 3a mov %edi,(%rdx) 7: c3 retq
whereas the generic implementation generates (no branch, but still an extra not and cmp) :
0000000000000000 <_Z17CPL_uadd_overflowjjPj>: 0: 8d 04 3e lea (%rsi,%rdi,1),%eax 3: 89 02 mov %eax,(%rdx) 5: f7 d6 not %esi 7: 39 fe cmp %edi,%esi 9: 0f 92 c0 setb %al c: c3 retq
Example of using CPLAddWillOverflow() with unsigned short and short