Should SafeConvert assert?

[Jeffrey Walton]

With the proposed changes for SafeConvert (see below), I'm wondering if we should add some asserts for debug builds.

The assert will alert of a potential problem with the conversion, so those who don't check return values will be made aware of potential problems with their code.

CRYPTOPP_ASSERT raises a SIGTRAP, so it won't degrade the debugging experience. Under GDB, the user can press "c" to continue.

**********

template <class T1, class T2>
inline bool SafeConvert(T1 from, T2 &to)
{
    // Original code: always perform the assignment
    to = (T2)from;
 
    // Check for sign difference
    if(std::numeric_limits<T1>::is_signed ^ std::numeric_limits<T2>::is_signed)
    {
        // Handle T1 is signed
        if(std::numeric_limits<T1>::is_signed && from < 0)
            return false;
       
        // Fall through for T1 is unsigned
    }
   
    if(from > static_cast<T1>(std::numeric_limits<T2>::max()))
        return false;
       
    return true;
}

[Jean-Pierre Münch]

I'm not sure if you have already committed this one, but I'd say everything that increases awareness of potential problems is a good thing and making devs aware of these issues is even better as bad conversions are a common source of exploit vectors (IIRC).

So I'd say: Let SafeConvert assert on error!

BR

JPM

--
--
You received this message because you are subscribed to the "Crypto++ Users" Google Group.
To unsubscribe, send an email to cryptopp-user...@googlegroups.com.
More information about Crypto++ and this group is available at http://www.cryptopp.com.
---
You received this message because you are subscribed to the Google Groups "Crypto++ Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cryptopp-user...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Jeffrey Walton]

On Thursday, July 30, 2015 at 5:17:40 PM UTC-4, jean-pierre.muench wrote:

I'm not sure if you have already committed this one,

I actually backed them out yesterday. Getting the implementation right was kind of tricky, and it was turning into a time sink.

Te original code had some opportunities for improvement in terms of efficiency, but it was right every time. Its kind of like Jon Bentley said, "If it doesn't have to be correct, I can make it as fast as you'd like it to be".

(We even added a validat0.cpp that is mostly full of SafeConvert tests).
 

but I'd say everything that increases awareness of potential problems is a good thing and making devs aware of these issues is even better as bad conversions are a common source of exploit vectors (IIRC).

So I'd say: Let SafeConvert assert on error!

OK, done. https://github.com/weidai11/cryptopp/commit/e9c086aaac1a68a03a711d12058cd42673fcd1cd.

Jeff