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

Add advapi32 as explicit link library #2363

Merged
merged 2 commits into from Mar 27, 2018

Conversation

Projects
None yet
4 participants
@janisozaur
Contributor

janisozaur commented Mar 6, 2018

advapi32 is required by some of libcurl's functions:

CryptReleaseContext:

lib/curl_ntlm_core.c=352=static bool encrypt_des(const unsigned char *in, unsigned char *out,
lib/curl_ntlm_core.c:384:    CryptReleaseContext(hprov, 0);
lib/curl_ntlm_core.c:395:  CryptReleaseContext(hprov, 0);
lib/curl_ntlm_core.c=555=CURLcode Curl_ntlm_core_mk_nt_hash(struct Curl_easy *data,
lib/curl_ntlm_core.c:616:      CryptReleaseContext(hprov, 0);
lib/md5.c=154=static void MD5_Final(unsigned char digest[16], MD5_CTX *ctx)
lib/md5.c:163:    CryptReleaseContext(ctx->hCryptProv, 0);
lib/vtls/schannel.c=1671=static CURLcode Curl_schannel_random(struct Curl_easy *data UNUSED_PARAM,
lib/vtls/schannel.c:1683:    CryptReleaseContext(hCryptProv, 0UL);
lib/vtls/schannel.c:1687:  CryptReleaseContext(hCryptProv, 0UL);
lib/vtls/schannel.c=1893=static void Curl_schannel_checksum(const unsigned char *input,
lib/vtls/schannel.c:1939:    CryptReleaseContext(hProv, 0);
src/tool_metalink.c=385=static void win32_crypto_final(struct win32_crypto_hash *ctx,
src/tool_metalink.c:396:    CryptReleaseContext(ctx->hCryptProv, 0);

CryptGetHashParam:

lib/curl_ntlm_core.c=555=CURLcode Curl_ntlm_core_mk_nt_hash(struct Curl_easy *data,
lib/curl_ntlm_core.c:613:        CryptGetHashParam(hhash, HP_HASHVAL, ntbuffer, &length, 0);
lib/md5.c=154=static void MD5_Final(unsigned char digest[16], MD5_CTX *ctx)
lib/md5.c:157:  CryptGetHashParam(ctx->hHash, HP_HASHVAL, NULL, &length, 0);
lib/md5.c:159:    CryptGetHashParam(ctx->hHash, HP_HASHVAL, digest, &length, 0);
lib/vtls/schannel.c=1893=static void Curl_schannel_checksum(const unsigned char *input,
lib/vtls/schannel.c:1923:    if(!CryptGetHashParam(hHash, HP_HASHSIZE, (BYTE *)&cbHashSize,
lib/vtls/schannel.c:1931:    if(CryptGetHashParam(hHash, HP_HASHVAL, checksum, &dwChecksumLen, 0))
src/tool_metalink.c=385=static void win32_crypto_final(struct win32_crypto_hash *ctx,
src/tool_metalink.c:390:  CryptGetHashParam(ctx->hHash, HP_HASHVAL, NULL, &length, 0);
src/tool_metalink.c:392:    CryptGetHashParam(ctx->hHash, HP_HASHVAL, digest, &length, 0);

As well as CryptCreateHash, CryptHashData, CryptDestroyHash, CryptGenRandom

Add advapi32 as explicit link library
advapi32 is required by some of libcurl's functions:

[`CryptReleaseContext`](https://msdn.microsoft.com/en-us/library/windows/desktop/aa380268(v=vs.85).aspx):
```
lib/curl_ntlm_core.c=352=static bool encrypt_des(const unsigned char *in, unsigned char *out,
lib/curl_ntlm_core.c:384:    CryptReleaseContext(hprov, 0);
lib/curl_ntlm_core.c:395:  CryptReleaseContext(hprov, 0);
lib/curl_ntlm_core.c=555=CURLcode Curl_ntlm_core_mk_nt_hash(struct Curl_easy *data,
lib/curl_ntlm_core.c:616:      CryptReleaseContext(hprov, 0);
lib/md5.c=154=static void MD5_Final(unsigned char digest[16], MD5_CTX *ctx)
lib/md5.c:163:    CryptReleaseContext(ctx->hCryptProv, 0);
lib/vtls/schannel.c=1671=static CURLcode Curl_schannel_random(struct Curl_easy *data UNUSED_PARAM,
lib/vtls/schannel.c:1683:    CryptReleaseContext(hCryptProv, 0UL);
lib/vtls/schannel.c:1687:  CryptReleaseContext(hCryptProv, 0UL);
lib/vtls/schannel.c=1893=static void Curl_schannel_checksum(const unsigned char *input,
lib/vtls/schannel.c:1939:    CryptReleaseContext(hProv, 0);
src/tool_metalink.c=385=static void win32_crypto_final(struct win32_crypto_hash *ctx,
src/tool_metalink.c:396:    CryptReleaseContext(ctx->hCryptProv, 0);
```

[`CryptGetHashParam`](https://msdn.microsoft.com/en-us/library/windows/desktop/aa379947(v=vs.85).aspx):
```
lib/curl_ntlm_core.c=555=CURLcode Curl_ntlm_core_mk_nt_hash(struct Curl_easy *data,
lib/curl_ntlm_core.c:613:        CryptGetHashParam(hhash, HP_HASHVAL, ntbuffer, &length, 0);
lib/md5.c=154=static void MD5_Final(unsigned char digest[16], MD5_CTX *ctx)
lib/md5.c:157:  CryptGetHashParam(ctx->hHash, HP_HASHVAL, NULL, &length, 0);
lib/md5.c:159:    CryptGetHashParam(ctx->hHash, HP_HASHVAL, digest, &length, 0);
lib/vtls/schannel.c=1893=static void Curl_schannel_checksum(const unsigned char *input,
lib/vtls/schannel.c:1923:    if(!CryptGetHashParam(hHash, HP_HASHSIZE, (BYTE *)&cbHashSize,
lib/vtls/schannel.c:1931:    if(CryptGetHashParam(hHash, HP_HASHVAL, checksum, &dwChecksumLen, 0))
src/tool_metalink.c=385=static void win32_crypto_final(struct win32_crypto_hash *ctx,
src/tool_metalink.c:390:  CryptGetHashParam(ctx->hHash, HP_HASHVAL, NULL, &length, 0);
src/tool_metalink.c:392:    CryptGetHashParam(ctx->hHash, HP_HASHVAL, digest, &length, 0);
```

As well as `CryptCreateHash`, `CryptHashData`, `CryptDestroyHash`, `CryptGenRandom`

@bagder bagder added the cmake label Mar 6, 2018

janisozaur added a commit to janisozaur/vcpkg that referenced this pull request Mar 6, 2018

[curl] Add missing advapi32 link
This adds missing library, advapi32, to linking. Fixes ARM builds.

The same patch is pending merge upstream:
curl/curl#2363

ras0219-msft added a commit to Microsoft/vcpkg that referenced this pull request Mar 6, 2018

[curl] Add missing advapi32 link (#2978)
This adds missing library, advapi32, to linking. Fixes ARM builds.

The same patch is pending merge upstream:
curl/curl#2363
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Mar 11, 2018

Member

I think this be if WIN32 since it seems to be what you are saying is it's needed for Windows generally not only for CMAKE_USE_WINSSL?

Member

jay commented Mar 11, 2018

I think this be if WIN32 since it seems to be what you are saying is it's needed for Windows generally not only for CMAKE_USE_WINSSL?

@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov Mar 19, 2018

Member

@jay CMAKE_USE_WINSSL will be present only for WIN32. For other platforms, it'll not be defined.

@bagder LGTM.

Member

snikulov commented Mar 19, 2018

@jay CMAKE_USE_WINSSL will be present only for WIN32. For other platforms, it'll not be defined.

@bagder LGTM.

@janisozaur

This comment has been minimized.

Show comment
Hide comment
@janisozaur

janisozaur Mar 19, 2018

Contributor

I think what @jay meant is to enable it in a if(WIN32) block rather than a more limited if(CMAKE_USE_WINSSL). It sounds fine, but OpenSSL already requires advapi32 itself:

https://github.com/openssl/openssl/blob/97a479c6f835ba7e1e5b03de4ff59bf829a6eadd/NOTES.WIN#L116-L118

https://github.com/openssl/openssl/blob/9e381e8a018592a2a42e83df402e1ef921469e9f/Configurations/10-main.conf#L1331

I can promote it to WIN32 if you want me to, but that's not a case that I was able to run into yet.

Contributor

janisozaur commented Mar 19, 2018

I think what @jay meant is to enable it in a if(WIN32) block rather than a more limited if(CMAKE_USE_WINSSL). It sounds fine, but OpenSSL already requires advapi32 itself:

https://github.com/openssl/openssl/blob/97a479c6f835ba7e1e5b03de4ff59bf829a6eadd/NOTES.WIN#L116-L118

https://github.com/openssl/openssl/blob/9e381e8a018592a2a42e83df402e1ef921469e9f/Configurations/10-main.conf#L1331

I can promote it to WIN32 if you want me to, but that's not a case that I was able to run into yet.

@janisozaur

This comment has been minimized.

Show comment
Hide comment
@janisozaur

janisozaur Mar 19, 2018

Contributor

Updated to WIN32 block.

Contributor

janisozaur commented Mar 19, 2018

Updated to WIN32 block.

@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov Mar 23, 2018

Member

@janisozaur just curious, why it's currently working on AppVeyor without those changes.
Any ideas how we can expose your issue?

Member

snikulov commented Mar 23, 2018

@janisozaur just curious, why it's currently working on AppVeyor without those changes.
Any ideas how we can expose your issue?

@janisozaur

This comment has been minimized.

Show comment
Hide comment
@janisozaur

janisozaur Mar 23, 2018

Contributor

Sure, target ARM.

Contributor

janisozaur commented Mar 23, 2018

Sure, target ARM.

@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov Mar 23, 2018

Member

@janisozaur just for my information :)
Why for ARM should it be explicitly specified?
Is advapi32 not used on x86 or it auto-linked?

Member

snikulov commented Mar 23, 2018

@janisozaur just for my information :)
Why for ARM should it be explicitly specified?
Is advapi32 not used on x86 or it auto-linked?

@janisozaur

This comment has been minimized.

Show comment
Hide comment
@janisozaur

janisozaur Mar 23, 2018

Contributor

Sorry, but I don't know. OpenSSL seems to already pull in advapi32, perhaps that's something that shadows the issue, at least in some cases.

I'd be surprised if it wasn't used on x86, as the linked MSDN pages suggest it is supposed to linked, but it's Windows, so you can never know.

Contributor

janisozaur commented Mar 23, 2018

Sorry, but I don't know. OpenSSL seems to already pull in advapi32, perhaps that's something that shadows the issue, at least in some cases.

I'd be surprised if it wasn't used on x86, as the linked MSDN pages suggest it is supposed to linked, but it's Windows, so you can never know.

@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov Mar 23, 2018

Member

@jay If you've no further questions, can I merge this?

Member

snikulov commented Mar 23, 2018

@jay If you've no further questions, can I merge this?

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Mar 23, 2018

Member

@jay If you've no further questions, can I merge this?

Yes but this will also need a squash and rebase and the commit message in our format, so for example (assuming I understand this issue right)

cmake: Add advapi32 as explicit link library for win32

ARM targets need advapi32 explicitly.

Closes #2363

A separate section with the metadata like Closes or Fixes signals to github to close the issue and we can parse that. (note in a previous bug I suggested something like 'This is a partial fix for ...' to prevent the automation from closing the bug since it was only a partial fix. normally if you close or fix a github issue please use the recognized keywords.)

Make sure the author is listed as the author before you commit, see also https://github.com/curl/curl/wiki/push-access-guidelines#how-to-work-with-a-pr-branch

The id is not needed in the subject line but not forbidden either afaik. Refer to existing commits already in the repo for more format examples

Member

jay commented Mar 23, 2018

@jay If you've no further questions, can I merge this?

Yes but this will also need a squash and rebase and the commit message in our format, so for example (assuming I understand this issue right)

cmake: Add advapi32 as explicit link library for win32

ARM targets need advapi32 explicitly.

Closes #2363

A separate section with the metadata like Closes or Fixes signals to github to close the issue and we can parse that. (note in a previous bug I suggested something like 'This is a partial fix for ...' to prevent the automation from closing the bug since it was only a partial fix. normally if you close or fix a github issue please use the recognized keywords.)

Make sure the author is listed as the author before you commit, see also https://github.com/curl/curl/wiki/push-access-guidelines#how-to-work-with-a-pr-branch

The id is not needed in the subject line but not forbidden either afaik. Refer to existing commits already in the repo for more format examples

@janisozaur

This comment has been minimized.

Show comment
Hide comment
@janisozaur

janisozaur Mar 23, 2018

Contributor

I'm happy for this to be squashed and the proposed commit message looks good.

Contributor

janisozaur commented Mar 23, 2018

I'm happy for this to be squashed and the proposed commit message looks good.

@snikulov snikulov merged commit bea18c7 into curl:master Mar 27, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 75.104%
Details
@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov Mar 27, 2018

Member

@janisozaur landed on bea18c7.
Thank you!

Member

snikulov commented Mar 27, 2018

@janisozaur landed on bea18c7.
Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Jun 25, 2018

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