weidai11 / cryptopp

free C++ class library of cryptographic schemes
https://cryptopp.com
Other
4.89k stars 1.51k forks source link

Recent Windows 8 and Windows 10 support broke Windows 7/VS 2012 #178

Closed noloader closed 8 years ago

noloader commented 8 years ago

Recently we added better support for Windows 8, Windows 10, Windows Store, and other newer Windows platforms. It appears Microsoft is doing something unexpected, and its causing cryptest.exe to fail under Windows 7/Visual Studio 2012. Running cryptest.exe results in: _The procedure entry point GetOverlappedResultEx could not be located in the dynamic link library KERNEL32.dll_.

windows-entry-point

Calling the newer functions, like GetOverlappedResultEx, is the result of better support for the newer Windows platforms and squashing the warnings generated by the Visual Studio toolchain. See, for example, the USE_WINDOWS8_API macro in winpipes.cpp. No good deed goes unpunished, as they say...

We don't set WINVER or _WIN32_WINNT. We don't have a <winver.h> or <targetver.h> in our pre-converted Visual Studio 2010 solution and project files. We expect users to set WINVER or _WIN32_WINNT appropriately when they have the need; otherwise, they get the default build environment. The folks contributing to Cmake maintenance do this for their Windows UWP builds.

Its not clear to me why Microsoft is setting Windows 8 for a Windows 7 build and execution environment. We are trying to gather details on Stack Overflow at Visual Studio setting WINVER/_WIN32_WINNT to Windows 8 on Windows 7?

Its not clear to me if we can easily clear this error. We _can't_ do something like below because Microsoft's toolchain is selecting the incorrect value, so our code will never execute:

#ifdef CRYPTOPP_WIN32_AVAILABLE
# ifndef _WIN32_WINNT
#  define _WIN32_WINNT 0x0500
# endif
#endif

And if we select 0x0500, then we are wrong for Windows 8, Phone 8, Store 8, etc. If we select 0x0602 for Windows 8, Phone 8, Store 8, then we break all the down level clients like Windows Vista and Windows 7. We will also be wrong for Windows 10, Phone 10, Store 10.

We are trying to locate a value for _WIN32_WINNT that symbolizes "this platform". On Windows XP, "this platform" will take on the value of Windows XP and 0x0501. On Windows 7, "this platform" will take on the value of Windows 7 and 0x601. On Windows 8, "this platform" will take on the value of Windows 8 and 0x602, etc.

zabulus commented 8 years ago

_WIN32_WINNT should be set apropriate to WINAPI_FAMILY define value: For Desktop 0x0500 For App 0x0602

EDIT: interesting how the error occured. GetOverlappedResultEx is guarded by USE_WINDOWS8_API define, which guarded by WINVER >= 0x0602 or _WIN32_WINNT >= 0x0602 defines. Are you sure that win7 build has right defines?

EDIT2: I think this messing of defines is occurred because aavailable APIs for Phone are determined by _WIN32_WINNT defines. I think UWP build should be fine-grained basing on WINAPI_FAMILY.

EDIT3: Do you have plans to organize CI, so we could not break things when adding there and here?

noloader commented 8 years ago

Do you have plans to organize CI, so we could not break things when adding there and here?

I'd like to. We ruled Travis CI out due to some architectural defects.

I tried to test BuildBot 0.8.12 on two separate machines. First is our Crypto++ VM, and second is a Ubuntu 14 LTS server. The idea was to run a central master, like on the Crypto++ VM, and then slaves can simply join or be provisioned. I have about 40 machines from my environment that would be reporting to the master. I imagine others would want to toss their machines and configuration into the mix. For example, I imagine you would want to ensure Windows 10 with Cmake works as expected, so you would join or ask to be provisioned.

Unfortunately, BuildBot did not work under either tests.

zabulus commented 8 years ago

I've done following:

  1. Checked out latest 94f26519d215d87ecffcdee6f30426f3b97dbee4
  2. Unpacked vs2010.zip into .\
  3. Opened resulting solution in VS 2015. Declined compiler upgrade dialog box.
  4. Compile cryptest in x64\Release configuration.
  5. Run dumpbin for executable produced. See result: https://gist.github.com/zabulus/f51620b061d8bb41c698756fdf15075e GetOverlappedResult is used instead GetOverlappedResultEx

