core: clean up Set/LoadCredential= parsers

Make logging consistent, plus introduce helper function
for adding creds to ExecContext.set_credential too.

6 files changed, 145 insertions, 158 deletions

diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c
index df5168caaf..c9b118a1a0 100644
--- a/src/core/dbus-execute.c
+++ b/src/core/dbus-execute.c
@@ -2061,43 +2061,14 @@ int bus_exec_context_set_transient_property(
                         isempty = false;
 
                         if (!UNIT_WRITE_FLAGS_NOOP(flags)) {
+                                bool encrypted = endswith(name, "Encrypted");
                                 _cleanup_free_ char *a = NULL, *b = NULL;
                                 _cleanup_free_ void *copy = NULL;
-                                ExecSetCredential *old;
 
                                 copy = memdup(p, sz);
                                 if (!copy)
                                         return -ENOMEM;
 
-                                old = hashmap_get(c->set_credentials, id);
-                                if (old) {
-                                        free_and_replace(old->data, copy);
-                                        old->size = sz;
-                                        old->encrypted = streq(name, "SetCredentialEncrypted");
-                                } else {
-                                        _cleanup_(exec_set_credential_freep) ExecSetCredential *sc = NULL;
-
-                                        sc = new(ExecSetCredential, 1);
-                                        if (!sc)
-                                                return -ENOMEM;
-
-                                        *sc = (ExecSetCredential) {
-                                                .id = strdup(id),
-                                                .data = TAKE_PTR(copy),
-                                                .size = sz,
-                                                .encrypted = streq(name, "SetCredentialEncrypted"),
-                                        };
-
-                                        if (!sc->id)
-                                                return -ENOMEM;
-
-                                        r = hashmap_ensure_put(&c->set_credentials, &exec_set_credential_hash_ops, sc->id, sc);
-                                        if (r < 0)
-                                                return r;
-
-                                        TAKE_PTR(sc);
-                                }
-
                                 a = specifier_escape(id);
                                 if (!a)
                                         return -ENOMEM;
@@ -2106,6 +2077,10 @@ int bus_exec_context_set_transient_property(
                                 if (!b)
                                         return -ENOMEM;
 
+                                r = exec_context_put_set_credential(c, id, TAKE_PTR(copy), sz, encrypted);
+                                if (r < 0)
+                                        return r;
+
                                 (void) unit_write_settingf(u, flags, name, "%s=%s:%s", name, a, b);
                         }
                 }
@@ -2146,9 +2121,9 @@ int bus_exec_context_set_transient_property(
                         isempty = false;
 
                         if (!UNIT_WRITE_FLAGS_NOOP(flags)) {
-                                bool encrypted = streq(name, "LoadCredentialEncrypted");
+                                bool encrypted = endswith(name, "Encrypted");
 
-                                r = hashmap_put_credential(&c->load_credentials, id, source, encrypted);
+                                r = exec_context_put_load_credential(c, id, source, encrypted);
                                 if (r < 0)
                                         return r;
 
diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c
index e46a76f0ac..b210850cb1 100644
--- a/src/core/exec-credential.c
+++ b/src/core/exec-credential.c
@@ -20,7 +20,7 @@
 #include "rm-rf.h"
 #include "tmpfile-util.h"
 
-ExecSetCredential *exec_set_credential_free(ExecSetCredential *sc) {
+ExecSetCredential* exec_set_credential_free(ExecSetCredential *sc) {
         if (!sc)
                 return NULL;
 
@@ -29,7 +29,7 @@ ExecSetCredential *exec_set_credential_free(ExecSetCredential *sc) {
         return mfree(sc);
 }
 
-ExecLoadCredential *exec_load_credential_free(ExecLoadCredential *lc) {
+ExecLoadCredential* exec_load_credential_free(ExecLoadCredential *lc) {
         if (!lc)
                 return NULL;
 
@@ -38,16 +38,108 @@ ExecLoadCredential *exec_load_credential_free(ExecLoadCredential *lc) {
         return mfree(lc);
 }
 
-DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR(
+DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(
         exec_set_credential_hash_ops,
         char, string_hash_func, string_compare_func,
         ExecSetCredential, exec_set_credential_free);
 
-DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR(
+DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(
         exec_load_credential_hash_ops,
         char, string_hash_func, string_compare_func,
         ExecLoadCredential, exec_load_credential_free);
 
+int exec_context_put_load_credential(ExecContext *c, const char *id, const char *path, bool encrypted) {
+        ExecLoadCredential *old;
+        int r;
+
+        assert(c);
+        assert(id);
+        assert(path);
+
+        old = hashmap_get(c->load_credentials, id);
+        if (old) {
+                r = free_and_strdup(&old->path, path);
+                if (r < 0)
+                        return r;
+
+                old->encrypted = encrypted;
+        } else {
+                _cleanup_(exec_load_credential_freep) ExecLoadCredential *lc = NULL;
+
+                lc = new(ExecLoadCredential, 1);
+                if (!lc)
+                        return -ENOMEM;
+
+                *lc = (ExecLoadCredential) {
+                        .id = strdup(id),
+                        .path = strdup(path),
+                        .encrypted = encrypted,
+                };
+                if (!lc->id || !lc->path)
+                        return -ENOMEM;
+
+                r = hashmap_ensure_put(&c->load_credentials, &exec_load_credential_hash_ops, lc->id, lc);
+                if (r < 0) {
+                        assert(r != -EEXIST);
+                        return r;
+                }
+
+                TAKE_PTR(lc);
+        }
+
+        return 0;
+}
+
+int exec_context_put_set_credential(
+                ExecContext *c,
+                const char *id,
+                void *data_consume,
+                size_t size,
+                bool encrypted) {
+
+        _cleanup_free_ void *data = data_consume;
+        ExecSetCredential *old;
+        int r;
+
+        /* Takes the ownership of data both on success and failure */
+
+        assert(c);
+        assert(id);
+        assert(data || size == 0);
+
+        old = hashmap_get(c->set_credentials, id);
+        if (old) {
+                free_and_replace(old->data, data);
+                old->size = size;
+                old->encrypted = encrypted;
+        } else {
+                _cleanup_(exec_set_credential_freep) ExecSetCredential *sc = NULL;
+
+                sc = new(ExecSetCredential, 1);
+                if (!sc)
+                        return -ENOMEM;
+
+                *sc = (ExecSetCredential) {
+                        .id = strdup(id),
+                        .data = TAKE_PTR(data),
+                        .size = size,
+                        .encrypted = encrypted,
+                };
+                if (!sc->id)
+                        return -ENOMEM;
+
+                r = hashmap_ensure_put(&c->set_credentials, &exec_set_credential_hash_ops, sc->id, sc);
+                if (r < 0) {
+                        assert(r != -EEXIST);
+                        return r;
+                }
+
+                TAKE_PTR(sc);
+        }
+
+        return 0;
+}
+
 bool exec_params_need_credentials(const ExecParameters *p) {
         assert(p);
 
diff --git a/src/core/exec-credential.h b/src/core/exec-credential.h
index 509c91e829..87b85d7709 100644
--- a/src/core/exec-credential.h
+++ b/src/core/exec-credential.h
@@ -12,7 +12,8 @@ typedef struct ExecParameters ExecParameters;
 
 /* A credential configured with LoadCredential= */
 typedef struct ExecLoadCredential {
-        char *id, *path;
+        char *id;
+        char *path;
         bool encrypted;
 } ExecLoadCredential;
 
@@ -24,14 +25,19 @@ typedef struct ExecSetCredential {
         size_t size;
 } ExecSetCredential;
 
-ExecSetCredential *exec_set_credential_free(ExecSetCredential *sc);
+ExecSetCredential* exec_set_credential_free(ExecSetCredential *sc);
 DEFINE_TRIVIAL_CLEANUP_FUNC(ExecSetCredential*, exec_set_credential_free);
 
-ExecLoadCredential *exec_load_credential_free(ExecLoadCredential *lc);
+ExecLoadCredential* exec_load_credential_free(ExecLoadCredential *lc);
 DEFINE_TRIVIAL_CLEANUP_FUNC(ExecLoadCredential*, exec_load_credential_free);
 
-extern const struct hash_ops exec_set_credential_hash_ops;
-extern const struct hash_ops exec_load_credential_hash_ops;
+int exec_context_put_load_credential(ExecContext *c, const char *id, const char *path, bool encrypted);
+int exec_context_put_set_credential(
+                ExecContext *c,
+                const char *id,
+                void *data_consume,
+                size_t size,
+                bool encrypted);
 
 bool exec_params_need_credentials(const ExecParameters *p);
 
diff --git a/src/core/execute-serialize.c b/src/core/execute-serialize.c
index c08d01af57..69f79984a5 100644
--- a/src/core/execute-serialize.c
+++ b/src/core/execute-serialize.c
@@ -3648,8 +3648,7 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) {
                         if (r < 0)
                                 return r;
                 } else if ((val = startswith(l, "exec-context-set-credentials="))) {
-                        _cleanup_(exec_set_credential_freep) ExecSetCredential *sc = NULL;
-                        _cleanup_free_ char *id = NULL, *encrypted = NULL, *data = NULL;
+                        _cleanup_free_ char *id = NULL, *data = NULL, *encrypted = NULL;
 
                         r = extract_many_words(&val, " ", EXTRACT_DONT_COALESCE_SEPARATORS, &id, &data, &encrypted);
                         if (r < 0)
@@ -3660,28 +3659,20 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) {
                         r = parse_boolean(encrypted);
                         if (r < 0)
                                 return r;
+                        bool e = r;
 
-                        sc = new(ExecSetCredential, 1);
-                        if (!sc)
-                                return -ENOMEM;
-
-                        *sc = (ExecSetCredential) {
-                                .id =  TAKE_PTR(id),
-                                .encrypted = r,
-                        };
+                        _cleanup_free_ void *d = NULL;
+                        size_t size;
 
-                        r = unbase64mem(data, &sc->data, &sc->size);
+                        r = unbase64mem_full(data, SIZE_MAX, /* secure = */ true, &d, &size);
                         if (r < 0)
                                 return r;
 
-                        r = hashmap_ensure_put(&c->set_credentials, &exec_set_credential_hash_ops, sc->id, sc);
+                        r = exec_context_put_set_credential(c, id, TAKE_PTR(d), size, e);
                         if (r < 0)
                                 return r;
-
-                        TAKE_PTR(sc);
                 } else if ((val = startswith(l, "exec-context-load-credentials="))) {
-                        _cleanup_(exec_load_credential_freep) ExecLoadCredential *lc = NULL;
-                        _cleanup_free_ char *id = NULL, *encrypted = NULL, *path = NULL;
+                        _cleanup_free_ char *id = NULL, *path = NULL, *encrypted = NULL;
 
                         r = extract_many_words(&val, " ", EXTRACT_DONT_COALESCE_SEPARATORS, &id, &path, &encrypted);
                         if (r < 0)
@@ -3693,21 +3684,9 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) {
                         if (r < 0)
                                 return r;
 
-                        lc = new(ExecLoadCredential, 1);
-                        if (!lc)
-                                return -ENOMEM;
-
-                        *lc = (ExecLoadCredential) {
-                                .id =  TAKE_PTR(id),
-                                .path = TAKE_PTR(path),
-                                .encrypted = r,
-                        };
-
-                        r = hashmap_ensure_put(&c->load_credentials, &exec_load_credential_hash_ops, lc->id, lc);
+                        r = exec_context_put_load_credential(c, id, path, r > 0);
                         if (r < 0)
                                 return r;
-
-                        TAKE_PTR(lc);
                 } else if ((val = startswith(l, "exec-context-import-credentials="))) {
                         r = set_ensure_allocated(&c->import_credentials, &string_hash_ops);
                         if (r < 0)
diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index debec0d57b..97b719f52a 100644
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -4737,18 +4737,13 @@ int config_parse_set_credential(
                 void *data,
                 void *userdata) {
 
-        _cleanup_free_ char *word = NULL, *k = NULL;
-        _cleanup_free_ void *d = NULL;
         ExecContext *context = ASSERT_PTR(data);
-        ExecSetCredential *old;
-        Unit *u = userdata;
-        bool encrypted = ltype;
-        const char *p = ASSERT_PTR(rvalue);
-        size_t size;
+        const Unit *u = ASSERT_PTR(userdata);
         int r;
 
         assert(filename);
         assert(lvalue);
+        assert(rvalue);
 
         if (isempty(rvalue)) {
                 /* Empty assignment resets the list */
@@ -4756,6 +4751,10 @@ int config_parse_set_credential(
                 return 0;
         }
 
+        _cleanup_free_ char *word = NULL, *id = NULL;
+        const char *p = rvalue;
+        bool encrypted = ltype;
+
         r = extract_first_word(&p, &word, ":", EXTRACT_DONT_COALESCE_SEPARATORS);
         if (r == -ENOMEM)
                 return log_oom();
@@ -4768,107 +4767,45 @@ int config_parse_set_credential(
                 return 0;
         }
 
-        r = unit_cred_printf(u, word, &k);
+        r = unit_cred_printf(u, word, &id);
         if (r < 0) {
                 log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to resolve unit specifiers in \"%s\", ignoring: %m", word);
                 return 0;
         }
-        if (!credential_name_valid(k)) {
-                log_syntax(unit, LOG_WARNING, filename, line, 0, "Credential name \"%s\" not valid, ignoring.", k);
+        if (!credential_name_valid(id)) {
+                log_syntax(unit, LOG_WARNING, filename, line, 0, "Credential name \"%s\" not valid, ignoring.", id);
                 return 0;
         }
 
+        _cleanup_free_ void *d = NULL;
+        size_t size;
+
         if (encrypted) {
-                r = unbase64mem_full(p, SIZE_MAX, true, &d, &size);
+                r = unbase64mem_full(p, SIZE_MAX, /* secure = */ true, &d, &size);
+                if (r == -ENOMEM)
+                        return log_oom();
                 if (r < 0) {
-                        log_syntax(unit, LOG_WARNING, filename, line, r, "Encrypted credential data not valid Base64 data, ignoring.");
+                        log_syntax(unit, LOG_WARNING, filename, line, r, "Encrypted credential data not valid Base64 data, ignoring: %m");
                         return 0;
                 }
         } else {
-                char *unescaped;
                 ssize_t l;
 
                 /* We support escape codes here, so that users can insert trailing \n if they like */
-                l = cunescape(p, UNESCAPE_ACCEPT_NUL, &unescaped);
+                l = cunescape(p, UNESCAPE_ACCEPT_NUL, (char**) &d);
+                if (l == -ENOMEM)
+                        return log_oom();
                 if (l < 0) {
                         log_syntax(unit, LOG_WARNING, filename, line, l, "Can't unescape \"%s\", ignoring: %m", p);
                         return 0;
                 }
 
-                d = unescaped;
                 size = l;
         }
 
-        old = hashmap_get(context->set_credentials, k);
-        if (old) {
-                free_and_replace(old->data, d);
-                old->size = size;
-                old->encrypted = encrypted;
-        } else {
-                _cleanup_(exec_set_credential_freep) ExecSetCredential *sc = NULL;
-
-                sc = new(ExecSetCredential, 1);
-                if (!sc)
-                        return log_oom();
-
-                *sc = (ExecSetCredential) {
-                        .id = TAKE_PTR(k),
-                        .data = TAKE_PTR(d),
-                        .size = size,
-                        .encrypted = encrypted,
-                };
-
-                r = hashmap_ensure_put(&context->set_credentials, &exec_set_credential_hash_ops, sc->id, sc);
-                if (r == -ENOMEM)
-                        return log_oom();
-                if (r < 0) {
-                        log_syntax(unit, LOG_WARNING, filename, line, r,
-                                   "Duplicated credential value '%s', ignoring assignment: %s", sc->id, rvalue);
-                        return 0;
-                }
-
-                TAKE_PTR(sc);
-        }
-
-        return 0;
-}
-
-int hashmap_put_credential(Hashmap **h, const char *id, const char *path, bool encrypted) {
-        ExecLoadCredential *old;
-        int r;
-
-        assert(h);
-        assert(id);
-        assert(path);
-
-        old = hashmap_get(*h, id);
-        if (old) {
-                r = free_and_strdup(&old->path, path);
-                if (r < 0)
-                        return r;
-
-                old->encrypted = encrypted;
-        } else {
-                _cleanup_(exec_load_credential_freep) ExecLoadCredential *lc = NULL;
-
-                lc = new(ExecLoadCredential, 1);
-                if (!lc)
-                        return log_oom();
-
-                *lc = (ExecLoadCredential) {
-                        .id = strdup(id),
-                        .path = strdup(path),
-                        .encrypted = encrypted,
-                };
-                if (!lc->id || !lc->path)
-                        return -ENOMEM;
-
-                r = hashmap_ensure_put(h, &exec_load_credential_hash_ops, lc->id, lc);
-                if (r < 0)
-                        return r;
-
-                TAKE_PTR(lc);
-        }
+        r = exec_context_put_set_credential(context, id, TAKE_PTR(d), size, encrypted);
+        if (r < 0)
+                return log_error_errno(r, "Failed to store set credential '%s': %m", rvalue);
 
         return 0;
 }
@@ -4939,7 +4876,7 @@ int config_parse_load_credential(
                 }
         }
 
-        r = hashmap_put_credential(&context->load_credentials, id, path, encrypted);
+        r = exec_context_put_load_credential(context, id, path, encrypted);
         if (r < 0)
                 return log_error_errno(r, "Failed to store load credential '%s': %m", rvalue);
 
diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h
index 94b13a4bb3..727ee4d791 100644
--- a/src/core/load-fragment.h
+++ b/src/core/load-fragment.h
@@ -12,8 +12,6 @@ int unit_is_likely_recursive_template_dependency(Unit *u, const char *name, cons
 int parse_crash_chvt(const char *value, int *data);
 int parse_confirm_spawn(const char *value, char **console);
 
-int hashmap_put_credential(Hashmap **h, const char *id, const char *path, bool encrypted);
-
 /* Read service data from .desktop file style configuration fragments */
 
 int unit_load_fragment(Unit *u);