tests/server: replace `errno` with `SOCKERRNO` in sockfilt, socksd, sws #16553

vszakats · 2025-03-04T14:37:29Z

To correctly read the winsock2 result code on Windows.

Follow-up to de2126b #5241
Ref: 5e855bb#r38507132
Ref: #14854

To behave properly on Windows. The same fix was previously applied to mqttd. That function and code repeats in socksd, where it may also be useful to fix the same issue. Follow-up to de2126b curl#5241 Ref: curl@5e855bb#r38507132

To follow the `errno` -> `SOCKERRNO` update. Missed from the previous commit. Follow-up to adcfd4f #16553

These tests seem to be running no less stable now than others. Stop ignoring their results to catch real issues. These are consistently failing and remain on the ignore list: in MSVC / vcpkg jobs: ``` FAIL-IGNORED 2302: 'WebSockets via callback (frame mode) + curl_ws_send()' WebSockets FAIL-IGNORED 2303: 'WebSockets but gets a 200 back' WebSockets FAIL-IGNORED 2307: 'WebSockets, overlong PING payload' WebSockets ``` https://github.com/curl/curl/actions/runs/13674664461/job/38233949942?pr=16570#step:14:4089 - Likely curl issues either in tests, server, or in WebSockets support. in tests running under MSYS, affecting native mingw Windows builds only: ``` FAIL-IGNORED 612: 'SFTP post-quote remove file' SFTP, post-quote [...] curl: (21) rm command failed: Operation failed ``` https://github.com/curl/curl/actions/runs/13674664461/job/38233952699?pr=16570#step:14:1378 in tests running under MSYS, affecting both MSYS and native mingw Windows builds: ```diff FAIL-IGNORED 613: 'SFTP directory retrieval' SFTP, directory [...] --- log/7/check-expected 2025-03-05 11:19:54.119658000 +0000 +++ log/7/check-generated 2025-03-05 11:19:54.119658000 +0000 @@ -1,3 +1,3 @@ d????????? N U U N ??? N NN:NN asubdir[LF] --rw?rw?rw? 1 U U 37 Jan 1 2000 plainfile.txt[LF] +-rw?r-?r-? 1 U U 37 Jan 1 2000 plainfile.txt[LF] -r-?r-?r-? 1 U U 47 Dec 31 2000 rofile.txt[LF] ``` https://github.com/curl/curl/actions/runs/13674664461/job/38233950866?pr=16570#step:14:1316 - Possibly a curl test portabibility, Perl or MSYS issue. in Cygwin tests: ``` FAIL-IGNORED 615: 'SFTP put remote failure' SFTP, SFTP put, FAILURE ``` https://github.com/curl/curl/actions/runs/13674664461/job/38233949428?pr=16570#step:12:3817 Follow-up to adcfd4f curl#16553 Ref: curl#14854 Closes curl#16570

