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

cmake: fix support for UnixSockets feature on Win32 #7034

Closed
wants to merge 1 commit into from
Closed

cmake: fix support for UnixSockets feature on Win32 #7034

wants to merge 1 commit into from

Conversation

lixinwei715
Copy link
Contributor

@lixinwei715 lixinwei715 commented May 8, 2021

Move the definition of sockaddr_un struct from config-win32.h to curl_setup.h, so that it could be shared by all build systems.

Add ADDRESS_FAMILY typedef for old mingw, now old mingw can also use unix sockets.

Also fix the build of tests/server/sws.c on Win32 when USE_UNIX_SOCKETS is defined.

Fixes #7029

@bagder bagder added cmake Windows Windows-specific labels May 8, 2021
@lixinwei715
Copy link
Contributor Author

tests/server/sws.exe build failed, because Windows does not have lstat() function
tests/server/sws.c line 2087~2089:

      /* socket server is not alive, now check if it was actually a socket.
       * Systems which have Unix sockets will also have lstat */
      rc = lstat(unix_socket, &statbuf);

How to fix it?

@bagder
Copy link
Member

bagder commented May 8, 2021

Systems which have Unix sockets will also have lstat

Obviously then a wrong comment! =)

I figure you can just use stat() then, since symbolic links are not used on windows anyway.

@bagder
Copy link
Member

bagder commented May 11, 2021

@vszakats you brought 23a870f, any thoughts on this PR?

@vszakats
Copy link
Member

vszakats commented May 11, 2021

The gist of this seems to move the existing UnixSockets trick to curl_addrinfo.c, making it available for UWP, but the CMake-specific logic and moving the small header stub into a separate file look unnecessary. Also the, sws.c changes seem unrelated to this patch, which is okay, but would probably be best in a separate patch to make this easier to understand.

Update: Moving around the UnixSockets trick also seems unnecessary, because UWP apps aren't an issue here (as introduced in 79a05e1).

@vszakats
Copy link
Member

vszakats commented May 11, 2021

I didn't test it, but would this simpler patch solve the CMake build issue?:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2ffb72faf..757599767 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -793,7 +793,9 @@ endif()
 option(ENABLE_UNIX_SOCKETS "Define if you want Unix domain sockets support" ON)
 if(ENABLE_UNIX_SOCKETS)
   include(CheckStructHasMember)
-  check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  if(NOT WIN32)
+    check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  endif()
 else()
   unset(USE_UNIX_SOCKETS CACHE)
 endif()

@lixinwei715
Copy link
Contributor Author

I didn't test it, but would this simpler patch solve the CMake build issue?:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2ffb72faf..757599767 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -793,7 +793,9 @@ endif()
 option(ENABLE_UNIX_SOCKETS "Define if you want Unix domain sockets support" ON)
 if(ENABLE_UNIX_SOCKETS)
   include(CheckStructHasMember)
-  check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  if(NOT WIN32)
+    check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  endif()
 else()
   unset(USE_UNIX_SOCKETS CACHE)
 endif()

Only changing CMakeLists.txt is not enough.
Please view the description of issue #7029.

@lixinwei715
Copy link
Contributor Author

lixinwei715 commented May 11, 2021

The gist of this seems to move the existing UnixSockets trick to curl_addrinfo.c, making it available for UWP, but the CMake-specific logic and moving the small header stub into a separate file look unnecessary. Also the, sws.c changes seem unrelated to this patch, which is okay, but would probably be best in a separate patch to make this easier to understand.

Update: Moving around the UnixSockets trick also seems unnecessary, because UWP apps aren't an issue here (as introduced in 79a05e1).

The UnixSockets trick only exists in config-win32.h, but cmake does not use config-win32.h. So I move the UnixSockets trick to a new header file "unixsockets_win32.h". But according to @bagder 's comment in #7029 , we can also move it to curl_config.h. If you think there is no need to create a new header file, I will move the UnixSockets trick to curl_config.h.
Without changing sws.c, compile on appveyor will fail, when USE_UNIX_SOCKETS is ON and Win32 platform. So sws.c changes is related to this patch.

@vszakats
Copy link
Member

vszakats commented May 11, 2021

@lixinwei715 Thanks for this info, I understand now. It looks a little bit weird that CMake doesn't use config-win32.h though. It makes things much more bumpy for CMake. I wonder what is the benefit/reason behind it not using it?

[ FWIW I tried moving to CMake in curl-for-win, but it was so limited compared to the bare-bones Makefile.m32 method, that I had to shelve the effort. ]

Re: to sws.c, Makefile.m32 doesn't build tests at all, so that's indeed related when built with CMake, which does.

@vszakats
Copy link
Member

vszakats commented May 11, 2021

The best place for the Windows UnixSockets header trick is where it is moved by this patch (curl_setup.h). curl_config.h isn't used by some build systems, such as Makefile.m32, and possibly winbuild.

@lixinwei715
Copy link
Contributor Author

@vszakats cmake doesn't use config-win32.h, that's because cmake will generate its curl_config.h, based on curl_config.h.cmake.
Compared to other build systems used by curl, there are still some issues on cmake indeed. I plan to commit some PRs to amend the cmake build system in the next few weeks.

