Visual Studio 2015 build results

[Jean-Pierre Münch]

Hey everyone,

I've ran the build of our current GitHub hosted sources on my VS 2015 installation and found the following warnings (build related, migration's a different story)

The following warnings appear in Debug and Release 32-bit and 64-bit builds:

We're getting a C4297 on the destructor of AlgorithmParameterBase (line 271 in algparam.h). The fix would be to remove this line. The problem I see here: Why do we even have a conditional throw in a destructor? Is there any really good rationale there? Is there any way to do whatever the throw should do without throwing?

We're getting another C4297 in the ThreadLocalStorage destructor (line 38 in trdlocal.cpp) which throws on free-error. I'm not sure how to mitigate this one.

We're getting a few C4996 warnings in sockeft.cpp (lines 102, 136, 140) because we're using a deprecated API there. The called functions were considered standard some years ago, but will be replaced in the future. The compiler proposes replacing "inet_addr" by "inet_pton() or InetPton()" and replacing "gethostbyname" by "getaddrinfo() or GetAddrInfoW()". I think we need to check availability of these instructions on VS 2005 before actually doing this.

Next is a standard C4242 in zdeflate.cpp (line 642), we're converting an unsigned integer (32-bit) into a word16. I think we can safely change the type of the m_blockLength from "unsigned int" to "word16" as the assert already implies the range beyond 16 bit was never intended.

Now comes another C4242 in nbtheory.cpp (line 48), we're converting an unsigned integer (32-bit) into a word16. I think we can safely change the type of the p from "unsigned int" to "word16" as the vector where it's being pushed into already uses word16s.

Finally we're getting a bunch of LNK4221 warnings for strciphr.obj (= compiled version of strciphr.cpp), simple.obj, polynomi.obj and algebra.obj. I just checked and simple.cpp actually does nothing, algebra.cpp is sometimes included in algebra.h to provide manual instantiations of templates, strciphr.cpp does the same as algebra.cpp for strciphr.h, polynomi.cpp is the same case as algebra.
This error happened on static library build and we could fix it by manually excluding those source files from the build, the #define-constant necessary for inclusion is only defined in a dll build apparently. (This one seems to be debug-exclusive)

The last one on the list is a C4191 in dll.cpp (line 85), we're doing an unsafe (function?) pointer conversion there. It looks like we have to do pointer conversion there so may can fix this by using static_cast<>() instead of the C-style conversion. But I'm not sure about this one.

What I'll tackle tomorrow:
Try to apply the proposed fixes above, create up to 7 PRs for GitHub (up to one for each change) and see what happens in the crazy DLL-Import builds then.

BR

JPM

[Jeffrey Walton]

On Friday, July 24, 2015 at 4:54:39 PM UTC-4, jean-pierre.muench wrote:

Hey everyone,

I've ran the build of our current GitHub hosted sources on my VS 2015 installation and found the following warnings (build related, migration's a different story)

Can you send that to me or make it a PasteBin?
 

What I'll tackle tomorrow:
Try to apply the proposed fixes above, create up to 7 PRs for GitHub (up to one for each change) and see what happens in the crazy DLL-Import builds then.

My personal feelings: The DLL-Import stuff is hosed. Don't waste too much time or effort on it.

Its hosed because that particular subproject was stood up just for a FIPS 140-validation. The Testing Lab required a Security Boundary due to an Implementation Guidance (IG) doc from the Cryptographic Module Validation Program (CMVP). The security boundary was created by the exported interfaces from the DLL. So the DLL export/import stuff was cut-in. That's also why things like HexEncoders are missing, and non-FIPS algorithms are missing.

I'm not opposed to a DLL or even a FIPS DLL. We just need to take the time to re-architect it so the existing pain points go away.

Jeff

[Jean-Pierre Münch]

Hey everyone,

I've gone through all the stuff. (not the special dll import stuff yet)

I could fix a few things without much trouble however there were three issues that were outstanding.

