changing sprintf -> snprintf & co

[omar-polo]

First of all, sorry for the long, boring diff!

While fixing the time_t casting i noticed a few sprintf calls and so I started to convert them to snprintf to avoid mangling the stack on overflow. Well... it took a bit longer than what I expected! There are still some strcat/strcpy/sprintf around, I changed only the "easy" ones, the majority of the diff is mechanical.

I'm quite confident of the changes and I've played a bit with Xephyr and it didn't explode, but it definitely needs more testing as I'm not sure how to trigger all those code paths.

[omar-polo]

wooops, i haven't thought that strlcat is not always available...

[ThomasAdam]

Hi @omar-polo,

It looks like we're building a compat version of strlcpy in libs/ -- perhaps you can check why the visibility isn't there for it?

[omar-polo]

I'm still not sure how the compat are handled, am i wrong or strlcpy is always built? anyway, here's an attempt at adding a strlcat(3) compat. Haven't tested on !OpenBSD yet, was hoping the CI would kick in and compile it for me ^^"

will try on linux soonish

[ThomasAdam]

Thanks. CI has confirmed that things are building. I'm not near a PC until tomorrow, so can't help until then. :⁠-⁠)

[omar-polo]

I've re-read it again and played outside Xephyr (using the default configuration though), nothing exploded! i'll call it a win :)

A few sprintf calls remains that are not trivial to replace, the majority is gone however! There are also a couple of other functions used often that should be dropped (atoi, alloca, ...) but getting rid of those string operations on unbound buffers is already a good start :D

[ThomasAdam]

Hi @omar-polo

Thanks for this. I've now had a proper look.

I see you've changed libs/cJSON.c -- this is actually a third-party JSON library. You weren't to know. But given how infrequently it changes, I think the change you made is fine. I've added an include for libs/strlcat.h to silence the compiler.

I've also fixed a few other warnings, mostly around the manual malloc/s(n)printf handling, and moved those calls to xasprintf where appropriate.

I'm in-lining my diff ontop of your latest commit on the PR. Can you take a look, and if happy, add it as a formal part of this PR? Then I'll merge it.

Thanks again for this -- it mops up a small part of something that should have been addressed years ago. :) Hopefully we can move on to the other areas you've identified next as well. ;)

commit 80cead578b734bf9d52500c15b60ea0776d1005b
Author: Thomas Adam <[email protected]>
Date:   Sun Oct 16 11:40:39 2022 +0100

    More sprintf/asprintf fixes

diff --git a/libs/cJSON.h b/libs/cJSON.h
index e97e5f4cd..41eb1fdf0 100644
--- a/libs/cJSON.h
+++ b/libs/cJSON.h
@@ -85,6 +85,8 @@ then using the CJSON_API_VISIBILITY flag to "export" the same symbols the way CJ
 
 #include <stddef.h>
 
+#include "libs/strlcat.h"
+
 /* cJSON Types: */
 #define cJSON_Invalid (0)
 #define cJSON_False  (1 << 0)
diff --git a/modules/FvwmAuto/FvwmAuto.c b/modules/FvwmAuto/FvwmAuto.c
index 73f8d82e9..779d4c6ff 100644
--- a/modules/FvwmAuto/FvwmAuto.c
+++ b/modules/FvwmAuto/FvwmAuto.c
@@ -105,7 +105,6 @@ main(int argc, char **argv)
 	char *enter_fn="Silent Raise";        /* default */
 	char *leave_fn=NULL;
 	char *buf;
-	int len;
 	unsigned long m_mask;
 	unsigned long mx_mask;
 	unsigned long last_win = 0;   /* last window handled */
@@ -337,22 +336,6 @@ main(int argc, char **argv)
 	fd_width = fd[1] + 1;
 	FD_ZERO(&in_fdset);
 
