imPrintf: avoid going out of bound

[N-R-K]

when the last char is $ or \, the c pointer would end up advancing past the string length.

additionally use the return value of strftime for determining the string length instead of checking for '\0'.

Fixes: https://github.com/resurrecting-open-source-projects/scrot/issues/252

[guijan]

The code could just copy the '\0' from str to ret as if it were any other character, then at the bottom of the loop, break out before incrementing if it copied a '\0' like the example in https://github.com/resurrecting-open-source-projects/scrot/pull/250#issuecomment-1483060435

[N-R-K]

then at the bottom of the loop, break out before incrementing

No, because the 3rd expression in the for loop will increment c beyond the nul byte before the condition is assessed. Here's a small test to reproduce:

diff --git a/src/scrot.c b/src/scrot.c
index 651c5d0..e381482 100644
--- a/src/scrot.c
+++ b/src/scrot.c
@@ -600,6 +600,7 @@ static char *imPrintf(char *str, struct tm *tm, char *filenameIM,

     if (strftime(strf, 4095, str, tm) == 0)
         errx(EXIT_FAILURE, "strftime returned 0");
+    strf[strlen(strf) + 1] = 'X';

     imlib_context_set_image(im);
     for (c = strf; *c != '\0'; c++) {

Now run scrot file$ under a debugger and you'll notice that it starts reading the X past the string.

[guijan]

I was thinking of something like this. How about it?

static char *imPrintf(char *str, struct tm *tm, char *filenameIM,
    char *filenameThumb, Imlib_Image im)
{
    char *c;
    char buf[20];
    Stream ret = {0};
    long hostNameMax = 0;
    char strf[4096];
    char *tmp;
    struct stat st;

    if (strftime(strf, 4095, str, tm) == 0)
        errx(EXIT_FAILURE, "strftime returned 0");

    imlib_context_set_image(im);
    for (c = strf;; c++) {
        switch(*c) {
        case '$':
            c++;
            switch (*c) {
            case 'a':
                /* freebsd and macos don't have HOST_NAME_MAX defined.
                 * instead sysconf is recommended by freebsd.
                 * +1 for nul-terminator. */
                if (hostNameMax == 0)
                    hostNameMax = sysconf(_SC_HOST_NAME_MAX) + 1;

                streamReserve(&ret, hostNameMax);
                char *target = ret.buf + ret.off;
                gethostname(target, hostNameMax);
                ret.off += strlen(target);
                assert(ret.buf[ret.off] == '\0');
                break;
            case 'f':
                if (filenameIM)
                    streamStr(&ret, filenameIM);
                break;
            case 'm': /* t was already taken, so m as in mini */
                if (filenameThumb)
                    streamStr(&ret, filenameThumb);
                break;
            case 'n':
                if (filenameIM) {
                    tmp = strrchr(filenameIM, '/');
                    streamStr(&ret, tmp ? tmp+1 : filenameIM);
                }
                break;
            case 'w':
                snprintf(buf, sizeof(buf), "%d", imlib_image_get_width());
                streamStr(&ret, buf);
                break;
            case 'h':
                snprintf(buf, sizeof(buf), "%d", imlib_image_get_height());
                streamStr(&ret, buf);
                break;
            case 's':
                if (filenameIM) {
                    if (!stat(filenameIM, &st)) {
                        int size;

                        size = st.st_size;
                        snprintf(buf, sizeof(buf), "%d", size);
                        streamStr(&ret, buf);
                    } else
                        streamStr(&ret, "[err]");
                }
                break;
            case 'p':
                snprintf(buf, sizeof(buf), "%d",
                    imlib_image_get_width() * imlib_image_get_height());
                streamStr(&ret, buf);
                break;
            case 't':
                tmp = imlib_image_format();
                if (tmp)
                    streamStr(&ret, tmp);
                break;
            case 'W':
                if (clientWindow) {
                    if (!(tmp = scrotGetWindowName(clientWindow)))
                        break;
                    streamStr(&ret, tmp);
                    free(tmp);
                }
                break;
            default:
                goto writechar;
            }
            break;
        case '\\':
            switch (c[1]) {
            case 'n':
                c[1] = '\n';
                /* FALLTHROUGH */
            case '\\':
                c++;
                break;
            }
            /* FALLTHROUGH */
        default:
writechar:
            streamChar(&ret, *c);
            if (*c == '\0')
                return ret.buf;
        }
    }
    /* NOTREACHED. */
}

Haven't tested it.

[N-R-K]

How about it?

Not a fan of what's going on with those fallthroughs in the '\\' case.