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

Jump to bottom

Add Windows header for CRYPT_E_REVOKED macro #411

Closed
wants to merge 1 commit into from
Closed

Add Windows header for CRYPT_E_REVOKED macro #411

wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 4, 2015

Starting with curl 7.44.0 [1], compiling source file lib/strerror.c
with mingw and SSPI enabled, may fail with error:

strerror.c: In function 'Curl_sspi_strerror':
strerror.c:827:10: error: 'CRYPT_E_REVOKED' undeclared (first use in this function)
     case CRYPT_E_REVOKED:
          ^

This happens when using mingw distros that ship with Windows headers
via the w32api package. Such distro is the canonical one, which is
also offered by certain [2] CI platforms. w32api developers implemented
[3] this macro strictly per MSDN [4], solely adding it to wincrypt.h,
This made it incompatible with original Windows SDK headers which define
it in winerror.h. Other mingw distros (f.e. [5]) that use the WINE
headers, work fine.

One solution is to comply with MSDN by #include-ing <wincrypt.h>
via lib/curl_setup.h.

This patch also fixes a TAB indentation.

[1] de74e85
[2] http://www.appveyor.com/docs/installed-software#mingw-msys-cygwin
[3] https://sourceforge.net/u/cstrauss/w32api/ci/3dd1f4b7cb6ab8da80e4c2bbcdb629c8c43b0d6b/
[4] https://msdn.microsoft.com/en-us/library/windows/desktop/aa377167.aspx
[5] https://sourceforge.net/projects/mingw-w64

@vszakats
Add Windows header for CRYPT_E_REVOKED macro
13c9bfb 
Starting with curl 7.44.0 [1], compiling source file `lib/strerror.c`
with mingw and SSPI enabled, may fails with error:

```
strerror.c: In function 'Curl_sspi_strerror':
strerror.c:827:10: error: 'CRYPT_E_REVOKED' undeclared (first use in this function)
     case CRYPT_E_REVOKED:
          ^
```

This happens when using mingw distros that ship with Windows headers
via the `w32api` package. Such distro is the canonical one, which is
also used by certain [2] CI platforms. `w32api` developers implemented
[3] this macro strictly per MSDN [4], solely adding it to `wincrypt.h`,
This made it incompatible with official Windows headers which define
it in `winerror.h`. Other mingw distros (f.e. [5]) that use the WINE
headers, work fine.

One solution is to comply with MSDN by #include-ing `<wincrypt.h>`
via `lib/curl_setup.h`.

This patch also fixes a TAB indentation.

[1] de74e85
[2] http://www.appveyor.com/docs/installed-software#mingw-msys-cygwin
[3] https://sourceforge.net/u/cstrauss/w32api/ci/3dd1f4b7cb6ab8da80e4c2bbcdb629c8c43b0d6b/
[4] https://msdn.microsoft.com/en-us/library/windows/desktop/aa377167.aspx
[5] https://sourceforge.net/projects/mingw-w64
@vszakats
Copy link
Member Author

vszakats commented Sep 4, 2015

Not sure why the CI failure, it doesn't seem to be related to this patch, given that it compiles strerror.c fine.

Anyhow an extra guard of defined(WIN32) may be added, though I'd assume USE_WINDOWS_SSPI is never defined on non-Windows platforms.

Or, for extra safety a HAVE_WINCRYPT_H could be defined and used, but (maybe besides certain versions of experimental Tiny C compiler and possibly other very old ones), this header should be present. My experience with autoconf tools is non-existent unfortunately.

@bagder
Copy link
Member

bagder commented Sep 6, 2015

Any objections or comments from @jay or @mback2k ?

@vszakats
Copy link
Member Author

vszakats commented Sep 8, 2015 •
edited

Here's the error in action on AppVeyor CI:
https://ci.appveyor.com/project/vszakats/harbour-deps/build/1.0.84#L2061

Here's a successful build run that uses the patch:
https://ci.appveyor.com/project/vszakats/harbour-deps/build/1.0.74#L2147
with resulting binaries here:
https://bintray.com/vszakats/generic/curl/7.44.0/view

@jay
Copy link
Member

jay commented Sep 9, 2015

It may be better to just define CRYPT_E_REVOKED ourselves. It is the only CRYPT_ code that schannel will return afaict. @vszakats can you try
https://github.com/bagder/curl/compare/master...jay:fix_missing_crypt_e_revoked?expand=1

@vszakats
Copy link
Member Author

vszakats commented Sep 9, 2015 •
edited

If such solution is okay in curl and it's used already, it's definitely much simpler, in fact this was how I fixed it first, but went the "official" way instead.

Build is okay with the new patch:
https://ci.appveyor.com/project/vszakats/harbour-deps/build/1.0.86#L2069

jay added a commit that referenced this pull request Sep 10, 2015
@jay
curl_sspi: fix possibly undefined CRYPT_E_REVOKED
df6a4d3 
Bug: #411
Reported-by: Viktor Szakats
@jay
Copy link
Member

jay commented Sep 10, 2015

Thanks, landed in df6a4d3.

@jay jay closed this Sep 10, 2015
@vszakats vszakats deleted the patch-3 branch September 10, 2015 07:41
@vszakats
Copy link
Member Author

Thank you @jay!

jgsogo pushed a commit to jgsogo/curl that referenced this pull request Oct 19, 2015
@jay @jgsogo
curl_sspi: fix possibly undefined CRYPT_E_REVOKED
24b4f22 
Bug: curl#411
Reported-by: Viktor Szakats
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vszakats @bagder @jay