Implement is_ip4() and is_ip6() to avoid network dependencies

[rockdaboot]

Can someone please review, especially try compiling on Windows?

Fixes #226

[eli-schwartz]

@rockdaboot I tested it in the meson CI matrix:

• Your patch compiles fine with Visual Studio: https://github.com/mesonbuild/wrapdb/actions/runs/7599023779/job/20695630342
• Your patch + removing ws2_32 in meson.build as well, FAILS to compile: https://github.com/mesonbuild/wrapdb/actions/runs/7599032955/job/20695724242 "unresolved external symbol __imp_WSAStartup referenced in function main"
• With Msys2 + clang, it fails due to GetACP being undefined.

There are some additional problems in these logs, which are unelated to your change (i.e. problems with stock 0.21.5). @neheb probably should look into that, having expressed interest before:

• With clang-cl: test-is-cookie-domain-acceptable.c fails to compile, alloca() is not made available
• With Msys2 and either gcc or clang it compiles, some tests fail

[neheb]

Why avoid network dependencies?

[rockdaboot]

Why avoid network dependencies?

Why having network dependencies? They may be annoying if not really needed by an application.
See also curl/curl-for-win#63 (comment)

[rockdaboot]

Thanks for testing @eli-schwartz

With Msys2 + clang, it fails due to GetACP being undefined.