These tests seem to be running no less stable now than others. Stop ignoring their results to catch real issues. These are consistently failing and remain on the ignore list: in MSVC / vcpkg jobs: ``` FAIL-IGNORED 2302: 'WebSockets via callback (frame mode) + curl_ws_send()' WebSockets FAIL-IGNORED 2303: 'WebSockets but gets a 200 back' WebSockets FAIL-IGNORED 2307: 'WebSockets, overlong PING payload' WebSockets ``` https://github.com/curl/curl/actions/runs/13674664461/job/38233949942?pr=16570#step:14:4089 - Likely curl issues either in tests, server, or in WebSockets support. in tests running under MSYS, affecting native mingw Windows builds only: ``` FAIL-IGNORED 612: 'SFTP post-quote remove file' SFTP, post-quote [...] curl: (21) rm command failed: Operation failed ``` https://github.com/curl/curl/actions/runs/13674664461/job/38233952699?pr=16570#step:14:1378 in tests running under MSYS, affecting both MSYS and native mingw Windows builds: ```diff FAIL-IGNORED 613: 'SFTP directory retrieval' SFTP, directory [...] --- log/7/check-expected 2025-03-05 11:19:54.119658000 +0000 +++ log/7/check-generated 2025-03-05 11:19:54.119658000 +0000 @@ -1,3 +1,3 @@ d????????? N U U N ??? N NN:NN asubdir[LF] --rw?rw?rw? 1 U U 37 Jan 1 2000 plainfile.txt[LF] +-rw?r-?r-? 1 U U 37 Jan 1 2000 plainfile.txt[LF] -r-?r-?r-? 1 U U 47 Dec 31 2000 rofile.txt[LF] ``` https://github.com/curl/curl/actions/runs/13674664461/job/38233950866?pr=16570#step:14:1316 - Possibly a curl test portabibility, Perl or MSYS issue. in Cygwin tests: ``` FAIL-IGNORED 615: 'SFTP put remote failure' SFTP, SFTP put, FAILURE ``` https://github.com/curl/curl/actions/runs/13674664461/job/38233949428?pr=16570#step:12:3817 Follow-up to adcfd4f #16553 Ref: #14854 Closes #16570

vszakats · 2025-03-06T18:50:35Z

The problems around this on Windows seem wider after further review:

curl uses both EINTR and EINVAL interchangeably for errno and SOCKERRNO throughout the codebase. In lib/src/libtests they're mapped to the winsock2 WSAEINTR and WSAEINVAL values respectively (so could only be used as SOCKERRNO). While in tests/server they're mapped to CRT errno values and also used as both errno and SOCKERRNO.

The winsock2 and CRT errno values are different.

Meaning, in tests/server, this PR fixed one side of the comparisons, but the other side may still need fixing. lib/src code would also need a review to see if there is any misuse. Finally these macro names/values should be synchronized between lib and server code, for clarity, and because these components do share code.

Still investigating.

jay · 2025-03-06T20:05:55Z

curl redefines posix socket error codes in Windows to use the Winsock values. I don't know why that was done it's before my time. For example curl defines EWOULDBLOCK as WSAEWOULDBLOCK (10035L). Normally EWOULDBLOCK would be 140 in Windows and defined by errno.

vszakats · 2025-03-06T20:17:24Z

curl redefines posix socket error codes in Windows to use the Winsock values. I don't know why that was done it's before my time. For example curl defines EWOULDBLOCK as WSAEWOULDBLOCK (10035L). Normally EWOULDBLOCK would be 140 in Windows and defined by errno.

Exactly, but then uses these redefined values to check against errno, which is filled by CRT functions, which are using the POSIX values (I haven't double-checked).

For example here:

curl/src/tool_cb_wrt.c

Lines 69 to 71 in 2d94439

    
             fd = open(fname, O_CREAT | O_WRONLY | O_EXCL | CURL_O_BINARY, OPENMODE); 
        
             /* Keep retrying in the hope that it is not interrupted sometime */ 
        
           } while(fd == -1 && errno == EINTR);

tests/server code also (re)defines these values, but to the POSIX ones, then compares it to WSA results after this patch. (before this patch it compared them with errno values on Windows which in turn weren't returning winsock2 errors at all.)

vszakats · 2025-03-06T20:53:17Z

I only checked EINVAL and EINTR, and these checks/sets seem wrong on Windows:

lib/src:

lib/curl_multibyte.c:    CURL_SETERRNO(EINVAL);
lib/curl_multibyte.c:    CURL_SETERRNO(EINVAL);
lib/curl_multibyte.c:    CURL_SETERRNO(EINVAL);
lib/curl_threads.c:               EACCES : EINVAL;
lib/vtls/gtls.c:                               (CURLE_AGAIN == result) ? EAGAIN : EINVAL);
lib/vtls/gtls.c:      gnutls_transport_set_errno(backend->gtls.session, EINVAL);
lib/vtls/gtls.c:                               (CURLE_AGAIN == result) ? EAGAIN : EINVAL);
lib/vtls/rustls.c:      ret = EINVAL;
lib/vtls/rustls.c:      ret = EINVAL;
src/tool_cb_wrt.c:    } while(fd == -1 && errno == EINTR);
src/tool_cb_wrt.c:        } while(fd == -1 && errno == EINTR);

tests/server:

tests/server/mqttd.c:    } while((rc == -1) && ((error = SOCKERRNO) == EINTR));
tests/server/sockfilt.c:  } while((rc == -1) && ((error = SOCKERRNO) == EINTR));
tests/server/socksd.c:    } while((rc == -1) && ((error = SOCKERRNO) == EINTR));
tests/server/sws.c:          } while(rc < 0 && SOCKERRNO == EINTR && !got_exit_signal);
tests/server/sws.c:        if(rc < 0 && SOCKERRNO != EINTR)
tests/server/sws.c:    } while(rc < 0 && SOCKERRNO == EINTR && !got_exit_signal);
tests/server/sws.c:    } while(rc < 0 && SOCKERRNO == EINTR && !got_exit_signal);

jay · 2025-03-07T07:46:56Z

Microsoft lists the practice as highly discouraged, despite style examples that don't make it easier to understand imo. I tried to clarify it. Some of their doc on this is antiquated. Anyway, I'm at a loss for a better solution if we want to continue using the BSD error codes rather than the Microsoft equivalent in code. (eg EWOULDBLOCK vs WSAEWOULDBLOCK respectively)

edit: We could add a windows_update_errno() function after every winsock call to read the WSA error code and assign it to the BSD equivalent in errno, that would allow getting rid of the redefinitions of the BSD error codes. I don't like it very much but spitballing here

(TEST PR, spitballing) - Stop redefining BSD socket error constants as WSA error constants in Windows. - Convert WSA error codes on the fly to BSD error codes when SOCKERRNO is used. Prior to this change we redefined the BSD error codes (like EWOULDBLOCK was redefined to WSAEWOULDBLOCK) but that caused problems with POSIX-like functions that are not Winsock functions which expect the real EWOULDBLOCK, EINTR, etc. Microsoft doc says the redefining is highly discouraged. Ref: curl#16553 (comment) Ref: https://learn.microsoft.com/en-us/windows/win32/winsock/error-codes-errno-h-errno-and-wsagetlasterror-2 Closes #xxxx

jay · 2025-03-07T09:03:49Z

alright here's another idea, translate the WSA error codes to BSD error codes in SOCKERRNO, ~~like #define SOCKERRNO (errno = winsock_error_to_bsd_socket_error(WSAGetLastError())~~. see #16605 for test

Windows's winsock2 returns socket errors via `WSAGetLastError()` and not via `errno` like most systems out there. This was covered by switching to the `SOCKERRNO` curl macro earlier. But, on Windows the returned socket error codes have different values than the standard POSIX errno values. Existing code was using the POSIX values for all these checks. Meaning they never actually matched on Windows. This patch defines a set of `SOCKERRNO` constants that map to the correct socket error values for Windows and other platforms. The same issue exists in core curl code, which redefines the POSIX errno values to winsock2 ones, breaking non-socket uses on Windows. Cherry-picked from curl#15000 Follow-up to adcfd4f curl#16553 Bug: curl#16553 (comment)

Windows's winsock2 returns socket errors via `WSAGetLastError()` and not via `errno` like most systems out there. This was covered by switching to the `SOCKERRNO` curl macro earlier. But, on Windows the returned socket error codes have different values than the standard POSIX errno values. Existing code was using the POSIX values for all these checks. Meaning they never actually matched on Windows. This patch defines a set of `SOCKERRNO` constants that map to the correct socket error values for Windows and other platforms. The reverse issue exists in core curl code, which redefines POSIX errno values to winsock2 ones, breaking non-socket uses on Windows. Cherry-picked from #15000 Follow-up to adcfd4f #16553 Bug: #16553 (comment) Closes #16612

