Store name and format string in internal state as their own copies rather than pointers to strings outside the state, for memory safety and to avoid a leak on remote control.

doc/NEWS.md

@@ -5,6 +5,7 @@
   * cleanup: added a test to "`make test`" to ensure that "`make install`" installs everything expected
   * cleanup: replaced *AC_HEADER_TIOCGWINSZ* with *AC_CHECK_HEADERS(sys/ioctl.h)* for better MacOS compatibility ([#74](https://codeberg.org/a-j-wood/pv/issues/74))
   * cleanup: with "`--sync`", call `fsync()` instead of `fdatasync()` on incapable systems ([#73](https://codeberg.org/a-j-wood/pv/issues/73))
+  * cleanup: keep self-contained copies of name and format string in PV internal state for memory safety
   * misc: moved from GitHub to Codeberg; updated issue tracking links in documentation
 
 1.7.24 - 30 July 2023

src/include/pv-internal.h

@@ -106,9 +106,9 @@ struct pvstate_s {
 	int watch_fd;			 /* fd to watch */
 	unsigned int width;              /* screen width */
 	unsigned int height;             /* screen height */
-	/*@null@*/ const char *name;	 /* display name */
+	/*@null@*/ char *name;		 /* display name */
 	char default_format[PV_SIZEOF_DEFAULT_FORMAT];	 /* default format string */
-	const char *format_string;	 /* output format string */
+	char *format_string;		 /* output format string */
 
 	/******************
 	 * Program status *

src/main/options.c

@@ -130,7 +130,7 @@ bool opts_add_file(opts_t opts, const char *filename)
  * aren't copied anywhere, just the pointers are copied, so make sure the
  * command line data isn't overwritten or argv[1] free()d or whatever.
  */
-					    /*@null@ *//*@only@ */
+/*@null@ */ /*@only@ */
 opts_t opts_parse(unsigned int argc, char **argv)
 {
 #ifdef HAVE_GETOPT_LONG

src/main/remote.c

@@ -280,13 +280,11 @@ void pv_remote_check(pvstate_t state)
 	msgbuf.name[sizeof(msgbuf.name) - 1] = '\0';
 	msgbuf.format[sizeof(msgbuf.format) - 1] = '\0';
 
-	/* TODO: replace strdup(msgbuf.name) with just msgbuf.name once pv_state_* do their own strdup of name */
-
 	pv_state_set_format(state, msgbuf.progress, msgbuf.timer,
 			    msgbuf.eta, msgbuf.fineta, msgbuf.rate,
 			    msgbuf.average_rate,
 			    msgbuf.bytes, msgbuf.bufpercent,
-			    msgbuf.lastwritten, '\0' == msgbuf.name[0] ? NULL : strdup(msgbuf.name));
+			    msgbuf.lastwritten, '\0' == msgbuf.name[0] ? NULL : msgbuf.name);
 
 	if (msgbuf.rate_limit > 0)
 		pv_state_rate_limit_set(state, msgbuf.rate_limit);
@@ -302,7 +300,7 @@ void pv_remote_check(pvstate_t state)
 	if (msgbuf.height > 0)
 		pv_state_height_set(state, msgbuf.height);
 	if (msgbuf.format[0] != '\0')
-		pv_state_format_string_set(state, strdup(msgbuf.format));
+		pv_state_format_string_set(state, msgbuf.format);
 }
 
 

src/pv/state.c

@@ -92,8 +92,15 @@ void pv_state_free(pvstate_t state)
 		free(state->display_buffer);
 	state->display_buffer = NULL;
 
-	/* TODO: after changing ->name to a strdup(), free() it here */
-	/* TODO: after changing ->format_string to a strdup(), free() it here */
+	if (NULL != state->name) {
+		free(state->name);
+		state->name = NULL;
+	}
+
+	if (NULL != state->format_string) {
+		free(state->format_string);
+		state->format_string = NULL;
+	}
 
 	if (NULL != state->transfer_buffer)
 		free(state->transfer_buffer);
@@ -138,8 +145,14 @@ void pv_state_set_format(pvstate_t state, bool progress, bool timer, bool eta, b
 		PV_ADDFORMAT(lastwritten > 0, buf);
 	}
 
-	/* TODO: free state->name, store strdup(name) */
-	state->name = name;
+	if (NULL != state->name) {
+		free(state->name);
+		state->name = NULL;
+	}
+
+	if (NULL != name)
+		state->name = strdup(name);
+
 	state->reparse_display = 1;
 }
 
@@ -247,14 +260,22 @@ void pv_state_height_set(pvstate_t state, unsigned int val)
 
 void pv_state_name_set(pvstate_t state, /*@null@ */ const char *val)
 {
-	/* TODO: free state->name, store strdup(val) */
-	state->name = val;
+	if (NULL != state->name) {
+		free(state->name);
+		state->name = NULL;
+	}
+	if (NULL != val)
+		state->name = strdup(val);
 };
 
 void pv_state_format_string_set(pvstate_t state, /*@null@ */ const char *val)
 {
-	/* TODO: free state->format_string, store strdup(val) */
-	state->format_string = val;
+	if (NULL != state->format_string) {
+		free(state->format_string);
+		state->format_string = NULL;
+	}
+	if (NULL != val)
+		state->format_string = strdup(val);
 };
 
 void pv_state_watch_pid_set(pvstate_t state, unsigned int val)