Note: I have WinSDK 7.0 installed, and it is enabled as default SDK for build if not specified other explicitly. So I have next defines used implicitly:

#if !defined(_WIN32_WINNT) && !defined(_CHICAGO_)
#define  _WIN32_WINNT   0x0601
#endif

0x0601 corresponds _WIN32_WINNT_WIN7. IMHO it is the root cause.

noloader commented 8 years ago

Microsoft states its a Windows 7/VS 2012 is a supported configuration, but it uses Windows 8 as a default setting which leads to this issue. The fix for this issue is to patch Microsoft's Windows 8.0 SDK header files on Windows 7.

_ORIGINAL_ (uses 0x0602):

#if !defined(_WIN32_WINNT) && !defined(_CHICAGO_)
#define  _WIN32_WINNT   0x0602
#endif

_PATCHED_ (uses 0x0601):

#if !defined(_WIN32_WINNT) && !defined(_CHICAGO_)
#define  _WIN32_WINNT   0x0601
#endif

Modifying a header on occasion is necessary. For example, including Microsoft C++ header files and /W4 often requires patching the headers to squash warnings. The other alternative is to lower warnings in the project, which is not acceptable.

denisbider commented 8 years ago

I have never, ever, found it necessary to modify a header file included with a Windows SDK or Visual Studio, and would not recommend this to anyone.

We compile with -WX -Wall, and do not modify any system or compiler header files. We instead have a standard platform include file which disables selected warnings before including system, compiler and library header files, and subsequently re-enables the warnings we want.

Requesting users to modify system header files seems like a dubious option. If that's what the library requires, I will sooner modify the library than the system header files.

noloader commented 8 years ago

@denisbider, I'm not sure what else we can do. Microsoft sets _WIN32_WINNT to the wrong value on Windows 7. All the ways to set _WIN32_WINNT at the library level sacrifice one use case for another. For example, if we set it to _WIN32_WINNT_WIN7, then we sacrifice up-level clients like Windows 8 and Windows 10. If we set it to _WIN32_WINNT_WIN10, then we sacrifice down-level clients like Windows Vista and Windows 7. We lose in every case we try and set it.


Another, not so readily apparent problem is code like this in fipstest.cpp:

#ifndef _WIN32_WINNT
# define WIN32_WINNT 0x0400
#endif

That code never executes under later SDKs because Microsoft sets it in winver.h or winsdk.h (or whatever the header file is). In this case, it does not matter what we set it to. We could even set it to the following and it would not affect a compile for platforms like Windows 7, Windows 8 and Windows 10:

#ifndef _WIN32_WINNT
# define WIN32_WINNT "Crypto++ rocks!"
#endif

Another option is to remove calls like GetOverlappedResultEx (and the proper includes). This use case sacrifices up-level clients. Eventually it will break the library when GetOverlappedResult goes away. To me, this is worse than modifying a Windows header to accommodate the mis-configured SDK configuration.


You proposed this:

We instead have a standard platform include file which disables selected warnings before including system...

That's possible for Windows headers, but what does Crypto++ do about warnings like using deprecated GetOverlappedResult? We should not turn them off. And using the recommended function takes us back to the problem of selecting the right platform by default.

Also, fixing the Windows header rather than supplying a custom header in a project means the remediations are applied to all past, current and future projects; rather than just the one project at hand. This is the overwhelming reason I started fixing the Windows headers - I wanted to do it once in one place so that it "just worked" everywhere.

For what its worth, I've been modifying Windows headers for about 15 years now when faced with a problem like this (back against the wall because its a system header, with a "fix it once and for all" requirement). Its never caused me any problems.


I do agree with you its not ideal. I think the ideal solution is for the Microsoft installer to set _WIN32_WINNT to the proper value. The next best solution is for Microsoft to provide a define that signifies "This Platform", similar to GCC's -march=native.

