imPrintf: use dynamic buffer and concatenate efficiently

[N-R-K]

The imPrintf function currently is horribly inefficient due to strlcat having to recompute strlen on every single call [^0].

The requirement of nul-terminator also makes for very awkward code like these:

https://github.com/resurrecting-open-source-projects/scrot/blob/5388fffd7be700834ee888ed129b2325c8ab1ef2/src/scrot.c#L684-L685

Instead just remember where to append, and grow the buffer dynamically if needed (which would enable properly fixing https://github.com/resurrecting-open-source-projects/scrot/issues/223).

[^0]: If the standard string copying functions weren't idiotic then strcat (and friends) wouldn't even need to exist.

[guijan]

I'd make each format specifier output directly to ret. It's not too hard to make the code grow ret and try again if it isn't large enough, or to continue if it was. 2 buffers just makes us have to keep track of and grow another buffer.

Here's a complete program that demonstrates this approach:

#include <err.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

static char *myPrintf(char *);

int
main(int argc, char **argv)
{
    char *msg;
    char *p;

    p = argc == 2 ? argv[1] : "test_$$_$a\\n";
    msg = myPrintf(p);
    fputs(msg, stdout);
    free(msg);
    return 0;
}

static char *myPrintf(char *fmt)
{
    char *ret;
    size_t retSz; /* Size of the buffer ret points to. */
    /* How many bytes we wanted to write to ret. This is where the magic
     * happens.
     * If retWantSz is > retSz-retWriteHead, grow ret and try again.
     * If retWantSz is <= retsz-retWriteHead, we wrote what we wanted to write,
     * so continue processing fmt.
     */
    size_t retWantSz;
    size_t retWriteHead; /* Index to start writing this iteration. */
    /* If retWantSz is smaller than retMinGrowth and we need to grow ret, grow
     * ret by this much. This is an optimization.
     */
    const size_t retMinGrowth = 16;
    /* How much to increment fmt this iteration.
     * Format specifiers consume multiple bytes, so we need to skip over all
     * those bytes for the next iteration.
     */
    size_t fmtInc;

    /* Start small to exercise the code. */
    if ((ret = malloc((retSz = 1))) == NULL)
        err(1, "malloc");

    /* Process fmt, writing the result to ret, growing ret as needed.
     * ret contains garbage from the index of the write head onwards and is not
     * considered '\0' terminated until fmt passes us a '\0'.
     */
    for (retWriteHead = 0;;) {
        /* Convenience variable with the space left in ret */
        size_t retSzLeft = retSz - retWriteHead;
        /* Convenience pointer to the write head. */
        char *p = ret + retWriteHead;
        char c;

        /* Loop body. */
        if (*fmt == '$') {
            fmtInc = 2;

            switch (fmt[1]) {
            case 'a':
                retWantSz = sysconf(_SC_HOST_NAME_MAX) + 1;
                if (retSzLeft >= retWantSz) {
                    gethostname(p, retSzLeft);
                    /* We had the space we wanted, so fix up retWantSz to
                     * communicate this.
                     */
                    retWantSz = strlen(p);
                }
                break;
            case '$':
                c = '$';
                goto writebyte;
            default:
                errx(1, "myPrintf: invalid format specifier '%.2s'", fmt);
            }
        } else {
            if (*fmt == '\\' && fmt[1] == 'n') {
                c = '\n';
                fmtInc = 2;
            } else {
                c = *fmt;
                fmtInc = 1;
            }
writebyte:
            retWantSz = 1;
            if (retWriteHead < retSz)
                ret[retWriteHead] = c;
        }

        /* Loop increment. */
        if (retWantSz <= retSzLeft) { /* If we did write what we wanted... */
            /* Loop condition: return if fmt is finished and we just terminated
             * ret.
             */
            if (*fmt == '\0')
                return ret;

            /* Skip over the fmt chars we processed, move the write head and
             * go on. */
            fmt += fmtInc;
            retWriteHead += retWantSz;
        } else {
            /* If ret doesn't have enough space, grow it and try again. */
            if (retWantSz < retMinGrowth)
                retWantSz = retMinGrowth;
            retSz += retWantSz;
            if ((p = realloc(ret, retSz)) == NULL)
                err(1, "realloc");
            ret = p;
        }
    }
    /* NOTREACHED */
}

In use:

$ ./a.out
test_$_jan-obsd-z87.my.domain
$ ./a.out '$f'
a.out: myPrintf: invalid format specifier '$f'
$ ./a.out 'Hello, world!\n'
Hello, world!
$ ./a.out '$$Cash$$\n'
$Cash$

[N-R-K]

2 buffers just makes us have to keep track of and grow another buffer.

The only other buffer we need to (potentially) dynamically allocate is the one needed for host name. I can add a streamReserve so that we can gethostname directly into the stream (I was going to add streamReserve() anyways as a minor optimization to streamMem).

[N-R-K]

I've pushed the change (lightly tested). Take a look and let me know how you feel about this.

[guijan]

It looks good, I didn't realize only gethostname() was troublesome, I hadn't looked very deeply into the function until now.

[N-R-K]

Added some additional assertions. The patch is looking good, should be ready for review.