Hi,
me again with another thing about acme-client. This time I run into an
issue with how it determines certdir. It uses

        if ((certdir = dirname(domain->cert)) != NULL) {
                if ((certdir = strdup(certdir)) == NULL)

which is wrong according to POSIX spec. Citing from
http://pubs.opengroup.org/onlinepubs/9699919799/ :

        The dirname() function may modify the string pointed to by path, and may
        return a pointer to internal storage.

On linux (at least on Alpine with musl-c), it actually does modify the
argument. That means that it changes `domain->cert', which introduces
issues later on.

I assume this currently is not an issue on openbsd since it probably
takes the second route (`... a pointer to internal storage.').

This patch fixes the issue for me:

diff --git a/main.c b/main.c
index fb895f1..d682658 100644
--- a/main.c
+++ b/main.c
@@ -37,7 +37,7 @@ int
 main(int argc, char *argv[])
 {
        const char       **alts = NULL;
-       char             *certdir = NULL, *certfile = NULL;
+       char             *certdir = NULL, *certdir_src = NULL, *certfile = NULL;
        char             *chainfile = NULL, *fullchainfile = NULL;
        char             *acctkey = NULL;
        char             *chngdir = NULL, *auth = NULL;
@@ -104,20 +104,13 @@ main(int argc, char *argv[])
        argc--;
        argv++;

-       if (domain->cert != NULL) {
-               if ((certdir = dirname(domain->cert)) != NULL) {
-                       if ((certdir = strdup(certdir)) == NULL)
-                               err(EXIT_FAILURE, "strdup");
-               } else
-                       err(EXIT_FAILURE, "dirname");
-       } else {
-               /* the parser enforces that at least cert or fullchain is set */
-               if ((certdir = dirname(domain->fullchain)) != NULL) {
-                       if ((certdir = strdup(certdir)) == NULL)
-                               err(EXIT_FAILURE, "strdup");
-               } else
-                       err(EXIT_FAILURE, "dirname");
-
+       /* the parser enforces that at least cert or fullchain is set */
+       certdir_src = domain->cert ? domain->cert : domain->fullchain;
+       if ((certdir_src = strdup(certdir_src)) == NULL) {
+               err(EXIT_FAILURE, "strdup");
+       }
+       if ((certdir = dirname(certdir_src)) == NULL) {
+               err(EXIT_FAILURE, "dirname");
        }

        if (domain->cert != NULL) {
@@ -415,6 +408,7 @@ main(int argc, char *argv[])
            checkexit(pids[COMP_DNS], COMP_DNS) +
            checkexit(pids[COMP_REVOKE], COMP_REVOKE);

+       free(certdir_src);
        free(alts);
        return rc != COMP__MAX ? EXIT_FAILURE : (c == 2 ? EXIT_SUCCESS : 2);
 usage:



Output of `acme-client -ADFv db.wolfsden.cz' before

        ...
        acme-client: /etc/acme/acme: created
        acme-client: /etc/acme/db.wolfsden.cz.pem: created

and after:

        ...
        acme-client: /etc/acme/db.wolfsden.cz.crt: created
        acme-client: /etc/acme/db.wolfsden.cz.pem: created

I'm not sure if you consider this a problem, but thought I would let you
know.

Have a nice day,
Gray Wolf

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.

Attachment: signature.asc
Description: PGP signature

Reply via email to