In the age of abondonware as a business use case to force an upgrade to drive sales or downloads, I don't expect Microsoft to fix the incorrect value anytime soon. I also wonder how many users are running VS2012 on Windows 7 (is this a corner case?)

I look forward to your suggestions and proposals. If this is a philosophical divide, then my apologies.

MarcelRaad commented 8 years ago

The Windows SDK always sets the target Windows version to the latest known version if not already set by the user. I don't feel that's wrong as you usually don't compile anything exclusively for your own development machine on Windows and only you can know what Windows versions your application will run on.

I have never compiled anything on Windows without setting the target Windows version manually in the project file (or property sheet), including in crypto++. Users who don't change the default value get the latest version in their application anyway, so I don't think a library should change it for itself.

noloader commented 8 years ago

@MarcelRaad: "I don't feel that's wrong as you usually don't compile anything exclusively for your own development machine on Windows..."

Tha'ts kind of interesting (to me). I would expect the dominate use case is build and then run on your workstation. Every developer I have ever known does it.

Or maybe the "not exclusively" part captures the minimal feature set to address the widest audience, like linux distro building a package using a minimal features: MMX and SSE2, etc.

The one-off case is the release build. It happens infrequently, and its performed by another group outside the developer group to enforce separation of concerns. Its also code signed in a separate, isolated environment; and its tested by another group, etc.

By the way, Crypto++ violates separation of concerns because it lacks a good separation between development, testing and release. We need more volunteers with more spare cycles.

noloader commented 8 years ago

@denisbider , @zabulus , @MarcelRaad,

So it sounds like the three options are:

Are there any other options? Maybe "Don't install Visual Studio on Windows 7"? Maybe something else?

For completeness, I avoided the LoadLibrary, LoadLibraryEx and LoadPackagedLibrary alternatives. Loading libraries on the fly at the application level adds additional points of failure and increases attack surface.

I'm OK with suppressing warnings as a short term fix, but we need a long term solution. We can't hide the fact that GetOverlappedResult is going away (or hide it forever). Eventually, it will go away (and we will have to act).

Which would you like to recommend?

denisbider commented 8 years ago

I would expect the dominate use case is build and then run on your workstation. Every developer I have ever known does it.

On Windows, almost all software comes pre-compiled. You only ever build software for release, i.e. for distribution, or for the ultimate purpose of distribution, unless you are specifically writing a custom program only for yourself.

Of course you run the software on your workstation during development, but end users don't compile for themselves.

If you are building for testing, you want to build in an environment as standard as possible, so that the release build will be the same. This means no modification of system or compiler header files. Horrible bugs can creep up if build environments have inconsistencies in this regard.

For example, if we set it to _WIN32_WINNT_WIN7, then we sacrifice up-level clients like Windows 8 and Windows 10.

But you don't! What we do is, set these macros to the lowest platform that we target. The compiled program needs to run on these platforms, so it cannot have compile-time dependencies on higher OS versions.

Often, we still want to use functionality that is available only in newer Windows versions. In this case, we define our own typedef for the function pointer type, and access the function conditionally at run-time using LoadLibrary(Ex), GetProcAddress, and then calling the function pointer.

I think the theoretical ideal thing for Crypto++ to do, assuming no effort spared, would be to dynamically invoke GetOverlappedResultEx when the code runs on Windows 8+, and use GetOverlappedResult on older Windows versions.

In practice, Crypto++ probably does not need the "Ex" version of that function, and should just use the older version, which works fine.

With regard to the SDK version selection macros, I think Crypto++ should simply protest if the user does not set them explicitly. Bad outcomes can result from linking object files built with different SDK version settings.

denisbider commented 8 years ago

On a related note, which almost certainly deserves a conversation as a separate issue: Crypto++ contains way too much infrastructural non-crypto code which simply does not belong in a crypto library. The reasons for this are historical, and have to do with that Wei Dai was the original developer of the first-generation SSH implementation that Bitvise used, and the original developer of the groundwork for the product now known as Bitvise SSH Server.

Wei worked on this software alone at that time, and made architectural decisions which in hindsight are a poor compartmentalization of responsibilities. This includes things like socket functionality being part of the crypto library simply because there was need for a socket implementation that follows the same source/filter/sink design as the rest of the library.