lib/curl_setup.h Outdated Show resolved Hide resolved
@bagder bagder requested a review from vszakats June 2, 2021 06:52
@ghost
Copy link

ghost commented Jun 2, 2021

Congratulations 🎉. DeepCode analyzed your code in 2.397 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Copy link
Member

@vszakats vszakats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The new WIN32 header check in CMakeLists.txt will always be true (it is looking for a header inside curl's own source tree), so I'm not convinced that the newly added header (and the check) is necessary here.
  2. The extra UWP logic was stripped from the last version of this PR. I can't test it, but if WIN32 is defined for UWP apps, this should work.

Modified patch below attempts to move Unix Sockets detection (and any necessary tricks) into curl_setup.h, so all Windows builds systems can use it.

@dmitrykos, @lixinwei715 Can you verify/test if this works for CMake and keeps working for UWP apps?

UPDATE: re-added CMake-level pre-detection via curl_setup.h.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1f49dcf3b..b1f4cc86b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -806,7 +806,13 @@ endif()
 option(ENABLE_UNIX_SOCKETS "Define if you want Unix domain sockets support" ON)
 if(ENABLE_UNIX_SOCKETS)
   include(CheckStructHasMember)
-  check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  if(WIN32)
+    set(CMAKE_REQUIRED_INCLUDES "${CMAKE_CURRENT_SOURCE_DIR}/include")
+    check_struct_has_member("struct sockaddr_un" sun_path
+      "${CMAKE_CURRENT_SOURCE_DIR}/lib/curl_setup.h" USE_UNIX_SOCKETS)
+  else()
+    check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  endif()
 else()
   unset(USE_UNIX_SOCKETS CACHE)
 endif()
diff --git a/lib/config-win32.h b/lib/config-win32.h
index fc8153683..5e4997bf0 100644
--- a/lib/config-win32.h
+++ b/lib/config-win32.h
@@ -693,23 +693,6 @@ Vista
 #define USE_WIN32_CRYPTO
 #endif
 
-/* On MinGW the ADDRESS_FAMILY typedef was committed alongside LUP_SECURE,
-   so we use it to check for the presence of the typedef. */
-#include <ws2tcpip.h>
-#if !defined(__MINGW32__) || defined(LUP_SECURE)
-/* Define to use Unix sockets. */
-#define USE_UNIX_SOCKETS
-#if !defined(UNIX_PATH_MAX)
-  /* Replicating logic present in afunix.h of newer Windows 10 SDK versions */
-# define UNIX_PATH_MAX 108
-  /* !checksrc! disable TYPEDEFSTRUCT 1 */
-  typedef struct sockaddr_un {
-    ADDRESS_FAMILY sun_family;
-    char sun_path[UNIX_PATH_MAX];
-  } SOCKADDR_UN, *PSOCKADDR_UN;
-#endif
-#endif
-
 /* ---------------------------------------------------------------- */
 /*                       ADDITIONAL DEFINITIONS                     */
 /* ---------------------------------------------------------------- */
diff --git a/lib/curl_addrinfo.c b/lib/curl_addrinfo.c
index 1d5067bc0..842fd7fe2 100644
--- a/lib/curl_addrinfo.c
+++ b/lib/curl_addrinfo.c
@@ -50,12 +50,6 @@
 #  define in_addr_t unsigned long
 #endif
 
-#if defined(USE_UNIX_SOCKETS) && defined(WINAPI_FAMILY) && \
-    (WINAPI_FAMILY == WINAPI_FAMILY_APP)
-   /* Required for sockaddr_un type */
-#  include <afunix.h>
-#endif
-
 #include <stddef.h>
 
 #include "curl_addrinfo.h"
diff --git a/lib/curl_setup.h b/lib/curl_setup.h
index 0b575ef78..76d63dc3d 100644
--- a/lib/curl_setup.h
+++ b/lib/curl_setup.h
@@ -815,4 +815,24 @@ int getpwuid_r(uid_t uid, struct passwd *pwd, char *buf,
 #define ENABLE_QUIC
 #endif
 
+/* Detect if we can use Unix Sockets on Windows,
+   then do any necessary preparations */
+#if defined(WIN32) && !defined(_WIN32_WCE)
+   /* On MinGW the ADDRESS_FAMILY typedef was committed alongside LUP_SECURE,
+      so we use it to check for the presence of the typedef. */
+#  if !defined(__MINGW32__) || defined(LUP_SECURE)
+#    define USE_UNIX_SOCKETS
+#    if !defined(UNIX_PATH_MAX)  /* Check if afunix.h was already included */
+       /* Replicate logic from afunix.h
+          (distributed with newer Windows 10 SDK versions only) */
+#      define UNIX_PATH_MAX 108
+       /* !checksrc! disable TYPEDEFSTRUCT 1 */
+       typedef struct sockaddr_un {
+         ADDRESS_FAMILY sun_family;
+         char sun_path[UNIX_PATH_MAX];
+       } SOCKADDR_UN, *PSOCKADDR_UN;
+#    endif
+#  endif
+#endif
+
 #endif /* HEADER_CURL_SETUP_H */
diff --git a/tests/server/sws.c b/tests/server/sws.c
index b1137414d..e60e9dfbe 100644
--- a/tests/server/sws.c
+++ b/tests/server/sws.c
@@ -2065,9 +2065,9 @@ int main(int argc, char *argv[])
     strncpy(me.sau.sun_path, unix_socket, sizeof(me.sau.sun_path) - 1);
     rc = bind(sock, &me.sa, sizeof(me.sau));
     if(0 != rc && errno == EADDRINUSE) {
-      struct stat statbuf;
+      struct_stat statbuf;
       /* socket already exists. Perhaps it is stale? */
-      int unixfd = socket(AF_UNIX, SOCK_STREAM, 0);
+      curl_socket_t unixfd = socket(AF_UNIX, SOCK_STREAM, 0);
       if(CURL_SOCKET_BAD == unixfd) {
         error = SOCKERRNO;
         logmsg("Error binding socket, failed to create socket at %s: (%d) %s",
@@ -2077,15 +2077,19 @@ int main(int argc, char *argv[])
       /* check whether the server is alive */
       rc = connect(unixfd, &me.sa, sizeof(me.sau));
       error = errno;
-      close(unixfd);
+      sclose(unixfd);
       if(ECONNREFUSED != error) {
         logmsg("Error binding socket, failed to connect to %s: (%d) %s",
                unix_socket, error, strerror(error));
         goto sws_cleanup;
       }
-      /* socket server is not alive, now check if it was actually a socket.
-       * Systems which have Unix sockets will also have lstat */
+      /* socket server is not alive, now check if it was actually a socket. */
+#ifdef WIN32
+      /* Windows does not have lstat function. */
+      rc = curlx_win32_stat(unix_socket, &statbuf);
+#else
       rc = lstat(unix_socket, &statbuf);
+#endif
       if(0 != rc) {
         logmsg("Error binding socket, failed to stat %s: (%d) %s",
                unix_socket, errno, strerror(errno));

CMakeLists.txt Outdated
check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
if(WIN32)
check_struct_has_member("struct sockaddr_un" sun_path
"${CMAKE_CURRENT_SOURCE_DIR}/lib/unixsockets_win32.h" USE_UNIX_SOCKETS)

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMake will print a summary in the end, like this:

-- Enabled features: SSL IPv6 libz brotli zstd AsynchDNS IDN Largefile SSPI alt-svc HSTS SPNEGO Kerberos NTLM HTTP2 unicode
-- Enabled protocols: DICT FILE FTP FTPS GOPHER GOPHERS HTTP HTTPS IMAP IMAPS LDAP LDAPS MQTT POP3 POP3S RTSP SCP SFTP SMB SMBS SMTP SMTPS TELNET TFTP
-- Enabled SSL backends: Schannel

The check of unixsockets should be done by cmake. So that we can see "unixsockets" in "Enabled features" before compiling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You move Unix Sockets detection to curl_setup.h, this means whether unix sockets enable of disable will be automatically determined, so that cmake option "ENABLE_UNIX_SOCKETS" doesn't work on Windows. Users cannot disable unix sockets feature by seting ENABLE_UNIX_SOCKETS to OFF.

Copy link
Member

@vszakats vszakats Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this code work for detection in CMake?:

  if(WIN32)
    set(CMAKE_REQUIRED_INCLUDES "${CMAKE_CURRENT_SOURCE_DIR}/include")
    check_struct_has_member("struct sockaddr_un" sun_path
      "${CMAKE_CURRENT_SOURCE_DIR}/lib/curl_setup.h" USE_UNIX_SOCKETS)

This comment was marked as resolved.

This comment was marked as resolved.

lib/curl_setup.h Show resolved Hide resolved
lib/curl_setup.h Outdated Show resolved Hide resolved
@lixinwei715
Copy link
Contributor Author

The new WIN32 header check in CMakeLists.txt will always be true

No, for 10y+ old mingw, !defined(__MINGW32__) || defined(LUP_SECURE) is false, sockaddr_un will not defined, the result of check_struct_has_member will be false.

If you want to know how check_struct_has_member works, please view:
https://github.com/Kitware/CMake/blob/master/Modules/CheckStructHasMember.cmake

@vszakats
Copy link
Member

vszakats commented Jun 2, 2021

@lixinwei715 Got it now, it's a pre-check for CMake to know (not for the file itself, but for the actual struct). Can you take a look at #7034 (comment)?

@vszakats
Copy link
Member

vszakats commented Jun 2, 2021

It looks like we're trying to achieve slightly different things. The title says 'fix UnixSockets support with CMake on Windows'. Fixing this doesn't need duplicate CMake-level feature detection. Other Windows build systems enable this feature automatically at compile-time, so it's expected for CMake to behave the same.

So, I'd personally keep it simple and not add an extra source file to detect one feature on a single platform and single make system, and instead prefer to keep the already-too-messy Windows detection/enabler logic in one block of code shared by all build systems and compilers.

If we want to implement a new feature where Unix Sockets can be manually force-disabled, a CURL_DISABLE_UNIX_SOCKETS or similar setting can be added, and make CMake set it when ENABLE_UNIX_SOCKETS was set to 0. I've added it to this patch:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1f49dcf3b..29cb6f4e3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -805,9 +805,12 @@ endif()
 
 option(ENABLE_UNIX_SOCKETS "Define if you want Unix domain sockets support" ON)
 if(ENABLE_UNIX_SOCKETS)
-  include(CheckStructHasMember)
-  check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  if(NOT WIN32)  # On Windows, Unix sockets detection is done in curl_setup.h
+    include(CheckStructHasMember)
+    check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  endif()
 else()
+  set(CURL_DISABLE_UNIX_SOCKETS ON)
   unset(USE_UNIX_SOCKETS CACHE)
 endif()
 
diff --git a/lib/config-win32.h b/lib/config-win32.h
index fc8153683..5e4997bf0 100644
--- a/lib/config-win32.h
+++ b/lib/config-win32.h
@@ -693,23 +693,6 @@ Vista
 #define USE_WIN32_CRYPTO
 #endif
 
-/* On MinGW the ADDRESS_FAMILY typedef was committed alongside LUP_SECURE,
-   so we use it to check for the presence of the typedef. */
-#include <ws2tcpip.h>
-#if !defined(__MINGW32__) || defined(LUP_SECURE)
-/* Define to use Unix sockets. */
-#define USE_UNIX_SOCKETS
-#if !defined(UNIX_PATH_MAX)
-  /* Replicating logic present in afunix.h of newer Windows 10 SDK versions */
-# define UNIX_PATH_MAX 108
-  /* !checksrc! disable TYPEDEFSTRUCT 1 */
-  typedef struct sockaddr_un {
-    ADDRESS_FAMILY sun_family;
-    char sun_path[UNIX_PATH_MAX];
-  } SOCKADDR_UN, *PSOCKADDR_UN;
-#endif
-#endif
-
 /* ---------------------------------------------------------------- */
 /*                       ADDITIONAL DEFINITIONS                     */
 /* ---------------------------------------------------------------- */
diff --git a/lib/curl_addrinfo.c b/lib/curl_addrinfo.c
index 1d5067bc0..842fd7fe2 100644
--- a/lib/curl_addrinfo.c
+++ b/lib/curl_addrinfo.c
@@ -50,12 +50,6 @@
 #  define in_addr_t unsigned long
 #endif
 
-#if defined(USE_UNIX_SOCKETS) && defined(WINAPI_FAMILY) && \
-    (WINAPI_FAMILY == WINAPI_FAMILY_APP)
-   /* Required for sockaddr_un type */
-#  include <afunix.h>
-#endif
-
 #include <stddef.h>
 
 #include "curl_addrinfo.h"
diff --git a/lib/curl_setup.h b/lib/curl_setup.h
index 0b575ef78..bf6aedb45 100644
--- a/lib/curl_setup.h
+++ b/lib/curl_setup.h
@@ -815,4 +815,25 @@ int getpwuid_r(uid_t uid, struct passwd *pwd, char *buf,
 #define ENABLE_QUIC
 #endif
 
+/* Detect if we can use Unix Sockets on Windows,
+   then do any necessary preparations */
+#if defined(WIN32) && !defined(_WIN32_WCE) && \
+    !defined(CURL_DISABLE_UNIX_SOCKETS)
+   /* On MinGW the ADDRESS_FAMILY typedef was committed alongside LUP_SECURE,
+      so we use it to check for the presence of the typedef. */
+#  if !defined(__MINGW32__) || defined(LUP_SECURE)
+#    define USE_UNIX_SOCKETS
+#    if !defined(UNIX_PATH_MAX)  /* Check if afunix.h was already included */
+       /* Replicate logic in afunix.h
+          (distributed with newer Windows 10 SDK versions only) */
+#      define UNIX_PATH_MAX 108
+       /* !checksrc! disable TYPEDEFSTRUCT 1 */
+       typedef struct sockaddr_un {
+         ADDRESS_FAMILY sun_family;
+         char sun_path[UNIX_PATH_MAX];
+       } SOCKADDR_UN, *PSOCKADDR_UN;
+#    endif
+#  endif
+#endif
+
 #endif /* HEADER_CURL_SETUP_H */
diff --git a/tests/server/sws.c b/tests/server/sws.c
index b1137414d..e60e9dfbe 100644
--- a/tests/server/sws.c
+++ b/tests/server/sws.c
@@ -2065,9 +2065,9 @@ int main(int argc, char *argv[])
     strncpy(me.sau.sun_path, unix_socket, sizeof(me.sau.sun_path) - 1);
     rc = bind(sock, &me.sa, sizeof(me.sau));
     if(0 != rc && errno == EADDRINUSE) {
-      struct stat statbuf;
+      struct_stat statbuf;
       /* socket already exists. Perhaps it is stale? */
-      int unixfd = socket(AF_UNIX, SOCK_STREAM, 0);
+      curl_socket_t unixfd = socket(AF_UNIX, SOCK_STREAM, 0);
       if(CURL_SOCKET_BAD == unixfd) {
         error = SOCKERRNO;
         logmsg("Error binding socket, failed to create socket at %s: (%d) %s",
@@ -2077,15 +2077,19 @@ int main(int argc, char *argv[])
       /* check whether the server is alive */
       rc = connect(unixfd, &me.sa, sizeof(me.sau));
       error = errno;
-      close(unixfd);
+      sclose(unixfd);
       if(ECONNREFUSED != error) {
         logmsg("Error binding socket, failed to connect to %s: (%d) %s",
                unix_socket, error, strerror(error));
         goto sws_cleanup;
       }
-      /* socket server is not alive, now check if it was actually a socket.
-       * Systems which have Unix sockets will also have lstat */
+      /* socket server is not alive, now check if it was actually a socket. */
+#ifdef WIN32
+      /* Windows does not have lstat function. */
+      rc = curlx_win32_stat(unix_socket, &statbuf);
+#else
       rc = lstat(unix_socket, &statbuf);
+#endif
       if(0 != rc) {
         logmsg("Error binding socket, failed to stat %s: (%d) %s",
                unix_socket, errno, strerror(errno));

@lixinwei715
Copy link
Contributor Author

As far as I know, the reason we disable unix sockets for old mingw is that old mingw missing ADDRESS_FAMILY typedef.
Could we add this typedef maually? So that unix sockets could be enabled on all Windows, cmake don't need to check.

#if defined(__MINGW32__) && !defined(LUP_SECURE)
  typedef u_short ADDRESS_FAMILY;
#endif
#if !defined(UNIX_PATH_MAX)
  /* Replicating logic present in afunix.h of newer Windows 10 SDK versions */
# define UNIX_PATH_MAX 108
  /* !checksrc! disable TYPEDEFSTRUCT 1 */
  typedef struct sockaddr_un {
    ADDRESS_FAMILY sun_family;
    char sun_path[UNIX_PATH_MAX];
  } SOCKADDR_UN, *PSOCKADDR_UN;
#endif

@lixinwei715
Copy link
Contributor Author

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2ffb72faf..6fcb68e79 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -793,7 +793,11 @@ endif()
 option(ENABLE_UNIX_SOCKETS "Define if you want Unix domain sockets support" ON)
 if(ENABLE_UNIX_SOCKETS)
   include(CheckStructHasMember)
-  check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  if(WIN32 AND NOT WINCE)
+    set(USE_UNIX_SOCKETS ON)
+  else()
+    check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  endif()
 else()
   unset(USE_UNIX_SOCKETS CACHE)
 endif()
diff --git a/lib/config-win32.h b/lib/config-win32.h
index 2e060f95c..5fca98c16 100644
--- a/lib/config-win32.h
+++ b/lib/config-win32.h
@@ -713,22 +713,8 @@ Vista
 #define USE_WIN32_CRYPTO
 #endif
 
-/* On MinGW the ADDRESS_FAMILY typedef was committed alongside LUP_SECURE,
-   so we use it to check for the presence of the typedef. */
-#include <ws2tcpip.h>
-#if !defined(__MINGW32__) || defined(LUP_SECURE)
-/* Define to use Unix sockets. */
+/* If Unix domain sockets are enabled. */
 #define USE_UNIX_SOCKETS
-#if !defined(UNIX_PATH_MAX)
-  /* Replicating logic present in afunix.h of newer Windows 10 SDK versions */
-# define UNIX_PATH_MAX 108
-  /* !checksrc! disable TYPEDEFSTRUCT 1 */
-  typedef struct sockaddr_un {
-    ADDRESS_FAMILY sun_family;
-    char sun_path[UNIX_PATH_MAX];
-  } SOCKADDR_UN, *PSOCKADDR_UN;
-#endif
-#endif
 
 /* ---------------------------------------------------------------- */
 /*                       ADDITIONAL DEFINITIONS                     */
diff --git a/lib/curl_addrinfo.c b/lib/curl_addrinfo.c
index 1d5067bc0..842fd7fe2 100644
--- a/lib/curl_addrinfo.c
+++ b/lib/curl_addrinfo.c
@@ -50,12 +50,6 @@
 #  define in_addr_t unsigned long
 #endif
 
-#if defined(USE_UNIX_SOCKETS) && defined(WINAPI_FAMILY) && \
-    (WINAPI_FAMILY == WINAPI_FAMILY_APP)
-   /* Required for sockaddr_un type */
-#  include <afunix.h>
-#endif
-
 #include <stddef.h>
 
 #include "curl_addrinfo.h"
diff --git a/lib/curl_setup.h b/lib/curl_setup.h
index 004accdf1..aa975669f 100644
--- a/lib/curl_setup.h
+++ b/lib/curl_setup.h
@@ -809,4 +809,21 @@ int getpwuid_r(uid_t uid, struct passwd *pwd, char *buf,
 #define ENABLE_QUIC
 #endif
 
+#if defined(USE_UNIX_SOCKETS) && defined(WIN32) && !defined(_WIN32_WCE)
+   /* On MinGW the ADDRESS_FAMILY typedef was committed alongside LUP_SECURE,
+      so we use it to check for the presence of the typedef. */
+#  if defined(__MINGW32__) && !defined(LUP_SECURE)
+     typedef u_short ADDRESS_FAMILY;
+#  endif
+#  if !defined(UNIX_PATH_MAX)
+     /* Replicating logic present in afunix.h of newer Windows 10 SDK versions */
+#    define UNIX_PATH_MAX 108
+     /* !checksrc! disable TYPEDEFSTRUCT 1 */
+     typedef struct sockaddr_un {
+       ADDRESS_FAMILY sun_family;
+       char sun_path[UNIX_PATH_MAX];
+     } SOCKADDR_UN, *PSOCKADDR_UN;
+#  endif
+#endif
+
 #endif /* HEADER_CURL_SETUP_H */
diff --git a/tests/server/sws.c b/tests/server/sws.c
index 99efa3786..b5b179386 100644
--- a/tests/server/sws.c
+++ b/tests/server/sws.c
@@ -2066,9 +2066,9 @@ int main(int argc, char *argv[])
     strncpy(me.sau.sun_path, unix_socket, sizeof(me.sau.sun_path) - 1);
     rc = bind(sock, &me.sa, sizeof(me.sau));
     if(0 != rc && errno == EADDRINUSE) {
-      struct stat statbuf;
+      struct_stat statbuf;
       /* socket already exists. Perhaps it is stale? */
-      int unixfd = socket(AF_UNIX, SOCK_STREAM, 0);
+      curl_socket_t unixfd = socket(AF_UNIX, SOCK_STREAM, 0);
       if(CURL_SOCKET_BAD == unixfd) {
         error = SOCKERRNO;
         logmsg("Error binding socket, failed to create socket at %s: (%d) %s",
@@ -2078,15 +2078,19 @@ int main(int argc, char *argv[])
       /* check whether the server is alive */
       rc = connect(unixfd, &me.sa, sizeof(me.sau));
       error = errno;
-      close(unixfd);
+      sclose(unixfd);
       if(ECONNREFUSED != error) {
         logmsg("Error binding socket, failed to connect to %s: (%d) %s",
                unix_socket, error, strerror(error));
         goto sws_cleanup;
       }
-      /* socket server is not alive, now check if it was actually a socket.
-       * Systems which have Unix sockets will also have lstat */
+      /* socket server is not alive, now check if it was actually a socket. */
+#ifdef WIN32
+      /* Windows does not have lstat function. */
+      rc = curlx_win32_stat(unix_socket, &statbuf);
+#else
       rc = lstat(unix_socket, &statbuf);
+#endif
       if(0 != rc) {
         logmsg("Error binding socket, failed to stat %s: (%d) %s",
                unix_socket, errno, strerror(errno));

@vszakats
Copy link
Member

vszakats commented Jun 2, 2021

@lixinwei715 Yes, this could work! As far as I could verify with a few old mingw releases, u_short is available.

If you agree with this revision, feel free to update this PR with it. Then we can see what the CIs say:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1f49dcf3b..efad9df68 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -805,9 +805,12 @@ endif()
 
 option(ENABLE_UNIX_SOCKETS "Define if you want Unix domain sockets support" ON)
 if(ENABLE_UNIX_SOCKETS)
-  include(CheckStructHasMember)
-  check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  if(NOT WIN32)
+    include(CheckStructHasMember)
+    check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
+  endif()
 else()
+  set(CURL_DISABLE_UNIX_SOCKETS ON)
   unset(USE_UNIX_SOCKETS CACHE)
 endif()
 
diff --git a/lib/config-win32.h b/lib/config-win32.h
index fc8153683..5e4997bf0 100644
--- a/lib/config-win32.h
+++ b/lib/config-win32.h
@@ -693,23 +693,6 @@ Vista
 #define USE_WIN32_CRYPTO
 #endif
 
-/* On MinGW the ADDRESS_FAMILY typedef was committed alongside LUP_SECURE,
-   so we use it to check for the presence of the typedef. */
-#include <ws2tcpip.h>
-#if !defined(__MINGW32__) || defined(LUP_SECURE)
-/* Define to use Unix sockets. */
-#define USE_UNIX_SOCKETS
-#if !defined(UNIX_PATH_MAX)
-  /* Replicating logic present in afunix.h of newer Windows 10 SDK versions */
-# define UNIX_PATH_MAX 108
-  /* !checksrc! disable TYPEDEFSTRUCT 1 */
-  typedef struct sockaddr_un {
-    ADDRESS_FAMILY sun_family;
-    char sun_path[UNIX_PATH_MAX];
-  } SOCKADDR_UN, *PSOCKADDR_UN;
-#endif
-#endif
-
 /* ---------------------------------------------------------------- */
 /*                       ADDITIONAL DEFINITIONS                     */
 /* ---------------------------------------------------------------- */
diff --git a/lib/curl_addrinfo.c b/lib/curl_addrinfo.c
index 1d5067bc0..842fd7fe2 100644
--- a/lib/curl_addrinfo.c
+++ b/lib/curl_addrinfo.c
@@ -50,12 +50,6 @@
 #  define in_addr_t unsigned long
 #endif
 
-#if defined(USE_UNIX_SOCKETS) && defined(WINAPI_FAMILY) && \
-    (WINAPI_FAMILY == WINAPI_FAMILY_APP)
-   /* Required for sockaddr_un type */
-#  include <afunix.h>
-#endif
-
 #include <stddef.h>
 
 #include "curl_addrinfo.h"
diff --git a/lib/curl_setup.h b/lib/curl_setup.h
index 0b575ef78..4dc63cfa8 100644
--- a/lib/curl_setup.h
+++ b/lib/curl_setup.h
@@ -815,4 +815,24 @@ int getpwuid_r(uid_t uid, struct passwd *pwd, char *buf,
 #define ENABLE_QUIC
 #endif
 
+/* Detect if we can use Unix Sockets on Windows,
+   then do any necessary preparations */
+#if defined(WIN32) && !defined(_WIN32_WCE) && \
+    !defined(CURL_DISABLE_UNIX_SOCKETS)
+#  define USE_UNIX_SOCKETS
+#  if defined(__MINGW32__) && !defined(LUP_SECURE)
+     typedef u_short ADDRESS_FAMILY;  /* Classic mingw, 11y+ old mingw-w64 */
+#  endif
+#  if !defined(UNIX_PATH_MAX)  /* Check if afunix.h was already included */
+     /* Replicate logic in afunix.h
+        (distributed with newer Windows 10 SDK versions only) */
+#    define UNIX_PATH_MAX 108
+     /* !checksrc! disable TYPEDEFSTRUCT 1 */
+     typedef struct sockaddr_un {
+       ADDRESS_FAMILY sun_family;
+       char sun_path[UNIX_PATH_MAX];
+     } SOCKADDR_UN, *PSOCKADDR_UN;
+#  endif
+#endif
+
 #endif /* HEADER_CURL_SETUP_H */
diff --git a/tests/server/sws.c b/tests/server/sws.c
index b1137414d..e60e9dfbe 100644
--- a/tests/server/sws.c
+++ b/tests/server/sws.c
@@ -2065,9 +2065,9 @@ int main(int argc, char *argv[])
     strncpy(me.sau.sun_path, unix_socket, sizeof(me.sau.sun_path) - 1);
     rc = bind(sock, &me.sa, sizeof(me.sau));
     if(0 != rc && errno == EADDRINUSE) {
-      struct stat statbuf;
+      struct_stat statbuf;
       /* socket already exists. Perhaps it is stale? */
-      int unixfd = socket(AF_UNIX, SOCK_STREAM, 0);
+      curl_socket_t unixfd = socket(AF_UNIX, SOCK_STREAM, 0);
       if(CURL_SOCKET_BAD == unixfd) {
         error = SOCKERRNO;
         logmsg("Error binding socket, failed to create socket at %s: (%d) %s",
@@ -2077,15 +2077,19 @@ int main(int argc, char *argv[])
       /* check whether the server is alive */
       rc = connect(unixfd, &me.sa, sizeof(me.sau));
       error = errno;
-      close(unixfd);
+      sclose(unixfd);
       if(ECONNREFUSED != error) {
         logmsg("Error binding socket, failed to connect to %s: (%d) %s",
                unix_socket, error, strerror(error));
         goto sws_cleanup;
       }
-      /* socket server is not alive, now check if it was actually a socket.
-       * Systems which have Unix sockets will also have lstat */
+      /* socket server is not alive, now check if it was actually a socket. */
+#ifdef WIN32
+      /* Windows does not have lstat function. */
+      rc = curlx_win32_stat(unix_socket, &statbuf);
+#else
       rc = lstat(unix_socket, &statbuf);
+#endif
       if(0 != rc) {
         logmsg("Error binding socket, failed to stat %s: (%d) %s",
                unix_socket, errno, strerror(errno));

@vszakats
Copy link
Member

vszakats commented Jun 2, 2021

Re: your revision. For clarity, if we enable this by default, let's not require USE_UNIX_SOCKETS from the build systems, and instead require something for disabling it.

@dmitrykos
Copy link
Contributor

@vszakats I confirm that it compiles ok for UWP platform.

@lixinwei715
Copy link
Contributor Author

lixinwei715 commented Jun 2, 2021

To be consistent with other platforms' build system, USE_UNIX_SOCKETS macro should still be handled by the build system.

And as far as I know, on build systems that use config-win32.h, USE_UNIX_SOCKETS is always enabled by default. No switch to disable it.

@vszakats
Copy link
Member

vszakats commented Jun 2, 2021

You're right that there was no way to disable it in config-win32.h. But, if we allow it to be disabled for CMake, for consistency within the Windows platform, it'd be best to allow the same for all Windows build systems. Hence my proposal for having it enabled without an extra step (USE_UNIX_SOCKETS) by default and allow CURL_DISABLE_UNIX_SOCKETS to disable it, in all of the Windows build systems.

CURL_DISABLE_UNIX_SOCKETS can be easily extended to all non-Windows platforms, which would make sense given that UnixSockets is enabled by default on every other platform too, when supported. (But most likely this feature is unnecessary for these platforms in practice.)

@lixinwei715
Copy link
Contributor Author

lixinwei715 commented Jun 3, 2021

If using cmake build system, users could set ENABLE_UNIX_SOCKETS to OFF ( by default it is ON ) to disable unix sockets.
If using autoconf build system, users could use --disable-unix-sockets option to disable unix sockets.
On Windows, only build systems that use config-win32.h (Makefile.vc and Makefile.m32) need CURL_DISABLE_UNIX_SOCKETS, so I add CURL_DISABLE_UNIX_SOCKETS to config-win32.h.

However, test 1165 failed:

test 1165...[Verify configure.ac and source code CURL_DISABLE_-sync]

perl -I.  returned 2, when expecting 0
 exit FAILED
== Contents of files in the log/ dir after test 1165
=== Start of file commands.log
 perl -I. ./disable-scan.pl ./.. >log/stdout1165 2>log/stderr1165
=== End of file commands.log
=== Start of file ftpserver.cmd
 Testnum 1165
=== End of file ftpserver.cmd
=== Start of file stdout1165
 Not set by configure: CURL_DISABLE_UNIX_SOCKETS (./../lib/config-win32.h)
 Used in code, not documented in CURL-DISABLE.md: CURL_DISABLE_UNIX_SOCKETS
=== End of file stdout1165

Update: this failure means I should rename CURL_DISABLE_UNIX_SOCKETS to other names, like CURL_WIN32_DISABLE_UNIX_SOCKETS.
Or I shouldn't simply add CURL_DISABLE_UNIX_SOCKETS, I need to provide options to enable or diable unix scokets for Makefile.vc and Makefile.m32 instead.

Update: After renaming to CURL_WIN32_DISABLE_UNIX_SOCKETS, test 1165 now passes. Providing new options for Makefile.vc and Makefile.m32 means a new feature, should be in a new PR. For this PR, I think adding CURL_WIN32_DISABLE_UNIX_SOCKETS temporarily is enough.

@lixinwei715 lixinwei715 requested a review from vszakats June 3, 2021 12:12
Copy link
Member

@vszakats vszakats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote against CURL_WIN32_DISABLE_UNIX_SOCKETS (it's Windows-specific, but even the WIN32-free name needs many more updates than I thought. Thanks for your test on this.), and I'd vote for moving defining USE_UNIX_SOCKETS to respective Makefile.m32 and Makefile.vc. (Then there are the many Windows builds inside projects/Windows which will likely need to be updated, too.)

@lixinwei715
Copy link
Contributor Author

lixinwei715 commented Jun 3, 2021

Sorry, I didn't realize projects/Windows also need to be updated.
Looks like whether adding CURL_DISABLE_UNIX_SOCKETS for all platforms, or making all windows build systems handle USE_UNIX_SOCKETS, both need a lot of work. I don't know if it is worth to do so.
However, the aim of this PR is to solve the cmake problem, not to solve the problem that no way to disable unix sockets in config-win32.h, the latter should be solved by a new PR.
So I think in this PR, we should still define USE_UNIX_SOCKETS in config-win32.h unconditionally.

Move the definition of sockaddr_un struct from config-win32.h to curl_setup.h, so
that it could be shared by all build systems.

Add ADDRESS_FAMILY typedef for old mingw, now old mingw can also use unix sockets.

Also fix the build of tests/server/sws.c on Win32 when USE_UNIX_SOCKETS is defined.
@@ -793,7 +793,11 @@ endif()
option(ENABLE_UNIX_SOCKETS "Define if you want Unix domain sockets support" ON)
if(ENABLE_UNIX_SOCKETS)
include(CheckStructHasMember)
check_struct_has_member("struct sockaddr_un" sun_path "sys/un.h" USE_UNIX_SOCKETS)
if(WIN32)
set(USE_UNIX_SOCKETS ON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes unix sockets is always available on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
With this PR, whether msvc or mingw, whether newer verison or older version, compilation will always pass when unix sockets feature is ON.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but do you know unix sockets to always be available on Windows? AFAIK it is only in the most recent builds of Windows.

/cc @vszakats

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for unix sockets, curl only requires the definition of sockaddr_un structure.
Although unix sockets support on Windows is introduced by Windows 10 SDK 17063. But actually, if we add the definition of sockaddr_un structure manually, it can be extended to all build toolchains on Windows.

See also: 23a870f

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Windows binary that supports UnixSockets can be built regardless of the toolchain or SDK version. As to whether UnixSockets works or not will/may depend on the actual Windows environment the binary is executed in. This patch brings nothing new here, only extends extisting support to CMake.

@lixinwei715
Copy link
Contributor Author

Could this PR be able to merge?

@bagder
Copy link
Member

bagder commented Jun 21, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

cmake: Unix Sockets support on Win32 platform is missing
5 participants