Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

curl.rc: embed manifest for correct Windows version detection #1221

Closed
wants to merge 1 commit into from
Closed

curl.rc: embed manifest for correct Windows version detection #1221

wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jan 18, 2017

Include a Windows Manifest in curl.exe.

The method used here doesn't need additional toolchain-dependent trickery with separate manifest file or with linking a manifest file, but it's instead supplying the manifest content as part of the existing curl.rc file. The end result will be the same only slightly more compact, due to omitted newlines and comments, plus it doesn't require rolling yet another instance of the curl version number.

Reasoning

Including the supportedOS Id manifest bits is required for VerifyVersionInfo() and GetVersionEx() to return correct OS version above Windows 8. In source code src/vtls/schannel.c there is logic that depends on detecting Windows 8.1 to enable ALPN and this won't work as expected without a manifest.

Potential issues

  • At quick glance these modifications don't interfere with other builds systems (configure, winbuild and Visual Studio ones), though possibly something to double-check by maintainers of those build systems.
  • There is one known side-effect when building using MSYS2/Cygwin toolchains, where these toolchains recently started shipping with default manifests that in turn may trigger a binutils bug with builds supplying their own manifests, that in turn may confuse certain tools that manipulate PE files directly. It doesn't seem like something critical though. [this has been fixed since then]
  • The stringifying macro _STR() will trigger a bug in very old Borland/Embarcadero C compilers (e.g. 5.5, others not tested, but possibly affected) where the resource compiler may insert invalid bytes (e.g. \x01) at the end of the resulting strings. Consequently, newer Windows versions (e.g. 10) may reject running executables having such values in their manifest version IDs. If this is a concern, I suggest doing the stringification inside curl/curlver.h to avoid the problem. This would mean to define 3 new global version macros over the existing ones. (Manifest Version ID must follow the format of <num>.<num>.<num>.<num>, so existing LIBCURL_VERSION value isn't usable because it may contain "-DEV".)
  • Watcom C resource compiler has a bug in its -zm option code-path in version 1.9 (though that option might not be necessary after all), and certain, earlier revisions of the 2.0 beta had another related issue.

@mention-bot
Copy link

@vszakats, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @yangtse and @gknauf to be potential reviewers.

@vszakats vszakats changed the title *.rc: embed manifest for correct OS version detection curl.rc: embed manifest for correct OS version detection Jan 18, 2017
@jay
Copy link
Member

jay commented Jan 19, 2017

Borland C from how many years ago and do you have a link to the bug. I'm skeptical it would make a difference in behavior where the stringification macros reside. How is defining _STR in curlver different? Are you sure it didn't just pick up a trailing carriage return? My first thought for old versions of Borland C is just skip the whole thing

#ifndef OLD_BORLAND_C
manifest
#endif

Also does this have the potential to conflict with the EMBED_MANIFEST directive in winbuild?

@jay jay added the build label Jan 19, 2017
@vszakats
Copy link
Member Author

vszakats commented Jan 19, 2017

It's a Borland C from ~1999. General internet search didn't reveal any results, but it's not unusual to find unreferenced weird errors in this software. Here's a reference to my own minimal example: vszakats/hb@e3e9b26. The PP seems to work okay with similar minimal examples and also in large projects using stringification extensively. It's probably a personal preference, but I'd prefer not to create and maintain extra code specifically for this old compiler.

As for EMBED_MANIFEST, it does indeed collide with this patch. A solution for both is to guard the manifest section off using a macro. This macro could then be defined automatically if EMBED_MANIFEST is used, and manually for Borland C as needed.

I've updated the PR with such logic. (untested on WinBuild though)

@vszakats
Copy link
Member Author

vszakats commented Jan 19, 2017

Looks like Makefile.b32 for Borland C doesn't make use of curl.rc at all, so it will be alright as it is.

@vszakats
Copy link
Member Author

Is there anyone with any objections or concerns regarding this patch?

@jay
Copy link
Member

jay commented Jan 24, 2017

It should be tested for projects/ to make sure it doesn't interfere with anything and winbuild/ to make sure that switch to disable it works when an alternative manifest is provided. I haven't had a chance to do that, so if you can do that and it comes out fine then 👍 otherwise you could wait until I have time to do it

@vszakats
Copy link
Member Author

vszakats commented Jan 24, 2017

@jay I'm barely running Windows these days, and only mingw as a C compiler on it, so I'm going to rely on your tests regarding MSVC.

[ I've made successful and working builds using (the initial iteration of) this patch using MSYS2/mingw: https://ci.appveyor.com/project/vszakats/harbour-deps/build/1.0.973 ]

@vszakats
Copy link
Member Author

This became a non-issue when using MSYS2/MINGW after the required binutils fixes landed. The issue this fixes is probably not a very important one, and MSVC users may prefer a "real" .mft file anyway, not a tricky solution like this one. Closing.

@vszakats
Copy link
Member Author

vszakats commented May 18, 2018

To add one thought to this: This only became a non-issue when using MSYS2/MinGW (or Cygwin), because these build environments (in their more recent versions) automatically add a default manifest.

Other build environments (likely meaning the vast majority of build environments) don't feature the above hack, so the manifest will still be missing and making this issue still relevant.

@vszakats vszakats reopened this May 25, 2018
@vszakats vszakats added the Windows Windows-specific label May 25, 2018
@vszakats
Copy link
Member Author

vszakats commented May 25, 2018

Reworked this to be opt-in and enabled it in src/Makefile.m32, where it works fine. Also enabled in MakefileBuild.vc if the user-supplied manifest is not requested via EMBED_MANIFEST.

It's possible to enable this in other build-systems, but first it has to be ensured that no other manifest is being linked/enabled throughout the build process. I can see that CMake/MSVC does create one (seemingly without the relevant supportedOS items), but can't readily see where this is controlled (not a CMake or MSBuild expert here.)

@vszakats
Copy link
Member Author

vszakats commented May 25, 2018

Test binaries:
https://ci.appveyor.com/project/vszakats/curl-for-win/build/1.0.1237/job/f3ctnjpfk99g98r0/artifacts

Above tests are SCHANNEL-enabled (they require set CURL_SSL_BACKEND=schannel to use it), but were built with MinGW-w64, and ALPN is only enabled in lib/vtls/schannel.c when MSVC 18.00 or higher is detected at build time. It means that this particular build doesn't fix #2591, even though the necessary manifest is correctly included.

HAS_ALPN detection could be improved to actually detect the feature being present in SDK headers rather than tying it to a specific MSVC version, but then the next problem is that e.g. MinGW-w64 headers don't have the necessary [0] ALPN declarations yet. Till these are fixed, ALPN with SCHANNEL requires MSVC 18.00 or newer.

It'd be interesting to see a test with this patch, using winbuild and a compatible MSVC version.

[0] e.g.

  • SecApplicationProtocolNegotiationExt_ALPN
  • SecApplicationProtocolNegotiationStatus_Success
  • SecPkgContext_ApplicationProtocol
  • SECPKG_ATTR_APPLICATION_PROTOCOL

@vszakats
Copy link
Member Author

vszakats commented May 27, 2018

@bagder Would you mind double-checking if these settings are enabled on AppVeyor CI for curl? It looks like each PR update is being held in the job queue, even those that are overridden by a newer push. This puts CI under a strain when pushing multiple updates/fixups to a PR in quick succession and creates delays for all queued jobs.

The settings:
screen shot 2018-05-27 at 12 12 56
[…]
screen shot 2018-05-27 at 12 13 03

@vszakats
Copy link
Member Author

vszakats commented May 27, 2018

I've enabled the embedded manifest for all CMake builds on Windows. For MSVC this required to have the default manifest options removed. This is done by adding /MANIFEST:NO to the EXE linker flags. So far so good. But, for some reason this option also gets passed to lib.exe, resulting in a warning by LINK. Pretty weird, even if only a cosmetic blip in the log output.

@bagder
Copy link
Member

bagder commented May 27, 2018

Would you mind double-checking if these settings are enabled on AppVeyor CI for curl?

They were not, but are now. Thanks!

@vszakats
Copy link
Member Author

vszakats commented May 29, 2018

I'm confident this patch helps with #2591, but I have no way to test it against the necessary MSVC build.

Any comment on how to proceed, or anybody willing to give this patch a shot with an MSVS 2013 or upper + WinSSL + nghttp2 build using CMake or WinBuild and run it on Windows 10?

/cc @fcharlie

@fcharlie
Copy link

see: #2591 (comment)

@vszakats
Copy link
Member Author

Thanks @fcharlie!

@vszakats vszakats changed the title curl.rc: embed manifest for correct OS version detection curl.rc: embed manifest for correct Windows version detection May 29, 2018
@bagder
Copy link
Member

bagder commented May 30, 2018

@vszakats even without confirmation on this for winbuild, I think it is fine to proceed to merge this PR once the things you've tested works. I suspect we have enough users of the winbuild setup to get told if that is broken in some ways even if nobody steps up and confirm it working right now.

* enable it in `src/Makefile.m32`
* enable it in `winbuild/MakefileBuild.vc` if a custom manifest
  is _not_ enabled via the existing `EMBED_MANIFEST` option
* enable it for all Windows CMake builds (also disable the
  built-in minimal manifest, added by CMake by default.)
  One minor caveat.

For other build systems, add the `-DCURL_EMBED_MANIFEST` option to
the list of RC (Resource Compiler) flags to enable the manifest
included in `src/curl.rc`. This may require to disable whatever
automatic or other means in which way another manifest is added to
`curl.exe`.

Notice that Borland C doesn't support this method due to a
long-pending resource compiler bug. Watcom C may also not handle
it correctly when the `-zm` `wrc` option is used (this option
may be unnecessary though) and regardless of options in certain
earlier revisions of the 2.0 beta version.

Ref: #1221
Ref: #2591
@vszakats vszakats closed this in ebd2132 May 30, 2018
@vszakats vszakats deleted the winmft branch May 30, 2018 12:25
@vszakats
Copy link
Member Author

vszakats commented May 30, 2018

@bagder Thanks! I think we're all set at this point. I've now pushed to master. (one Linux job here still pending to go green due to an external issue.)

@bagder
Copy link
Member

bagder commented May 30, 2018

Thanks a lot @vszakats !

@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

6 participants