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

CryptAcquireContext should be found #10353

Closed
wants to merge 3 commits into from
Closed

CryptAcquireContext should be found #10353

wants to merge 3 commits into from

Conversation

ALittleDruid
Copy link

@ALittleDruid ALittleDruid commented Jan 28, 2023

After set(CMAKE_REQUIRED_INCLUDES "${CURL_SOURCE_DIR}/include")

image

image

"D:\vcpkg\installed\x64-windows\include" is correct

microsoft/vcpkg#27978 (comment)

@bagder bagder added the Windows Windows-specific label Jan 29, 2023
@jay
Copy link
Member

jay commented Feb 12, 2023

Can any dev familiar with cmake explain how CMAKE_REQUIRED_INCLUDES is supposed to work? I notice in CMakeLists.txt it is sometimes appended to as a list and other times cleared, as it is before getting the type of curl_off_t, which seems to have caused this bug. The reporter's fix to move the CryptAcquireContext check before the curl_off_t check looks like it masks a larger problem.

@jzakrzewski
Copy link
Contributor

It's a list of include directories to use for all the checks that involve running a compiler. In my own projects I tend to re-set it after each check to make sure I'm not clobbering the next one, and to make sure the stuff I'm looking for is where I expect it, and not in some random place.

@ALittleDruid
Copy link
Author

ALittleDruid commented Feb 13, 2023

https://cmake.org/cmake/help/latest/module/CheckSymbolExists.html

check_symbol_exists(CryptAcquireContext "${CURL_INCLUDES}" USE_WINCRYPT)

"${CURL_INCLUDES}" is windows.h;ws2tcpip.h;winsock2.h;wincrypt.h;sys/stat.h;sys/types.h;sys/utime.h;fcntl.h;idn2.h;io.h;locale.h;setjmp.h;signal.h;stdbool.h;stdlib.h;string.h;time.h;stddef.h

File CheckSymbolExists.c:
/* */
#include <windows.h>
#include <ws2tcpip.h>
#include <winsock2.h>
#include <wincrypt.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/utime.h>
#include <fcntl.h>
#include <idn2.h>
#include <io.h>
#include <locale.h>
#include <setjmp.h>
#include <signal.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <stddef.h>

int main(int argc, char** argv)
{
(void)argv;
#ifndef CryptAcquireContext
return ((int*)(&CryptAcquireContext))[argc];
#else
(void)argc;
return 0;
#endif
}

Another way to modify it: change "${CURL_INCLUDES}" to "windows.h"

check_symbol_exists(CryptAcquireContext "windows.h" USE_WINCRYPT) or
check_symbol_exists(CryptAcquireContext "windows.h;wincrypt.h" USE_WINCRYPT)

@jzakrzewski
Copy link
Contributor

CryptAcquireContext is documented to live in wincrypt.h, so I suppose this is where we should be looking for it, and not "everywhere".

kr-zx and others added 2 commits February 14, 2023 23:47
@jay
Copy link
Member

jay commented Feb 15, 2023

I looked at your latest changes and I don't understand how this works for you now if CMAKE_REQUIRED_INCLUDES does not need to be reset. Don't you have the same problem, where the previously set CMAKE_REQUIRED_INCLUDES uses the wrong include directory?

@ALittleDruid
Copy link
Author

ALittleDruid commented Feb 15, 2023

I looked at your latest changes and I don't understand how this works for you now if CMAKE_REQUIRED_INCLUDES does not need to be reset. Don't you have the same problem, where the previously set CMAKE_REQUIRED_INCLUDES uses the wrong include directory?

check_symbol_exists(<symbol> <files> <variable>)
check_symbol_exists(CryptAcquireContext "${CURL_INCLUDES}" USE_WINCRYPT)
files is windows.h;ws2tcpip.h;winsock2.h;wincrypt.h;sys/stat.h;sys/types.h;sys/utime.h;fcntl.h;idn2.h;io.h;locale.h;setjmp.h;signal.h;stdbool.h;stdlib.h;string.h;time.h;stddef.h

CMAKE_REQUIRED_INCLUDES is ${CURL_SOURCE_DIR}/include

cmake try compile CheckSymbolExists.c

#include <windows.h>
#include <ws2tcpip.h>
#include <winsock2.h>
#include <wincrypt.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/utime.h>
#include <fcntl.h>
#include <idn2.h>
#include <io.h>
#include <locale.h>
#include <setjmp.h>
#include <signal.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <stddef.h>

int main(int argc, char** argv)
{
  (void)argv;
#ifndef CryptAcquireContext
  return ((int*)(&CryptAcquireContext))[argc];
#else
  (void)argc;
  return 0;
#endif
}

CheckSymbolExists.c include unnecessary header files and idn2.h not found in ${CURL_SOURCE_DIR}/include

The latest changes only include windows.h and wincrypt.h

@jay
Copy link
Member

jay commented Feb 16, 2023

CheckSymbolExists.c include unnecessary header files and idn2.h not found in ${CURL_SOURCE_DIR}/include

So in that case is the reason the test fails that idn2.h isn't found?

@ALittleDruid
Copy link
Author

CheckSymbolExists.c include unnecessary header files and idn2.h not found in ${CURL_SOURCE_DIR}/include

So in that case is the reason the test fails that idn2.h isn't found?

Yes

Befor set(CMAKE_REQUIRED_INCLUDES "${CURL_SOURCE_DIR}/include"), CMAKE_REQUIRED_INCLUDES is D:\vcpkg\installed\x64-windows\include and idn2.h can be found in that path

That's enough to check CryptAcquireContext include windows.h and wincrypt.h , the default include path is able to find these header files

jay pushed a commit to jay/curl that referenced this pull request Feb 17, 2023
Check for CryptAcquireContext in windows.h and wincrypt.h.

Closes curl#10353
@jay jay closed this in f5a88f2 Feb 19, 2023
@jay
Copy link
Member

jay commented Feb 19, 2023

Thanks

@ALittleDruid ALittleDruid deleted the fix_check_symbol_exists_CryptAcquireContext branch February 19, 2023 05:30
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
Check for CryptAcquireContext in windows.h and wincrypt.h only, since
otherwise this check may fail due to third party headers not found.

Closes curl#10353
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants