[aquette]

Closes #676

Signed-off-by: Arnaud Quette ArnaudQuette@eaton.com

[zykh]

As previously mentioned, I still think that we should also make maxage, and friends, unsigned int.

[aquette]

@zykh fixed the both

[clepple]

LGTM

[zykh]

Just a couple of things:

• printf formats:
  

  nut/server/upsd.c

  Lines 699 to 702 in 3d79006

  	fatalx(EXIT_FAILURE,
  	"Your system limits the maximum number of connections to %d\n"
  	"but you requested %d. The server won't start until this\n"
  	"problem is resolved.\n", ret, maxconn);

• shouldn't the following be if (certrequest < NETSSL_CERTREQ_NO || certrequest > NETSSL_CERTREQ_REQUIRE) (or, given that now certrequest is unsigned and NETSSL_CERTREQ_NO is zero, just if (certrequest > NETSSL_CERTREQ_REQUIRE)) ?
  (i.e. || instead of &&, and NETSSL_CERTREQ_REQUIRE, which is 2, instead of NETSSL_CERTREQ_REQUEST, which is 1)
  

  nut/server/netssl.c

  Lines 481 to 485 in 3d79006

  	if (certrequest < NETSSL_CERTREQ_NO &&
  	certrequest > NETSSL_CERTREQ_REQUEST) {
  	upslogx(LOG_ERR, "Invalid certificate requirement");
  	return;
  	}

• how unlikely is that sysconf(_SC_OPEN_MAX) returns a) -1 or b) a value that (as long) exceeds what can be stored in an unsigned int?
  here:
  

  nut/server/upsd.c

  Line 696 in 3d79006

  ret = sysconf(_SC_OPEN_MAX);

  
  and here:
  

  nut/server/upsd.c

  Line 1251 in 3d79006

  maxconn = sysconf(_SC_OPEN_MAX);

[clepple]

In light of @zykh's comments, I think the change to unsigned is hasty. As pointed out, the sysconf() call shouldn't return a negative number, but a check is not expensive.

[jimklimov]