A proper design would be to realize the need to separate the infrastructure into a separate library, on which Crypto++ could depend. Or better yet, the infrastructure should have been completely separate from the crypto, so that cryptographic algorithms do not have the weird requirement of having to implement an asynchronous filter interface just to slightly more neatly fit into an asynchronous SSH library.

This was a poor outcome not only for Crypto++, which now has interfaces much more clunky than it should given its main purpose and its most common usage scenarios; it was also a poor result for earlier versions of our SSH software, which ended up with their core infrastructure married to Crypto++ like this. To remedy this problem, we wrote a whole new SSH library, FlowSsh, which no longer uses any of the Crypto++ infrastructure, but uses Flow - a newer and completely separate library actually intended for the infrastructural role. In the way we now use Crypto++, we now use it at an arm's distance, and have no need for its asynchronous IO support.

I wonder how many users of this infrastructural code in Crypto++ there remain. If you ask me, I would cut most non-crypto code out of the library, preserving only what is needed for testing; and deprecate or even remove non-blocking IO support; retain compatibility with simple usage cases that are most common; and release that smaller, nimbler library as Crypto++ version 6. The result would be both easier to use, and easier to maintain by a group of volunteers, now that Wei has retired.

I would cut the library aggressively, leaving a simpler interface and only the crypto features.

noloader commented 8 years ago

@denisbider , @zabulus , @MarcelRaad,

So what is the recommendation or actionable item?

Also, keep in mind the APIs usually progress, but they sometimes regress. For example, Windows Phone 8 and Windows Store 8 are missing features both its predecessor had and successors have. If you happen to fall into the "Windows 8 as a minimum platform" trap, then you suffer the API gaps.

denisbider commented 8 years ago

Is there a different defined macro that could be used to decide if the compilation target is Windows Phone or Windows Store? Perhaps a different macro that specifically indicates these platforms could be used to select APIs at compilation time that these platforms need to use.

Alternately, it would be reasonable to require the user to define the SDK macros in project configuration, especially if it only affects a particular old Visual Studio version.

zabulus commented 8 years ago

It is not a problem in cryptopp. You build the library using SDK 8 (or higher), which targets win8 by default, but then use the library in win 7. Your options:

  1. Explicitly set _WIN32_WINNT to win 7 during the library build.
  2. Use Windows SDK 7.1A that targets win 7 by default.
noloader commented 8 years ago

Your options:

  1. Explicitly set _WIN32_WINNT to win 7 during the library build.
  2. Use Windows SDK 7.1A that targets win 7 by default.

Or, (3) open the SDK header file and change the default value to something reasonable for Windows 7. I selected (3) and used changed the default to _WIN32_WINNT_WIN7. I have not experienced any trouble since the change. Even better, it did not change one line of Crypto++ code.

You guys are free to slug it out how you like :)

noloader commented 8 years ago

@denisbider, @DevJPM, @MarcelRaad,

I would cut the library aggressively, leaving a simpler interface and only the crypto features.

We are now tracking this at Issue 208: Investigate moving OS specific Socket and Thread to the Patch Page.

andim2 commented 8 years ago

Since this seems to be a very usefully specific discussion here, I am attaching this more general note here:

I believe that way too many development projects out there get the platform configuration completely wrong on a very fundamental level (and yes, folks, that includes MFC projects' default setup!).

I believe that the platform value settings are supposed to be a REQUEST made on a fully build-wide i.e. solution-wide scope (read: more or less governed/adjusted by a developer itself!), to ensure that all projects which are being aggregated to be built by this solution get to know (are being made aware of) which lowest-common-denominator platform baseline value is to be STRICTLY OBEYED by all source implementation code which is taking part in the build, fully consistently (this is the only way to avoid having excessively modern/old system library symbol imports grabbed by certain non-conformant source areas of the build, i.e. those imports which are not provided by operating system platform eras which are more critical vs. the "usual" platform baseline that a build targets/aims for).

This reasoning has the following consequences:

#ifndef WINVER
#define WINVER 0x0501 /* 0x0501 == XP */
#endif

is COMPLETELY ILLEGAL since it locally fumble-bends inclusions of Windows SDK definition code to be using (providing) specifically Windows-platform-version-dependent features inconsistently differing from the settings used by other projects or single source files --> that's the road to h*ll for the non-negotiable requirement of achieving a consistently baseline-platform-supporting set of binary files (libraries plus binaries) which are the result of a common, supposedly-consistent, entire whole-solution build (one could say that instead one should be doing code such as

#ifdef _WIN32
#ifndef WINVER
#pragma message("You failed to consistently indicate a minimum platform baseline version config to this place here, aborting!! " __FILE__)
ERROR_missing_platform_config;
#endif
#endif

And once having achieved setting the platform baseline version values consistently, this means that any (usually more modern) platform features which contradict this platform baseline request (yet which would be very nice to be used on the platforms that support them) need to be handled on a dynamic, case-by-case basis (i.e. GetProcAddress() etc.), rather than standard (baseline-platform-SDK-based) linking - that way the fixed, final result binary set will be able to support a lowest-common-denominator baseline platform version yet still be able to make use of features provided by more modern platforms.

HTH (and I'd love to hear your thoughts on this),

Andreas Mohr

denisbider commented 8 years ago

To the extent of my understanding right now, I believe I fully agree with Andreas's above discussion. I especially agree with this:

individual source code areas are to never, ever custom-bend values of any defines which indicate the target baseline platform value to be implementing for

And with this:

(one could say that instead one should be doing code such as

ifdef _WIN32

#ifndef WINVER
#pragma message("You failed to consistently indicate a minimum platform baseline version config to this place here, aborting!! " __FILE__)
ERROR_missing_platform_config;
#endif
#endif
noloader commented 8 years ago

Hi Everyone,

I just came across this on MSDN Community Support: How to create a project that has both Metro style and desktop build configurations. According to a Microsoft engineer, this configuration is not supported even though the Microsoft marketing literature state it is. The minimum platform is Windows 8. Sigh...

I'm closing this issue. We are battling windmills. Its wasted enough of our time.

@denisbider, @andim2,

If you want to open a new issue with an actionable item, them please feel free. I did not close it to shut down your opinion or disregard your counsel. The issue was closed because its an unsupported configuration.

andim2 commented 8 years ago

@noloader: I am perfectly fine with this, and thanks for your thorough consideration (would hope though that my clarification text remains publicly discoverable for some more time, post closing).

My clarification text was about sufficiently correctly (i.e., solution-globally and thus consistently) requesting (instructing) platform version configuration from an SDK - but that of course usually in a situation where the requested configuration in fact is properly (and thus usually: officially) supported by the SDK content. Yet here it seems we have a configuration corner-case which is not-/semi-supported by relevant SDK product offerings. In such cases one might even decide having to resort to the very ugly sinful activity of actively hacking (bending/tweaking/causing deviations) inner content of official SDKs - which quickly becomes both fully unsupported (off the mainstream way) and a maintenance effort/nightmare issue.

noloader commented 8 years ago

@denisbider, @andim2,

I've opened a question at Stack Overflow to see if we could close some of the gaps: Implicit library linking and GetModuleHandle/GetProcAdress.


Regarding "would hope though that my clarification text remains publicly discoverable for some more time, post closing"

Yes, its always available. The bug report list you see is filtered. If you remove the is:open filter, then you see them all.


Regarding this observation:

#ifndef WINVER
#define WINVER 0x0501 /* 0x0501 == XP */
#endif

We used to do it privately in some CPP files, but we no longer do it at all. We did not do it in a H file such that it cross-pollinated.

We used to do it privately to ensure some WinCrypt symbols were available. But that has not been needed since Windows 9x/Windows 2000 transitions. I believe there was a bug report opened to ensure it was properly tracked during removal.

A quick audit on Linux:

$ grep '#ifndef WINVER' *
$
$ grep '#ifndef[[:space:]]WINVER' *
$
noloader commented 8 years ago

Now open: Remove dependencies on WINVER and _WIN32_WINNT