1. The destructor of AlgorithmParametersBase. It can throw under specific circumstances. However destructors are assumed not to throw and I think it's considered bad style if they do (there's no one to catch usually). Here's the code and how I disabled the warning for now (not in the proposed patch):
   

   virtual ~AlgorithmParametersBase()
   	{
   #ifdef CRYPTOPP_UNCAUGHT_EXCEPTION_AVAILABLE
   		if (!std::uncaught_exception())
   #else
   		try
   #endif
   		{
   			//if (m_throwIfNotUsed && !m_used)
   			//	throw ParameterNotUsed(m_name);
   		}
   #ifndef CRYPTOPP_UNCAUGHT_EXCEPTION_AVAILABLE
   		catch(...)
   		{
   		}
   #endif
   	}
   

   I don't know how to properly handle the warning generated by this.
2. We have the same issue with the destructor of ThreadLocalStorage. I don't know how to handle this one either. Here's the code in question:
   

   ThreadLocalStorage::~ThreadLocalStorage()
   {
   #ifdef HAS_WINTHREADS
   	//if (!TlsFree(m_index))
   	//	throw Err("TlsFree", GetLastError());
   #else
   	int error = pthread_key_delete(m_index);
   	if (error)
   		throw Err("pthread_key_delete", error);
   #endif
   }
   

3. Our network interface uses deprecated functions. They still work in VS 2015 but are deprecated and we get a warning. However, fixing this warning would either mean defining the silencing constant or migrating to the new API. The problem with the new API is that it requires Windows Vista at a minimum, meaning we'd drop support for Windows XP. So I'd say we stay with the old API as long as we can and migrate later hoping that XP won't be required than. Anyone knows of any cases where we actually have to provide (full) XP support?

So for my fixes (mainly type conversions), the patch is attached.
I can create a PR for the main repo on GitHub if necessary. (Fork repo: https://github.com/DevJPM/cryptopp-1 Commit: https://github.com/DevJPM/cryptopp-1/commit/2d5497a8b8666e051029440890a7809bd006700e )
I don't know why the ptr_diff stuff got changed...

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 Saturday, July 25, 2015 at 1:16:11 PM UTC-4, jean-pierre.muench wrote:

Hey everyone,

I've gone through all the stuff. (not the special dll import stuff yet)

I could fix a few things without much trouble however there were three issues that were outstanding.

1. The destructor of AlgorithmParametersBase. It can throw under specific circumstances. However destructors are assumed not to throw and I think it's considered bad style if they do (there's no one to catch usually). Here's the code and how I disabled the warning for now (not in the proposed patch)...

OK, at 10,000 feet, there are a few things going on here:

(1) What does C++ provide or recommend
(2) What the library guarantees
(3) What users expect

Related to (1) we also have:

(4) What version of C++ are we working with (or against)?

So, before we move to implementation changes, I think we should understand the intersection of (1) - (3).

For (1), we can start at https://stackoverflow.com/questions/130117/throwing-exceptions-out-of-a-destructor. But we also need to know what has changed with C++11 and C++14.

In the case of (2) and (3), AlgorithmParameters has a contractual obligation to throw (modulo some conditions, like the user specifically asked throwIfNotUsed = false). So its likely going to throw in some circumstances, even if its unpalatable for others.

*****
For AlgorithmParameters, maybe we can change some internals so that the implementation is tear off (similar to a PIMPL):

class AlgorithmParameters
{
    AlgorithmParameters(bool throwIfUnused);
    ...
private:
    AlgorithmParametersImpl* m_impl;
}

AlgorithmParameters::AlgorithmParameters(bool throwIfUnused)
    : m_impl(AlgorithmParametersHelper(throwIfUnused)
{

}

AlgorithmParametersImpl* AlgorithmParametersHelper(bool throwIfUnused)
{
    if(throwIfUnused)
        return new AlgorithmParametersThrowIfUnused;
    else
        return new AlgorithmParametersDoNotThrowIfUnused;
}

*****

So here's what I was thinking for the immediate future:

(A) we need to study it some more before we move forward.

*****

Looking long term, I despise this pattern:

try
{
    ...
}
catch(...)
{
   // Swallow everything
}

As a matter of C++ policy, I'll begrudgingly agree to catching and swallowing a "CryptoPP::Exception&" in dtors. I'll agree because its working and playing well with others, so it speaks to (3) and addresses (1) above.

However, I don't think we should catch and swallow a "std::exception&" or "std::runtime_error&" or "...". The library is layered on top of the runtime, and there usually no way the library can claim it knows how to handle a C++ runtime problem. If the library knew there was a problem, then it should have avoid the state in the first place...