Unnecessary string truncation in src/scrot.c

[guijan]

This truncates:

scrot/src/scrot.c

Lines 613 to 614 in 4c59bd4

	gethostname(buf, sizeof(buf));
	strlcat(ret, buf, sizeof(ret));

sizeof(buf) is 20:

scrot/src/scrot.c

Line 596 in 4c59bd4

char buf[20];

POSIX says:

{HOST_NAME_MAX}
    Maximum length of a host name (not including the terminating null) as returned from the [gethostname()](https://pubs.opengroup.org/onlinepubs/009695399/functions/gethostname.html) function.
    Minimum Acceptable Value: {_POSIX_HOST_NAME_MAX}

Further down:

{_POSIX_HOST_NAME_MAX}
    Maximum length of a host name (not including the terminating null) as returned from the [gethostname()](https://pubs.opengroup.org/onlinepubs/009695399/functions/gethostname.html) function.
    Value: 255

The fix here is to make buf's size HOST_NAME_MAX+1, but the real problem is the function's design. It should make gethostname() directly output to ret and it should dynamically grow ret as needed.

[N-R-K]

Github doesn't understand "partially fixes" and keeps closing this, pretty annoying. :/

But in any case, strftime is still happening on a fixed length buffer. So I don't think this is fully fixed.

[N-R-K]

I think the only remaining issue now is the strftime buffer. This would (almost) fix it:

     Stream ret = {0};
     long hostNameMax = 0;
-    char strf[4096];
+    char *strf = NULL;
+    size_t strfSize = 64;
     char *tmp;
     struct stat st;
 
-    if (strftime(strf, 4095, str, tm) == 0)
-        errx(EXIT_FAILURE, "strftime returned 0");
+    do {
+        strfSize *= 2;
+        strf = erealloc(strf, strfSize);
+    } while (strftime(strf, strfSize, str, tm) == 0); /* FIXME: infinite loop */
 
     imlib_context_set_image(im);

But the problem is strftime can validly return 0 and there's no way to distinguish between a valid 0 return vs a 0 return due to small buffer.

[guijan]

Filenames can't be the null string, dirname and basename will guarantee that the destination is at least "." when #226 is fixed.

I've been reading books instead of contributing as of late.

[N-R-K]

From the strftime manpage:

Note that the return value 0 does not necessarily indicate an error. For example, in many
locales %p yields an empty string.

So if someone does scrot "%p" strftime may "validly" return a zero. I don't think the chdir will fix that.

I've been reading books instead of contributing as of late.

It's voluntary work, do it at your own pace :)

[guijan]

Maybe we can use strptime() then.
https://pubs.opengroup.org/onlinepubs/007904875/functions/strptime.html
Or, set the first byte to zero, call strftime(), and if it's still 0 after the call and the size of the buffer is greater than 2 bytes, strftime() created a null string.

[N-R-K]

and if it's still 0 after the call and the size of the buffer is greater than 2 bytes

In the case of error 0 return, the contents of buf is undefined.

If the length of the result string (including the terminating null byte) would exceed max bytes, then strftime() returns 0, and the contents of the array are undefined.

So we can't read the contents of the buffer to make any conclusions.

Maybe we can use strptime() then.

I'll take a look into it.

[guijan]

POSIX says unspecified. It's unlikely an an implementation wouldn't output directly to the buffer anyway.

[guijan]

If the output file is an absolute path (starts with '/'), then strftime() can't create a 0-sized string. If the output file isn't an absolute path, prepending "./" to it is harmless and prevents the creation of a 0-sized string.