These were not used in curl sources at all. Except `EDQUOT` which was used after `mkdir()` in `src/tool_dirhie.c` for error display. It should not be redefined to a winsock2 error. This makes the "exceeded your quota" error correctly appear on Windows, if detected, after operations that create directories. After this patch there remain 14 `E*` macro redefines on Windows, down from 40 before this patch. Bug: #16553 (comment) Ref: #16612 Ref: #16605 Closes #16615

(TEST PR, spitballing) - Stop redefining BSD socket error constants as WSA error constants in Windows. - Convert WSA error codes on the fly to BSD error codes when SOCKERRNO is used. Prior to this change we redefined the BSD error codes (like EWOULDBLOCK was redefined to WSAEWOULDBLOCK) but that caused problems with POSIX-like functions that are not Winsock functions which expect the real EWOULDBLOCK, EINTR, etc. Microsoft doc says the redefining is highly discouraged. Ref: curl#16553 (comment) Ref: https://learn.microsoft.com/en-us/windows/win32/winsock/error-codes-errno-h-errno-and-wsagetlasterror-2 Closes #xxxx

…ixes Before this patch, standard `E*` errno codes were redefined on Windows, onto matching winsock2 `WSA*` error codes, which have different values. This broke uses where using the `E*` value in non-socket context, or other places expecting a POSIX `errno`, e.g. file I/O, threads, IDN or interfacing with dependencies. Fix it by introducing a curl-specific `SOCKE*` set of macros that map to `WSA*` on Windows and standard POSIX codes on other platforms. Then verify and update the code to use `SOCKE*` or `E*` macro depending on context. - Add `SOCKE*` macros that map to either winsock2 or POSIX error codes. And use them with `SOCKERRNO` or in contexts requiring platform-dependent socket error codes. This fixes `E*` uses which were supposed be POSIX values, not `WSA*` socket errors, on Windows: - lib/curl_multibyte.c - lib/curl_threads.c - lib/idn.c - lib/vtls/gtls.c - lib/vtls/rustls.c - src/tool_cb_wrt.c - src/tool_dirhie.c - Ban `E*` codes having a `SOCKE*` mapping, via checksrc. Authored-by: Daniel Stenberg - Add exceptions for `E*` codes used in file I/O, or other contexts requiring POSIX error codes. Also: - ftp: fix missing `SOCKEACCES` mapping for Windows. - add `SOCKENOMEM` for `Curl_getaddrinfo()` via `asyn-thread.c`. - tests/server/sockfilt: fix to set `SOCKERRNO` in local `select()` override on Windows. - lib/inet_ntop: fix to return `WSAEINVAL` on Windows, where `ENOSPC` is used on other platforms. To simulate Windows' built-in `inet_ntop()`, as tested on a Win10 machine. Note: - WINE returns `STATUS_INVALID_PARAMETER` = `0xC000000D`. - Microsoft documentation says it returns `WSA_INVALID_PARAMETER` (= `ERROR_INVALID_PARAMETER`) 87: https://learn.microsoft.com/windows/win32/api/ws2tcpip/nf-ws2tcpip-inet_ntop#return-value - lib/inet_ntop: drop redundant `CURL_SETERRNO(ENOSPC)`. `inet_ntop4()` already sets it before returning `NULL`. - replace stray `WSAEWOULDBLOCK` with `USE_WINSOCK` macro to detect winsock2. - move existing `SOCKE*` mappings from `tests/server` to `curl_setup_once.h`. - add missing `EINTR`, `EINVAL` constants for WinCE. Follow-up to abf80aa #16612 Follow-up to d69425e #16615 Bug: #16553 (comment) Closes #16621

vszakats added tests Windows labels Mar 4, 2025

github-actions bot added the connecting & proxies label Mar 4, 2025

This comment was marked as resolved.

Sign in to view

