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

GHA/windows: add gssapi to openssl job #15549

Closed
wants to merge 1 commit into from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Nov 11, 2024

Add gssapi to openssl job

Ref: #15545
Ref: #15564
Ref: #15585

@github-actions github-actions bot added Windows Windows-specific CI Continuous Integration labels Nov 11, 2024
@talregev
Copy link
Contributor Author

It fail, and I am not sure if it intended:

D:\a\curl\curl\lib\strtok.h(31,9): error C2220: the following warning is treated as an error [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')
  
D:\a\curl\curl\lib\strtok.h(31,9): warning C4005: 'strtok_r': macro redefinition [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')
      C:\vcpkg\installed\x64-windows\include\win-mac.h(215,9):
      see previous definition of 'strtok_r'
  
D:\a\curl\curl\lib\krb5.c(252,[48](https://github.com/talregev/curl/actions/runs/11779158992/job/32807035787?pr=27#step:9:49)): error C2065: 'gss_nt_service_name': undeclared identifier [D:\a\curl\curl\bld\lib\libcurl_object.vcxproj]
  (compiling source file 'CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c')

@vszakats
Copy link
Member

Can you test it with #15564?

@talregev
Copy link
Contributor Author

Can you test it with #15564?

I rebase over your branch.

@talregev
Copy link
Contributor Author

It solve the 'strtok_r': macro redefinition But there is more compilation problems.

@vszakats
Copy link
Member

vszakats commented Nov 13, 2024

There is something funny going on when trying and failing to detect GSS_C_NT_HOSTBASED_SERVICE.
Failing to detect it may cause the 'gss_OID' differs in levels of indirection from 'int' errors.

Can you also apply #15545? It may help avoiding some warnings, and perhaps this, and perhaps-perhaps the detection above:

LINK : fatal error LNK1104: cannot open file 'gssapi64.lib'

https://github.com/curl/curl/actions/runs/11819947642/job/32931130624?pr=15549#step:7:690

This still leaves a bunch of other issues though.

@talregev
Copy link
Contributor Author

I rebease over your other branch. It solve many problems.

@vszakats
Copy link
Member

vszakats commented Nov 13, 2024

It fixed the detection and some of the warnings with it.

Reduced to this now:

D:\a\curl\curl\lib\krb5.c(652,28): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
D:\a\curl\curl\lib\krb5.c(654,28): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
D:\a\curl\curl\lib\krb5.c(656,26): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
D:\a\curl\curl\lib\krb5.c(657,26): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
D:\a\curl\curl\lib\krb5.c(665,24): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
D:\a\curl\curl\lib\krb5.c(666,24): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data 

I don't understand which type is wrong. There is a sockindex (int) and
the socket itself (curl_socket_t). On Windows their types are not
interchangeable.

socket_write() is passed a socket, but expects a sockindex. Then
it passes down this sockindex to Curl_conn_send().

vszakats added a commit that referenced this pull request Nov 14, 2024
krb5 defines `strtok_r` for Windows unconditionally in its public
header:
https://github.com/krb5/krb5/blob/dc5554394e5a4363b3e109623edbeb9ad6c18a62/src/include/win-mac.h#L214-L215
resulting in this warning:
```
lib\strtok.h(31,9): warning C4005: 'strtok_r': macro redefinition
      C:\vcpkg\installed\x64-windows\include\win-mac.h(215,9):
      see previous definition of 'strtok_r'
```

The krb5 macro collides with curl's internal definition, in case
the `strtok_r` function is undetected and falling back to a local
replacement.

Reported-by: Tal Regev
Bug: #15549 (comment)
Closes #15564
@vszakats
Copy link
Member

@talregev Would you mind opening an issue with the int/socket confusion warnings? To give it visibility.

@talregev
Copy link
Contributor Author

@talregev Would you mind opening an issue with the int/socket confusion warnings? To give it visibility.

I open an issue:
#15582

@talregev
Copy link
Contributor Author

I disable warning as errors to see what happen next.

vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
@talregev talregev closed this Nov 15, 2024
@talregev talregev deleted the TalR/gssapi branch November 15, 2024 03:38
vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
vszakats pushed a commit to vszakats/curl that referenced this pull request Nov 15, 2024
Add gssapi to openssl job

waiting for:
- [x] curl#15564
- [x] curl#15545

Closes curl#15549
vszakats added a commit that referenced this pull request Nov 15, 2024
- fix socket/sockindex confusion on writes:

  The callstack used to end with `Curl_write_plain()` accepting a socket
  till 7.87.0. This call got swapped for `Curl_conn_send()`, expecting
  a sockindex. `socket_write()` was updated accordingly. Its callers
  missed it and continued operating on sockets: `do_sec_send()`,
  `sec_write()`, passing it down the stack and `Curl_conn_send()`
  resolving it as if it were a sockindex.
  It affected FTP Kerberos authentication.

  Discovered through MSVC warnings:
  ```
  curl\lib\krb5.c(652,28): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
  curl\lib\krb5.c(654,28): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
  curl\lib\krb5.c(656,26): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
  curl\lib\krb5.c(657,26): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
  curl\lib\krb5.c(665,24): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
  curl\lib\krb5.c(666,24): warning C4244: 'function': conversion from 'curl_socket_t' to 'int', possible loss of data
  ```
  Ref: https://github.com/curl/curl/actions/runs/11846599621/job/33014592805#step:9:32

  Follow-up to 5651a36 #10280
  Bug: #15549 (comment)
  Fixes #15582

- fix uninitialized buffer:
  ```
  curl\lib\krb5.c(288,1): warning C4701: potentially uninitialized local variable '_gssresp' used
  ```
  Ref: https://github.com/curl/curl/actions/runs/11848626645/job/33020501026?pr=15585#step:9:31

- silence unreachable code compiler warning:
  ```
  curl\lib\krb5.c(370,1): warning C4702: unreachable code
  ```
  Ref: https://github.com/curl/curl/actions/runs/11848626645/job/33020501026?pr=15585#step:9:30

Closes #15585
vszakats pushed a commit that referenced this pull request Nov 15, 2024
@vszakats
Copy link
Member

Thanks @talregev! I merged this separately and pushed in batch together with last round of fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authentication CI Continuous Integration Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

2 participants