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

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

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) {

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))
+bool version_is_valid(const char *s, VersionFlags flags) {
+
+        /* 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. */
+
+        if (FLAGS_SET(flags, VERSION_ALLOW_EMPTY) ? !s : 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 "_". */
-        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);
 
-bool version_is_valid_versionspec(const char *s) {
+        if (FLAGS_SET(flags, VERSION_ALLOW_UNDERSCORE))
+                charset[l++] = '_';
+        if (FLAGS_SET(flags, VERSION_ALLOW_PLUS))
+                charset[l++] = '+';
 
-        if (!filename_part_is_valid(s))
-                return false;
-
-        if (!in_charset(s, ALPHANUMERICAL ".-~^"))
-                return false;
-
-        return true;
+        return in_charset(s, charset);
 }
 
 ssize_t strlevenshtein(const char *x, const char *y) {

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);
 

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))

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);

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;

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;
                 }

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;
                         }

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;

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";

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) {

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);