That has been my fear. GetACP() requires Windows.h (see https://learn.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getacp).

"unresolved external symbol __imp_WSAStartup referenced in function main"

Maybe we can get rid of that. Not sure if GetACP() needs it.

With clang-cl: test-is-cookie-domain-acceptable.c fails to compile, alloca() is not made available

I can fix this.

[vszakats]

With Msys2 + clang, it fails due to GetACP being undefined.

That has been my fear. GetACP() requires Windows.h (see https://learn.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getacp).

"unresolved external symbol __imp_WSAStartup referenced in function main"

Maybe we can get rid of that. Not sure if GetACP() needs it.

GetACP() doesn't need it: https://learn.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getacp

windows.h is only required with libidn2 or libidn, of which libidn2 already
requires it, so it's okay. Haven't checked libidn. Either way it's safe to
assume that windows.h is available when _WIN32 is set by the compiler.

Here's a patch that may help (I've only given it a limited build test):

diff --git a/src/psl.c b/src/psl.c
index 0168a68..9fdef85 100644
--- a/src/psl.c
+++ b/src/psl.c
@@ -47,6 +47,13 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 
+#if defined(_WIN32) && (defined(WITH_LIBIDN2) || defined(WITH_LIBIDN))
+# ifndef WIN32_LEAN_AND_MEAN
+# define WIN32_LEAN_AND_MEAN
+# endif
+# include <windows.h> /* for GetACP() */
+#endif
+
 #if defined(_MSC_VER) && ! defined(ssize_t)
 # include <basetsd.h>
 typedef SSIZE_T ssize_t;
diff --git a/tests/test-is-cookie-domain-acceptable.c b/tests/test-is-cookie-domain-acceptable.c
index d58bd47..17b46c6 100644
--- a/tests/test-is-cookie-domain-acceptable.c
+++ b/tests/test-is-cookie-domain-acceptable.c
@@ -32,10 +32,6 @@
 # include <config.h>
 #endif
 
-#ifdef _WIN32
-# include <winsock2.h> // WSAStartup, WSACleanup
-#endif
-
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -113,18 +109,6 @@ static void test_psl(void)
 
 int main(int argc, const char * const *argv)
 {
-#ifdef _WIN32
-	WSADATA wsa_data;
-	int err;
-
-	if ((err = WSAStartup(MAKEWORD(2,2), &wsa_data))) {
-		printf("WSAStartup failed with error: %d\n", err);
-		return 1;
-	}
-
-	atexit((void (__cdecl*)(void)) WSACleanup);
-#endif
-
 	/* if VALGRIND testing is enabled, we have to call ourselves with valgrind checking */
 	if (argc == 1) {
 		const char *valgrind = getenv("TESTS_VALGRIND");
diff --git a/tools/psl.c b/tools/psl.c
index 527d780..05b2560 100644
--- a/tools/psl.c
+++ b/tools/psl.c
@@ -37,10 +37,9 @@
 #endif
 
 #ifdef _WIN32
-# include <winsock2.h> // WSAStartup, WSACleanup
-
 // Windows does not have localtime_r but has localtime_s, which is more or less
 // the same except that the arguments are reversed
+# include <time.h>
 # define LOCALTIME_R_SUCCESSFUL(t_sec,t_now)	\
 	(localtime_s(t_now, t_sec) == 0)
 #else
@@ -78,20 +77,6 @@ static void usage(int err, FILE* f)
 	exit(err);
 }
 
-static void init_windows(void) {
-#ifdef _WIN32
-	WSADATA wsa_data;
-	int err;
-
-	if ((err = WSAStartup(MAKEWORD(2,2), &wsa_data))) {
-		printf("WSAStartup failed with error: %d\n", err);
-		exit(EXIT_FAILURE);
-	}
-
-	atexit((void (__cdecl*)(void)) WSACleanup);
-#endif
-}
-
 /* RFC 2822-compliant date format */
 static const char *time2str(time_t t)
 {
@@ -238,8 +223,6 @@ int main(int argc, const char *const *argv)
 				else if (mode == 4) {
 					char *cookie_domain_lower;
 
-					init_windows();
-
 					if ((rc = psl_str_to_utf8lower(domain, NULL, NULL, &cookie_domain_lower)) == PSL_SUCCESS) {
 						if (!batch_mode)
 							printf("%s: ", domain);
@@ -284,8 +267,6 @@ int main(int argc, const char *const *argv)
 		}
 	}
 	else if (mode == 4) {
-		init_windows();
-
 		for (; arg < argv + argc; arg++) {
 			if (!batch_mode)
 				printf("%s: ", *arg);

[rockdaboot]

Thank you @vszakats !

@eli-schwartz Pushed the removal of WSA code. When #231 is merged, I'll rebase this PR and ping you for another round of tests.

[rockdaboot]

@vszakats or someone else, could you test again building libpsl on Windows / Msys2 and running the tests? If tests fail, it would be interesting to see the log output from the failing tests.

[eli-schwartz]

It works with msys2 and GCC, but fails with msys2 and clang:

3/8 test-is-cookie-domain-acceptable FAIL            0.03s   exit status 1
>>> ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MALLOC_PERTURB_=90 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 PATH=D:/a/_temp/msys64/clang64/bin;D:/a/wrapdb/wrapdb/_build/subprojects/libpsl-0.21.5/src;D:\a\_temp\msys64\clang64\bin;D:\a\_temp\msys64\usr\local\bin;D:\a\_temp\msys64\usr\bin;D:\a\_temp\msys64\usr\bin;C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;D:\a\_temp\msys64\usr\bin\site_perl;D:\a\_temp\msys64\usr\bin\vendor_perl;D:\a\_temp\msys64\usr\bin\core_perl D:/a/wrapdb/wrapdb/_build/subprojects/libpsl-0.21.5/tests/test-is-cookie-domain-acceptable.exe
------------------------------------- 8< -------------------------------------
loaded 9556 suffixes and 8 exceptions

psl_is_cookie_domain_acceptable(192.1.123.2, .1.123.2)=1 (expected 0)

psl_is_cookie_domain_acceptable(::ffff:192.1.123.2, .1.123.2)=1 (expected 0)

Summary: 2 out of 23 tests failed

[...]

Summary of Failures:

3/8 test-is-cookie-domain-acceptable FAIL            0.03s   exit status 1

Ok:                 4   
Expected Fail:      0   
Fail:               1   
Unexpected Pass:    0   
Skipped:            3   
Timeout:            0   

[rockdaboot]

@eli-schwartz Just to make sure, without the new is_ip functions, does the test succeed?

Because if the first parameter to psl_is_cookie_domain_acceptable() is an IP address, the function returns 0. Getting a 1 here means, that check failed. And the is_ip checks are pure string/memory operations, I don't see anything there that isn't portable.

[eli-schwartz]

It's being run against the 0.21.5 release with:

• libpsl-no-alloca.patch (backport of Avoid alloca in libpsl and tests #231) applied
  • which passes here: https://github.com/mesonbuild/wrapdb/actions/runs/7859202656/job/21445032024
• libpsl-winipv4.patch, libpsl-winipv4-meson.patch applied, the first being this PR, the second being my adaptation of the meson files
  • which fails here: https://github.com/mesonbuild/wrapdb/actions/runs/7863678837/job/21454561767

[vszakats]

I don't have an MSYS2 environment. Tried cross-building with autotools + llvm-mingw (v20231128) for x64 instead, which is close enough. Source was fe13dad + https://github.com/mesonbuild/wrapdb/blob/libpsl/subprojects/packagefiles/libpsl-winipv4.patch.

The build first failed with:

../../src/psl.c:320:14: error: static declaration of 'strdup' follows non-static declaration
  320 | static char *strdup(const char *s)
      |              ^
[...]/llvm-mingw/x86_64-w64-mingw32/include/string.h:108:17: note: previous declaration is here
  108 |   char *__cdecl strdup(const char *_Src) __MINGW_ATTRIB_DEPRECATED_MSVC2005;
      |                 ^
1 error generated.

Fixed it with this patch:

--- a/src/psl.c
+++ b/src/psl.c
@@ -320,22 +334,13 @@ static int suffix_init(psl_entry_t *suffix, const char *rule, size_t length)
 	return 0;
 }
 
-#ifndef HAVE_STRDUP
-static char *strdup(const char *s)
+static char *my_strdup(const char *s)
 {
 	char *p = malloc(strlen(s) + 1);
 	if (!p)
 		return NULL;
 	return strcpy(p, s);
 }
-#elif !HAVE_DECL_STRDUP
-/*
- *  On Linux with
- *    CC=gcc CFLAGS="-Wall -Wextra -Wpedantic -std=c89" ./configure
- *  strdup isn't declared (warning: implicit declaration of function 'strdup').
- */
-char *strdup(const char *);
-#endif
 
 #if !defined(WITH_LIBIDN) && !defined(WITH_LIBIDN2) && !defined(WITH_LIBICU)
 /*
@@ -733,7 +738,7 @@ static int psl_idna_toASCII(psl_idna_t *idna, const char *utf8, char **ascii)
 
 			lookupname[bytes_written] = 0; /* u_strToUTF8() doesn't 0-terminate if dest is filled up */
 		} else {
-			if (!(lookupname = strdup(lookupname)))
+			if (!(lookupname = my_strdup(lookupname)))
 				goto cleanup;
 		}
 
@@ -808,7 +813,7 @@ cleanup:
 
 	if (domain_to_punycode(utf8, lookupname, sizeof(lookupname)) == 0) {
 		if (ascii)
-			if ((*ascii = strdup(lookupname)))
+			if ((*ascii = my_strdup(lookupname)))
 				ret = 0;
 	}
 #endif
@@ -1758,7 +1763,7 @@ psl_error_t psl_str_to_utf8lower(const char *str, const char *encoding, const ch
 		if (lower) {
 			char *p, *tmp;
 
-			if (!(tmp = strdup(str)))
+			if (!(tmp = my_strdup(str)))
 				return PSL_ERR_NO_MEM;
 
 			*lower = tmp;
@@ -1816,7 +1821,7 @@ psl_error_t psl_str_to_utf8lower(const char *str, const char *encoding, const ch
 				if (U_SUCCESS(status)) {
 					ret = PSL_SUCCESS;
 					if (lower) {
-						char *tmp = strdup(utf8_lower);
+						char *tmp = my_strdup(utf8_lower);
 
 						if (tmp)
 							*lower = tmp;

Then it failed to build tests when running make check: libpsl.h header was not found. Tried to disable fuzz tests with --disable-fuzz, but it did not disable building fuzz tests. Fixed by copying the header to the test directories manually. Then WinMain was missing at link time, with libtool warnings about incompatible options (e.g. -no-install).

Patched to skip fuzz tests:

--- a/Makefile.am
+++ b/Makefile.am
@@ -8,7 +8,7 @@ if ENABLE_MAN
   SUBDIRS += docs/libpsl
 endif
 endif
-SUBDIRS += fuzz tests msvc
+SUBDIRS += tests msvc
 
 ACLOCAL_AMFLAGS = -I m4 ${ACLOCAL_FLAGS}

With this, tests built correctly, but they failed to execute due to the cross-build scenario.
Fixed this by running tests manually, and in my case, it run without issues:

$ file test-is-cookie-domain-acceptable.exe 
test-is-cookie-domain-acceptable.exe: PE32+ executable (console) x86-64, for MS Windows

$ wine64 test-is-cookie-domain-acceptable.exe 
loaded 9556 suffixes and 8 exceptions
Summary: All 23 tests passed

Rest of tests run OK too, except test-registrable-domain.exe which is missing ../../list/tests/tests.txt.

I repeated the cross-build with stock llvm/clang (17.0.6) + mingw-w64 (11.0.1) both via Homebrew and the tests passed too:

$ file test-is-cookie-domain-acceptable.exe 
test-is-cookie-domain-acceptable.exe: PE32+ executable (console) x86-64, for MS Windows

$ wine64 test-is-cookie-domain-acceptable.exe 
loaded 9556 suffixes and 8 exceptions
Summary: All 23 tests passed

The MSYS2 tests passing with UCRT64 but failing with CLANG64 seems unrelated to the winsock2 removal. (Sadly I have no idea what else may be the problem there.)

[rockdaboot]

The reason for the fuzz tests not building is likely a missing DLL (need to be added to LDADD in fuzz/Makefile.am), but can't test that anyway. With MinGW on Linux, it's not a problem (fuzz tests are SKIPped). Added contrib/mingw.static to cross-build the library, tool and tests.