@aquette @clepple : a few merge conflicts cropped up over time :( Probably your versions should be trusted, but can you please revise that this is the case - and that master-branch changes so far did not change non-trivially some semantics etc.?

[jimklimov]

@user01010111 : wanna take a bite at another low-hanging fruit? It seems we have some trouble with 32-bit builds here, probably related to change of these data points into unsigned int... maybe easier to revert that one and keep range-checks in conf.c...

FAILURE [#1]    MAXAGE '2147483648': accepted=1, value=2147483648; expected 0, 17
FAILURE [#2]    load_upsdconf(0), MAXAGE '2147483648': value=2147483648; expected 17
FAILURE [#3]    load_upsdconf(1), MAXAGE '2147483648': value=2147483648; expected 17
FAILURE [#4]    TRACKINGDELAY '2147483648': accepted=1, value=2147483648; expected 0, 17
FAILURE [#5]    load_upsdconf(0), TRACKINGDELAY '2147483648': value=2147483648; expected 17
FAILURE [#6]    load_upsdconf(1), TRACKINGDELAY '2147483648': value=2147483648; expected 17

My recent commits on the branch fixed same issues for default (64-bit here) builds. Going offline now, so if you (or your AI) has a moment to comment here with a suggested fix, that would be welcome :)

• Repro (on Ubuntu): ./configure CFLAGS="-m32" CXXFLAGS=-m32 LDFLAGS=-m32 --build=x86_64-pc-linux-gnu --host=i686-pc-linux-gnu --without-docs (may also require apt install gcc-multilib).

[user01010111]

Suggested test-only follow-up for b79b8cb7e54f21401f78bfb01db7c8017c361919.

The six failures come from the test still treating LONG_MAX + 1 as invalid for every option. With 32-bit long, that value is 2147483648, which fits the new unsigned MAXAGE and TRACKINGDELAY types. The test now derives the expected result from each option's limit, preserving the unsigned types and the existing MAXCONN limit.

Enabling NSS client validation exposed another outdated part of the same test: its CERTREQUEST stub was still signed, and its expectations allowed values above the parser's new 0..2 limit. The patch aligns those test expectations with the current parser. It changes only tests/test_upsdconf.c.

I reproduced the six failures on the unchanged head, then ran the corrected test natively on i386 and x86-64: 263 checks passed on each. The NSS client-validation configuration passed 365 checks. Both default builds, all 13 tests in the x86-64 test directory, and the whitespace/non-ASCII checks passed.

Top-level make check and make distcheck-light initially stopped on the existing possessive-word spelling failures. Both now pass with spelling enabled after applying only the exact dictionary changes from pending #3629 (5131bdb6). That dependency is separate from this test patch. The distribution check completed archive build, tests, staged install, uninstall and cleanup; the archived test and dictionary match the validated source. Documentation rendering was skipped. These full-gate results therefore depend on integrating the #3629 dictionary changes.

AI assistance: OpenAI Codex, using gpt-5.6-sol for initial scoping and gpt-6-astra for review, implementation and validation. I have reviewed this change and accept responsibility for contributing it under the project's licence.

Patch against b79b8cb

--- a/tests/test_upsdconf.c
+++ b/tests/test_upsdconf.c
@@ -24,7 +24,7 @@
 char *certfile = NULL, *certpath = NULL, *certname = NULL, *certpasswd = NULL;
 int disable_weak_ssl = 0;
 #ifdef WITH_CLIENT_CERTIFICATE_VALIDATION
-int certrequest = 0;
+unsigned int certrequest = 0;
 #endif
 
 /* These daemon operations must not be reached by the configuration tests. */
@@ -185,23 +185,25 @@
 		sizeof(int), sizeof(long), sizeof(nfds_t));
 	for (i = 0; i < sizeof(options) / sizeof(options[0]); i++) {
 		previous = !strcmp(options[i], "CERTREQUEST") ? 2 : 17;
-		check_value(options[i], "0", 1, 0);
-		check_value(options[i], "1", 1, 1);
-		check_value(options[i], "2", 1, 2);
-		check_value(options[i], "00015", 1, 15);
-		check_value(options[i], "32 \t", 1, 32);
-		check_value(options[i], "3600", 1, 3600);
-		for (j = 0; j < sizeof(invalid) / sizeof(invalid[0]); j++)
-			check_value(options[i], invalid[j], 0, previous);
-
-		limit = (uintmax_t)INT_MAX;
+		limit = (uintmax_t)NETSSL_CERTREQ_MAX;
 		if (!strcmp(options[i], "MAXAGE") || !strcmp(options[i], "TRACKINGDELAY")) {
 			limit = (uintmax_t)UINT_MAX;
 		} else
 		if (!strcmp(options[i], "MAXCONN")) {
 			limit = (uintmax_t)((nfds_t)-1);
-			if (limit > (uintmax_t)LONG_MAX) limit = (uintmax_t)LONG_MAX;
+			if (limit > (uintmax_t)LONG_MAX) {
+				limit = (uintmax_t)LONG_MAX;
+			}
 		}
+		check_value(options[i], "0", 1, 0);
+		check_value(options[i], "1", 1, 1);
+		check_value(options[i], "2", 1, 2);
+		check_value(options[i], "00015", limit >= 15, limit >= 15 ? 15 : previous);
+		check_value(options[i], "32 \t", limit >= 32, limit >= 32 ? 32 : previous);
+		check_value(options[i], "3600", limit >= 3600, limit >= 3600 ? 3600 : previous);
+		for (j = 0; j < sizeof(invalid) / sizeof(invalid[0]); j++)
+			check_value(options[i], invalid[j], 0, previous);
+
 		snprintf(number, sizeof(number), "%" PRIuMAX, limit - 1);
 		check_value(options[i], number, 1, limit - 1);
 		snprintf(number, sizeof(number), "%" PRIuMAX, limit);
@@ -209,15 +211,19 @@
 		snprintf(number, sizeof(number), "%" PRIuMAX, limit + 1);
 		check_value(options[i], number, 0, previous);
 		snprintf(number, sizeof(number), "%" PRIuMAX, (uintmax_t)LONG_MAX + 1);
-		check_value(options[i], number, 0, previous);
+		if ((uintmax_t)LONG_MAX < limit) {
+			check_value(options[i], number, 1, (uintmax_t)LONG_MAX + 1);
+		} else {
+			check_value(options[i], number, 0, previous);
+		}
 	}
 #if defined(WITH_SSL) && defined(WITH_CLIENT_CERTIFICATE_VALIDATION)
 	check_value("CERTREQUEST", "NO", 1, 0);
 	check_value("CERTREQUEST", "REQUEST", 1, 1);
 	check_value("CERTREQUEST", "REQUIRE", 1, 2);
 	check_value("CERTREQUEST", "require", 0, 2);
-	/* ssl_init still owns the 0..2 domain check and its fatal error policy. */
-	check_value("CERTREQUEST", "3", 1, 3);
+	/* Configuration parsing owns the 0..2 domain check. */
+	check_value("CERTREQUEST", "3", 0, 2);
 #endif
 
 	f = fopen(config_file, "w");

Signed-off-by: user01010111 lapses.50.booster@icloud.com

[jimklimov]

@user01010111 : thanks for the research and confirmation! So after all it seems quite similar to what I did to this test program for #3640. I'll model the fix here on that code, after merging that PR then.

[user01010111]

No worries, @jimklimov .. Nice work as usual! 🫡

[AppVeyorBot]

❌ Build nut 2.8.5.5913-master failed (commit cca94791b0 by @jimklimov)

• artifacts (if any)

[AppVeyorBot]

❌ Build nut 2.8.5.5913-master failed (commit cca94791b0 by @jimklimov)

[AppVeyorBot]

✅ Build nut 2.8.5.5919-master completed (commit cca94791b0 by @jimklimov)

• artifacts
• A 7-zip archive with built NUT for Windows debug binaries is temporarily available: NUT-for-Windows-x86_64-SNAPSHOT-2.8.5.5919-master.7z
• ccache: 393 of 410 compilations were cache hits (95.9%); cache size 748 of 750 MiB after cleanup