vszakats changed the title ~~socksd: replace an errno with SOCKERRNO~~ tests/server: replace an errno with SOCKERRNO in sockfilt, socksd, sws Mar 4, 2025

vszakats changed the title ~~tests/server: replace an errno with SOCKERRNO in sockfilt, socksd, sws~~ tests/server: replace errno with SOCKERRNO in sockfilt, socksd, sws Mar 4, 2025

vszakats added 3 commits March 4, 2025 16:55

sws: replace more errno that should be SOCKERRNO

Verified

This commit was signed with the committer’s verified signature.

vszakats Viktor Szakats

GPG key ID: B5ABD165E2AEF201

Verified
Learn about vigilant mode

4748bfb

sockfilt: replace more errno that should be SOCKERRNO

Verified

This commit was signed with the committer’s verified signature.

vszakats Viktor Szakats

GPG key ID: B5ABD165E2AEF201

Verified
Learn about vigilant mode

7f6cf2e

vszakats force-pushed the t-sockerrno branch from 1496a7d to 7f6cf2e Compare March 4, 2025 15:55

vszakats changed the title ~~tests/server: replace errno with SOCKERRNO in sockfilt, socksd, sws~~ tests/server: fix errno to SOCKERRNO in sockfilt, socksd, sws Mar 4, 2025

vszakats changed the title ~~tests/server: fix errno to SOCKERRNO in sockfilt, socksd, sws~~ tests/server: replace errno with SOCKERRNO in sockfilt, socksd, sws Mar 4, 2025

vszakats closed this in adcfd4f Mar 4, 2025

vszakats deleted the t-sockerrno branch March 4, 2025 17:25

vszakats mentioned this pull request Mar 5, 2025

GHA/windows: stop ignoring most ignored test results #16570

Closed

jay mentioned this pull request Mar 7, 2025

lib: revamp SOCKERRNO on Windows #16605

Closed

vszakats mentioned this pull request Mar 7, 2025

tests/server: fix to check against winsock2 error codes on Windows #16612

Closed

vszakats mentioned this pull request Mar 7, 2025

curl_setup_once: drop E* macro redefines unused (with winsock2) #16615

Closed

vszakats mentioned this pull request Mar 8, 2025

core: stop redefining E* macros on Windows, map EACCES, related fixes #16621

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests/server: replace `errno` with `SOCKERRNO` in sockfilt, socksd, sws #16553

tests/server: replace `errno` with `SOCKERRNO` in sockfilt, socksd, sws #16553

vszakats commented Mar 4, 2025 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

vszakats commented Mar 6, 2025 •

edited

Loading

jay commented Mar 6, 2025

vszakats commented Mar 6, 2025 •

edited

Loading

vszakats commented Mar 6, 2025 •

edited

Loading

jay commented Mar 7, 2025 •

edited

Loading

jay commented Mar 7, 2025 •

edited

Loading

tests/server: replace errno with SOCKERRNO in sockfilt, socksd, sws #16553

tests/server: replace errno with SOCKERRNO in sockfilt, socksd, sws #16553

Conversation

vszakats commented Mar 4, 2025 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

vszakats commented Mar 6, 2025 • edited Loading

jay commented Mar 6, 2025

vszakats commented Mar 6, 2025 • edited Loading

vszakats commented Mar 6, 2025 • edited Loading

jay commented Mar 7, 2025 • edited Loading

jay commented Mar 7, 2025 • edited Loading

tests/server: replace `errno` with `SOCKERRNO` in sockfilt, socksd, sws #16553

tests/server: replace `errno` with `SOCKERRNO` in sockfilt, socksd, sws #16553

vszakats commented Mar 4, 2025 •

edited

Loading

vszakats commented Mar 6, 2025 •

edited

Loading

vszakats commented Mar 6, 2025 •

edited

Loading

vszakats commented Mar 6, 2025 •

edited

Loading

jay commented Mar 7, 2025 •

edited

Loading

jay commented Mar 7, 2025 •

edited

Loading