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
base: master
from

Conversation

Projects
None yet
6 participants
@vszakats
Member

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

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jan 18, 2017

@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.

mention-bot commented Jan 18, 2017

@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 from *.rc: embed manifest for correct OS version detection to curl.rc: embed manifest for correct OS version detection Jan 18, 2017

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jan 19, 2017

Member

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?

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Jan 19, 2017

Member

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/harbour-core@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)

Member

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/harbour-core@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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Jan 19, 2017

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Jan 24, 2017

Member

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

Member

vszakats commented Jan 24, 2017

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

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jan 24, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Jan 24, 2017

Member

@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 ]

Member

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Mar 14, 2017

Member

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.

Member

vszakats commented Mar 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats May 18, 2018

Member

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.

Member

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 label May 25, 2018

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats May 25, 2018

Member

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.)

Member

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats May 25, 2018

Member

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
Member

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats May 27, 2018

Member

@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

Member

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats May 27, 2018

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 27, 2018

Member

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

They were not, but are now. Thanks!

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats May 29, 2018

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@fcharlie

fcharlie commented May 29, 2018

see: #2591 (comment)

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats
Member

vszakats commented May 29, 2018

Thanks @fcharlie!

@vszakats vszakats changed the title from curl.rc: embed manifest for correct OS version detection to curl.rc: embed manifest for correct Windows version detection May 29, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 30, 2018

Member

@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.

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.

curl.rc: embed manifest for correct Windows version detection
* 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 vszakats:winmft branch May 30, 2018

@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats May 30, 2018

Member

@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.)

Member

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 30, 2018

Member

Thanks a lot @vszakats !

Member

bagder commented May 30, 2018

Thanks a lot @vszakats !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment