Merge pull request #28095 from poettering/generic-valid-version

make version_is_valid() generic

src/analyze/analyze-compare-versions.c

@@ -9,17 +9,26 @@
 #include "strv.h"
 
 int verb_compare_versions(int argc, char *argv[], void *userdata) {
+        const char *v1 = ASSERT_PTR(argv[1]), *v2 = ASSERT_PTR(argv[argc-1]);
         int r;
 
         assert(IN_SET(argc, 3, 4));
         assert(argv);
 
+        /* We only output a warning on invalid version strings (instead of failing), since the comparison
+         * functions try to handle invalid strings graceful and it's still interesting to see what the
+         * comparison result will be. */
+        if (!version_is_valid(v1))
+                log_warning("Version string 1 is not valid, comparing anyway: %s", v1);
+        if (!version_is_valid(v2))
+                log_warning("Version string 2 is not valid, comparing anyway: %s", v2);
+
         if (argc == 3) {
-                r = strverscmp_improved(ASSERT_PTR(argv[1]), ASSERT_PTR(argv[2]));
+                r = strverscmp_improved(v1, v2);
                 printf("%s %s %s\n",
-                       isempty(argv[1]) ? "''" : argv[1],
+                       isempty(v1) ? "''" : v1,
                        comparison_operator(r),
-                       isempty(argv[2]) ? "''" : argv[2]);
+                       isempty(v2) ? "''" : v2);
 
                 /* This matches the exit convention used by rpmdev-vercmp.
                  * We don't use named values because 11 and 12 don't have names. */
@@ -28,12 +37,13 @@ int verb_compare_versions(int argc, char *argv[], void *userdata) {
         } else {
                 const char *op = ASSERT_PTR(argv[2]);
                 CompareOperator operator;
+                assert(argc == 4);
 
                 operator = parse_compare_operator(&op, COMPARE_ALLOW_TEXTUAL);
                 if (operator < 0 || !isempty(op))
                         return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Unknown operator \"%s\".", op);
 
-                r = version_or_fnmatch_compare(operator, ASSERT_PTR(argv[1]), ASSERT_PTR(argv[3]));
+                r = version_or_fnmatch_compare(operator, v1, v2);
                 if (r < 0)
                         return log_error_errno(r, "Failed to compare versions: %m");
 

src/basic/path-util.c

@@ -1124,13 +1124,13 @@ int path_extract_directory(const char *path, char **ret) {
         return 0;
 }
 
-bool filename_is_valid(const char *p) {
+bool filename_part_is_valid(const char *p) {
         const char *e;
 
-        if (isempty(p))
-                return false;
+        /* Checks f the specified string is OK to be *part* of a filename. This is different from
+         * filename_is_valid() as "." and ".." and "" are OK by this call, but not by filename_is_valid(). */
 
-        if (dot_or_dot_dot(p)) /* Yes, in this context we consider "." and ".." invalid */
+        if (!p)
                 return false;
 
         e = strchrnul(p, '/');
@@ -1143,6 +1143,17 @@ bool filename_is_valid(const char *p) {
         return true;
 }
 
+bool filename_is_valid(const char *p) {
+
+        if (isempty(p))
+                return false;
+
+        if (dot_or_dot_dot(p)) /* Yes, in this context we consider "." and ".." invalid */
+                return false;
+
+        return filename_part_is_valid(p);
+}
+
 bool path_is_valid_full(const char *p, bool accept_dot_dot) {
         if (isempty(p))
                 return false;

src/basic/path-util.h

@@ -163,6 +163,7 @@ const char *last_path_component(const char *path);
 int path_extract_filename(const char *path, char **ret);
 int path_extract_directory(const char *path, char **ret);
 
+bool filename_part_is_valid(const char *p) _pure_;
 bool filename_is_valid(const char *p) _pure_;
 bool path_is_valid_full(const char *p, bool accept_dot_dot) _pure_;
 static inline bool path_is_valid(const char *p) {

src/basic/string-util.c

@@ -16,6 +16,7 @@
 #include "macro.h"
 #include "memory-util.h"
 #include "memstream-util.h"
+#include "path-util.h"
 #include "string-util.h"
 #include "strv.h"
 #include "terminal-util.h"
@@ -1296,3 +1297,17 @@ char *startswith_strv(const char *string, char **strv) {
 
         return found;
 }
+
+bool version_is_valid(const char *s) {
+        if (isempty(s))
+                return false;
+
+        if (!filename_part_is_valid(s))
+                return false;
+
+        /* This is a superset of the characters used by semver. We additionally allow "," and "_". */
+        if (!in_charset(s, ALPHANUMERICAL ".,_-+"))
+                return false;
+
+        return true;
+}

src/basic/string-util.h

@@ -275,3 +275,5 @@ char *startswith_strv(const char *string, char **strv);
 
 #define STARTSWITH_SET(p, ...)                                  \
         startswith_strv(p, STRV_MAKE(__VA_ARGS__))
+
+bool version_is_valid(const char *s);

src/sysupdate/meson.build

@@ -15,8 +15,6 @@ systemd_sysupdate_sources = files(
         'sysupdate-transfer.h',
         'sysupdate-update-set.c',
         'sysupdate-update-set.h',
-        'sysupdate-util.c',
-        'sysupdate-util.h',
         'sysupdate.c',
         'sysupdate.h',
 )

src/sysupdate/sysupdate-pattern.c

@@ -8,7 +8,6 @@
 #include "stdio-util.h"
 #include "string-util.h"
 #include "sysupdate-pattern.h"
-#include "sysupdate-util.h"
 
 typedef enum PatternElementType {
         PATTERN_LITERAL,

src/sysupdate/sysupdate-transfer.c

@@ -24,7 +24,6 @@
 #include "sysupdate-pattern.h"
 #include "sysupdate-resource.h"
 #include "sysupdate-transfer.h"
-#include "sysupdate-util.h"
 #include "sysupdate.h"
 #include "tmpfile-util.h"
 #include "web-util.h"

src/sysupdate/sysupdate-util.c

@@ -2,16 +2,3 @@
 
 #include "path-util.h"
 #include "sysupdate-util.h"
-
-bool version_is_valid(const char *s) {
-        if (isempty(s))
-                return false;
-
-        if (!filename_is_valid(s))
-                return false;
-
-        if (!in_charset(s, ALPHANUMERICAL ".,_-+"))
-                return false;
-
-        return true;
-}

src/sysupdate/sysupdate-util.h

@@ -1,6 +0,0 @@
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-#pragma once
-
-#include <stdbool.h>
-
-bool version_is_valid(const char *s);

src/test/test-string-util.c

@@ -1264,4 +1264,14 @@ TEST(strstrafter) {
         assert_se(!strstrafter(buffer, "-"));
 }
 
+TEST(version_is_valid) {
+        assert_se(!version_is_valid(NULL));
+        assert_se(!version_is_valid(""));
+        assert_se(version_is_valid("0"));
+        assert_se(version_is_valid("5"));
+        assert_se(version_is_valid("999999"));
+        assert_se(version_is_valid("999999.5"));
+        assert_se(version_is_valid("6.2.12-300.fc38.x86_64"));
+}
+
 DEFINE_TEST_MAIN(LOG_DEBUG);