string-util: replace version_is_valid()/version_is_valid_version_spec() by a common...

Let's take inspiration from string_is_safe() and take a flags field that
allows fine tuning the validation.

Then port over all current users of either function to the new logic.
Note that this *does* change behaviour in various cases:

1. Generally: we'll now always accept the full UAPI.10 alphabet,
   including the "~" and "^" characters. As far as I can see there's no
   downside to this liberalization as none of the current consumers of
   the two functions uses these characters for anything else.

2. systemd-analyze compare-version will now accept version strings with
   "_" and "+" without complaining. I see no downside here, it just
   normalizes these debugging tools, to make them accept what most our
   other tools accept.

3. "bootctl link" will not accept empty version strings anymore
   Which is a bugfix I guess.

4. vpick will now refuse "_" and "+" in version strings. It kinda
   already did, because when parsing versions from filenames it uses "_"
   and "+" as name, architecture and attempt counter separators. We now
   systematically refuse it everywhere else in vpick too. This is hence
   a clean-up.

Fixes: #28906
Replaces: #42815 #41937

diff --git a/src/analyze/analyze-compare-versions.c b/src/analyze/analyze-compare-versions.c
index 5c15fd044d65a547a4c224214011787777754053..a3293d73b72fa1c44f2887f4b4b3a0bad1f0d8a6 100644 (file)
--- a/src/analyze/analyze-compare-versions.c
+++ b/src/analyze/analyze-compare-versions.c
@@ -17,9 +17,9 @@ int verb_compare_versions(int argc, char *argv[], uintptr_t _data, void *userdat
         /* We only output a warning on invalid version strings (instead of failing), since the comparison
          * functions try to handle invalid strings gracefully and it's still interesting to see what the
          * comparison result will be. */
-        if (!version_is_valid_versionspec(v1))
+        if (!version_is_valid(v1, VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS|VERSION_ALLOW_EMPTY))
                 log_warning("Version string 1 contains disallowed characters, they will be treated as separators: %s", v1);
-        if (!version_is_valid_versionspec(v2))
+        if (!version_is_valid(v2, VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS|VERSION_ALLOW_EMPTY))
                 log_warning("Version string 2 contains disallowed characters, they will be treated as separators: %s", v2);
 
         if (argc == 3) {

diff --git a/src/basic/string-util.c b/src/basic/string-util.c
index a806d2ddfa3f741d4a231a362f8bfa9046969268..8de801a74b4ef334957974195f36c10e4bf0b6ce 100644 (file)
--- a/src/basic/string-util.c
+++ b/src/basic/string-util.c
@@ -1462,29 +1462,47 @@ char* find_line_after_internal(const char *haystack, const char *needle) {
         return NULL;
 }
 
-bool version_is_valid(const char *s) {
-        if (isempty(s))
-                return false;
+bool version_is_valid(const char *s, VersionFlags flags) {
 
-        if (!filename_part_is_valid(s))
-                return false;
+        /* Validates a version string superficially. This does not proces the version string in any
+         * semantical way, it mostly just validates that its charset is reasonable. */
 
-        /* This is a superset of the characters used by semver. We additionally allow "_". */
-        if (!in_charset(s, ALPHANUMERICAL ".-+_"))
+        if (FLAGS_SET(flags, VERSION_ALLOW_EMPTY) ? !s : isempty(s))
                 return false;
 
-        return true;
-}
-
-bool version_is_valid_versionspec(const char *s) {
-
         if (!filename_part_is_valid(s))
                 return false;
 
-        if (!in_charset(s, ALPHANUMERICAL ".-~^"))
-                return false;
+        /* We always allow all characters specified by the UAPI.10 Version Specification, i.e. 0-9, a-z, A-Z,
+         * ".", "-", "~", "^".
+         *
+         * If the relevant flags are set we'll also allow "+" and "_" separators.
+         *
+         * Note that with SemVer allows 0-9, a-z, A-Z, "+", "-", ".", hence with VERSION_ALLOW_PLUS we
+         * implement a superset of it.
+         *
+         * If you wonder when to use which flags: when validating foreign versions (e.g. distribution
+         * versions in /etc/os-release or so) validate liberally, i.e. add
+         * VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS. When validating our own versioned objects (e.g. vpick
+         * or so) validate more strictly, and in particular refuse characters such as "_" and "+" that may be
+         * used for separating component names or boot attempt counters. Also: first – if appropriate – split
+         * the string into individual components. For example, if the string consists of a name and a
+         * version, separated by some character, only pass the version part to this function. The name part
+         * may pass verification, but it's cleaner to not rely on that.
+         *
+         * For details about UAPI.10 see:
+         *
+         * → https://uapi-group.org/specifications/specs/version_format_specification/ */
 
-        return true;
+        char charset[] = ALPHANUMERICAL ".-~^" /* plus room for the two chars below: */ "\0\0";
+        size_t l = strlen(charset);
+
+        if (FLAGS_SET(flags, VERSION_ALLOW_UNDERSCORE))
+                charset[l++] = '_';
+        if (FLAGS_SET(flags, VERSION_ALLOW_PLUS))
+                charset[l++] = '+';
+
+        return in_charset(s, charset);
 }
 
 ssize_t strlevenshtein(const char *x, const char *y) {

diff --git a/src/basic/string-util.h b/src/basic/string-util.h
index 8f4cc69a54b530625f94999676fe4c843f70644c..d0b614f775ab98a191626625c713e02ecda3743c 100644 (file)
--- a/src/basic/string-util.h
+++ b/src/basic/string-util.h
@@ -315,8 +315,13 @@ char* find_line_after_internal(const char *haystack, const char *needle);
 #define find_line_after(haystack, needle) \
         const_generic(haystack, find_line_after_internal(haystack, needle))
 
-bool version_is_valid(const char *s) _pure_;
-bool version_is_valid_versionspec(const char *s) _pure_;
+typedef enum VersionFlags {
+        VERSION_ALLOW_EMPTY      = 1 << 0,
+        VERSION_ALLOW_UNDERSCORE = 1 << 1, /* Allow "_" as separator (recommended separator) */
+        VERSION_ALLOW_PLUS       = 1 << 2, /* Allow "+" as separator (sometimes used as separator for boot attempt counters) */
+} VersionFlags;
+
+bool version_is_valid(const char *s, VersionFlags flags) _pure_;
 
 ssize_t strlevenshtein(const char *x, const char *y);
 

diff --git a/src/bootctl/bootctl-link.c b/src/bootctl/bootctl-link.c
index b4706670857ff0495741d9a6beae7dc58af6d67d..ed1753af99e51b73c5b2f970caaee70c718cec98 100644 (file)
--- a/src/bootctl/bootctl-link.c
+++ b/src/bootctl/bootctl-link.c
@@ -1492,7 +1492,7 @@ static int vl_link_prepare(sd_varlink *link, LinkParameters *p) {
         if (p->context.entry_title && !efi_loader_entry_title_valid(p->context.entry_title))
                 return sd_varlink_error_invalid_parameter_name(link, "entryTitle");
 
-        if (p->context.entry_version && !version_is_valid_versionspec(p->context.entry_version))
+        if (p->context.entry_version && !version_is_valid(p->context.entry_version, /* flags= */ 0))
                 return sd_varlink_error_invalid_parameter_name(link, "entryVersion");
 
         if (p->context.entry_commit != 0 && !entry_commit_valid(p->context.entry_commit))

diff --git a/src/bootctl/bootctl.c b/src/bootctl/bootctl.c
index 72d96bc1b0ea9ea8ccd039760fa197870e920027..238102095ed641e94c7de87e55977d7029d6affe 100644 (file)
--- a/src/bootctl/bootctl.c
+++ b/src/bootctl/bootctl.c
@@ -695,7 +695,7 @@ static int parse_argv(int argc, char *argv[], char ***ret_args) {
                                 break;
                         }
 
-                        if (!version_is_valid_versionspec(opts.arg))
+                        if (!version_is_valid(opts.arg, /* flags= */ 0))
                                 return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Not a valid boot menu entry version: %s", opts.arg);
 
                         r = free_and_strdup_warn(&arg_entry_version, opts.arg);

diff --git a/src/libsystemd/sd-json/json-util.c b/src/libsystemd/sd-json/json-util.c
index d69e37a3ce6826220a133df31c470d2cb5edc12c..6cbb61cb5e300e4de7ec2ac5548bffb3909eb37c 100644 (file)
--- a/src/libsystemd/sd-json/json-util.c
+++ b/src/libsystemd/sd-json/json-util.c
@@ -449,7 +449,7 @@ int json_dispatch_const_version(const char *name, sd_json_variant *variant, sd_j
                 return json_log(variant, flags, SYNTHETIC_ERRNO(EINVAL), "JSON field '%s' is not a string.", strna(name));
 
         const char *version = sd_json_variant_string(variant);
-        if (!version_is_valid(version))
+        if (!version_is_valid(version, FLAGS_SET(flags, SD_JSON_STRICT) ? 0 : VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS))
                 return json_log(variant, flags, SYNTHETIC_ERRNO(EINVAL), "JSON field '%s' is not a valid version string.", strna(name));
 
         *n = version;

diff --git a/src/shared/vpick.c b/src/shared/vpick.c
index c661f92fafc95d25628cee9f16baebbc7bc8dfff..4f709c9e19a4410756c48b10850871fb39352d47 100644 (file)
--- a/src/shared/vpick.c
+++ b/src/shared/vpick.c
@@ -447,7 +447,7 @@ static int make_choice(
                                 *underscore = 0;
                 }
 
-                if (!version_is_valid(e)) {
+                if (!version_is_valid(e, /* flags= */ 0)) {
                         log_debug("Version string '%s' of entry '%s' is invalid, ignoring entry.", e, (*entry)->d_name);
                         continue;
                 }

diff --git a/src/sysupdate/sysupdate-pattern.c b/src/sysupdate/sysupdate-pattern.c
index 459c8fda29d5118508e1aae8d91d2faef3e1d155..294499f4af8701f8c194da8ea5d9d7f9c2c947da 100644 (file)
--- a/src/sysupdate/sysupdate-pattern.c
+++ b/src/sysupdate/sysupdate-pattern.c
@@ -268,7 +268,7 @@ int pattern_match(const char *pattern, const char *s, InstanceMetadata *ret) {
                 switch (e->type) {
 
                 case PATTERN_VERSION:
-                        if (!version_is_valid(t)) {
+                        if (!version_is_valid(t, VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS)) {
                                 log_debug("Version string is not valid, refusing: %s", t);
                                 goto nope;
                         }

diff --git a/src/sysupdate/sysupdate-transfer.c b/src/sysupdate/sysupdate-transfer.c
index 30e879574f0289189b18ffc64ce0db46238defc8..e80dcaf39a02390a1208f499ab57b32c4b13630b 100644 (file)
--- a/src/sysupdate/sysupdate-transfer.c
+++ b/src/sysupdate/sysupdate-transfer.c
@@ -141,7 +141,7 @@ static int config_parse_protect_version(
                 return 0;
         }
 
-        if (!version_is_valid(resolved))  {
+        if (!version_is_valid(resolved, VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS))  {
                 log_syntax(unit, LOG_WARNING, filename, line, 0,
                            "ProtectVersion= string is not valid, ignoring: %s", resolved);
                 return 0;
@@ -180,7 +180,7 @@ static int config_parse_min_version(
                 return 0;
         }
 
-        if (!version_is_valid(rvalue)) {
+        if (!version_is_valid(resolved, VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS)) {
                 log_syntax(unit, LOG_WARNING, filename, line, 0,
                            "MinVersion= string is not valid, ignoring: %s", resolved);
                 return 0;

diff --git a/src/sysupdate/sysupdated.c b/src/sysupdate/sysupdated.c
index bc5a2439b5a3e939645e385cd4fee0caaa0dbf0e..9bbcc870bc903e17ab27c35c24d5e14b9bf9b8f4 100644 (file)
--- a/src/sysupdate/sysupdated.c
+++ b/src/sysupdate/sysupdated.c
@@ -952,7 +952,7 @@ static int target_method_describe(sd_bus_message *msg, void *userdata, sd_bus_er
         if (r < 0)
                 return r;
 
-        if (!version_is_valid(version))
+        if (!version_is_valid(version, VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS))
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid version");
 
         if ((flags & ~SD_SYSUPDATE_FLAGS_ALL) != 0)
@@ -1102,7 +1102,7 @@ static int target_method_acquire(sd_bus_message *msg, void *userdata, sd_bus_err
         if (isempty(version))
                 action = "org.freedesktop.sysupdate1.update";
         else {
-                if (!version_is_valid(version))
+                if (!version_is_valid(version, VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS))
                         return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid version");
 
                 action = "org.freedesktop.sysupdate1.update-to-version";
@@ -1190,7 +1190,7 @@ static int target_method_install(sd_bus_message *msg, void *userdata, sd_bus_err
         if (isempty(version))
                 action = "org.freedesktop.sysupdate1.update";
         else {
-                if (!version_is_valid(version))
+                if (!version_is_valid(version, VERSION_ALLOW_UNDERSCORE|VERSION_ALLOW_PLUS))
                         return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid version");
 
                 action = "org.freedesktop.sysupdate1.update-to-version";

diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c
index 970421c7abb261f4200c1a381a421844404c01b5..a18999cf8a56b88d9a0d732a027797b4377be3a4 100644 (file)
--- a/src/test/test-string-util.c
+++ b/src/test/test-string-util.c
@@ -1408,13 +1408,13 @@ TEST(strstrafter) {
 }
 
 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"));
+        assert_se(!version_is_valid(NULL, /* flags= */ 0));
+        assert_se(!version_is_valid("", /* flags= */ 0));
+        assert_se(version_is_valid("0", /* flags= */ 0));
+        assert_se(version_is_valid("5", /* flags= */ 0));
+        assert_se(version_is_valid("999999", /* flags= */ 0));
+        assert_se(version_is_valid("999999.5", /* flags= */ 0));
+        assert_se(version_is_valid("6.2.12-300.fc38.x86_64", /* flags= */ VERSION_ALLOW_UNDERSCORE));
 }
 
 TEST(strextendn) {

diff --git a/src/vpick/vpick-tool.c b/src/vpick/vpick-tool.c
index 85f2b5c1cbb379d546a81b6fcdd706a97dc746b4..8dd22e0ea28ae856f524b9467d0a6c76d18d8f60 100644 (file)
--- a/src/vpick/vpick-tool.c
+++ b/src/vpick/vpick-tool.c
@@ -115,7 +115,7 @@ static int parse_argv(int argc, char *argv[], char ***ret_args) {
                         break;
 
                 OPTION_SHORT('V', "VERSION", "Look for specified version"):
-                        if (!version_is_valid(opts.arg))
+                        if (!version_is_valid(opts.arg, /* flags= */ 0))
                                 return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid version string: %s", opts.arg);
 
                         r = free_and_strdup_warn(&arg_filter_version, opts.arg);