-	/* create the command buffer */
-	len = 0;
-	if (enter_fn != 0)
-	{
-		len = strlen(enter_fn);
-	}
-	if (leave_fn != NULL)
-	{
-		len = max(len, strlen(leave_fn));
-	}
-	if (do_pass_id)
-	{
-		len += 32;
-	}
-	buf = fxmalloc(len);
-
 	while (!isTerminated)
 	{
 		char raise_window_now;
@@ -509,14 +492,16 @@ main(int argc, char **argv)
 				/* if focus_win isn't the root */
 				if (do_pass_id)
 				{
-					snprintf(buf, sizeof(buf), "%s 0x%x\n", leave_fn,
+					xasprintf(&buf, "%s 0x%x\n", leave_fn,
 						(int)last_win);
 				}
 				else
 				{
-					snprintf(buf, sizeof(buf), "%s\n", leave_fn);
+					xasprintf(&buf, "%s\n", leave_fn);
 				}
 				SendInfo(fd, buf, last_win);
+				free(buf);
+
 				if (use_enter_mode)
 				{
 					raised_win = 0;
@@ -528,14 +513,16 @@ main(int argc, char **argv)
 				/* if focus_win isn't the root */
 				if (do_pass_id)
 				{
-					snprintf(buf, sizeof(buf), "%s 0x%x\n", enter_fn,
+					xasprintf(&buf, "%s 0x%x\n", enter_fn,
 						(int)focus_win);
 				}
 				else
 				{
-					snprintf(buf, sizeof(buf), "%s\n", enter_fn);
+					xasprintf(&buf, "%s\n", enter_fn);
 				}
 				SendInfo(fd, buf, focus_win);
+				free(buf);
+
 				raised_win = focus_win;
 			}
 			else if (focus_win && enter_fn == NULL)
diff --git a/modules/FvwmButtons/FvwmButtons.c b/modules/FvwmButtons/FvwmButtons.c
index f486fcf51..e3d07e030 100644
--- a/modules/FvwmButtons/FvwmButtons.c
+++ b/modules/FvwmButtons/FvwmButtons.c
@@ -3373,7 +3373,6 @@ void exec_swallow(char *action, button_info *b)
 {
 	static char *my_sm_env = NULL;
 	static char *orig_sm_env = NULL;
-	static int len = 0;
 	static Bool sm_initialized = False;
 	static Bool session_manager = False;
 	char *cmd;
@@ -3404,13 +3403,10 @@ void exec_swallow(char *action, button_info *b)
 	if (my_sm_env == NULL)
 	{
 		my_sm_env = getenv("SESSION_MANAGER");
-		len = 45 + strlen(my_sm_env) + strlen(orig_sm_env);
 	}
 
 	/* TA:  FIXME!  xasprintf() */
-	cmd = fxmalloc(len + strlen(action));
-	snprintf(cmd, sizeof(cmd),
-		"FSMExecFuncWithSessionManagment \"%s\" \"%s\" \"%s\"",
+	xasprintf(&cmd, "FSMExecFuncWithSessionManagment \"%s\" \"%s\" \"%s\"",
 		my_sm_env, action, orig_sm_env);
 	SendText(fd, cmd, 0);
 	free(cmd);

[omar-polo]

@ThomasAdam thanks! and sorry for modifying cJSON -- i started with grep and killed most instances of sprintf without looking if it were a vendored library. Do you think upstream could be interested in merging those changes? i could try upstreaming that part if so.

i've also taken the liberty to reorder the commits, so it adds the compat for strlcat before using that.

thanks!

[ThomasAdam]

@ThomasAdam thanks! and sorry for modifying cJSON -- i started with grep and killed most instances of sprintf without looking if it were a vendored library. Do you think upstream could be interested in merging those changes? i could try upstreaming that part if so.

Wouldn't hurt to try, I suppose. :)

I'll go ahead and merge this, once the CI has completed, and if there's any fallout from that, it'll be clearer -- but I'm not expecting any.

[omar-polo]

Thanks!