Merge pull request networkupstools#3253 from jimklimov/fix-strcpy

Further removal of unbounded `strcpy()` usage

Author: jimklimov

NEWS.adoc

@@ -58,6 +58,7 @@ https://github.com/networkupstools/nut/milestone/12
      other too. [issue #3136]
    * Improved high-verbosity debug tracing of NUT `libhid` and `usbhid-ups`
      to help make sense of data processing issues (HID paths, strings). [#3201]
+   * Further removal of unbounded `strcpy()` and `sprintf()` usage. [#3253]
    * For state tree methods, introduced `difftime_st_tree_timespec()` to
      abstract platform-dependent definitions of `st_tree_timespec_t`. [PR #3156]
    * Introduced global variables for last changed timestamp and value of

clients/upsmon.c

@@ -233,19 +233,21 @@ static void wall(const char *text)
 	pclose(wf);
 #else	/* WIN32 */
 #	define MESSAGE_CMD "message.exe"
-	char * command;
+	char	*command;
 
 	/* first +1 is for the space between message and text
 	   second +1 is for trailing 0
 	   +2 is for "" */
-	command = malloc (strlen(MESSAGE_CMD) + 1 + 2 + strlen(text) + 1);
-	if( command == NULL ) {
+	size_t	commandsz = strlen(MESSAGE_CMD) + 1 + 2 + strlen(text) + 1;
+
+	command = malloc (commandsz);
+	if (command == NULL) {
 		upslog_with_errno(LOG_NOTICE, "Not enough memory for wall");
 		return;
 	}
 
-	sprintf(command,"%s \"%s\"",MESSAGE_CMD,text);
-	if ( system(command) != 0 ) {
+	snprintf(command, commandsz, "%s \"%s\"", MESSAGE_CMD, text);
+	if (system(command) != 0) {
 		upslog_with_errno(LOG_NOTICE, "Can't invoke wall");
 	}
 	free(command);

clients/upsstats.c

@@ -633,7 +633,7 @@ static void do_ifeq(const char *s)
 
 	upsdebug_call_starting_for_str1(s);
 
-	strcpy(var, s);
+	strncpy(var, s, sizeof(var) - 1);
 
 	nargs = breakargs(var, aa);
 	if(nargs != 2) {
@@ -659,7 +659,7 @@ static void do_ifbetween(const char *s)
 
 	upsdebug_call_starting_for_str1(s);
 
-	strcpy(var, s);
+	strncpy(var, s, sizeof(var) - 1);
 
 	nargs = breakargs(var, aa);
 	if (nargs != 3) {

common/setenv.c

@@ -1,4 +1,6 @@
-/* setenv.c Ben Collver <collver@softhome.net> */
+/* fallback setenv.c Ben Collver <collver@softhome.net>
+ * tightened by Jim Klimov <jimklimov+nut@gmail.com>
+ */
 #include "config.h" /* must be first */
 
 #ifndef HAVE_SETENV
@@ -10,6 +12,7 @@ int nut_setenv(const char *name, const char *value, int overwrite)
 {
 	char	*val;
 	char	*buffer;
+	size_t	buflen = 0;
 	int	rv;
 
 	if (overwrite == 0) {
@@ -19,10 +22,15 @@ int nut_setenv(const char *name, const char *value, int overwrite)
 		}
 	}
 
-	buffer = xmalloc(strlen(value) + strlen(name) + 2);
-	strcpy(buffer, name);
-	strcat(buffer, "=");
-	strcat(buffer, value);
+	buflen = strlen(value) + strlen(name) + 2;
+	buffer = xmalloc(buflen);
+	/* TOTHINK: is this stack more portable than one command?
+	 *   snprintf(buffer, buflen, "%s=%s", name, value);
+	 * (also can easily check that we got (buflen-1) as return value)
+	 */
+	strncpy(buffer, name, buflen);
+	strncat(buffer, "=", buflen);
+	strncat(buffer, value, buflen);
 	rv = putenv(buffer); /* man putenv, do not free(buffer) */
 	return (rv);
 }

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3626 utf-8
+personal_ws-1.1 en 3627 utf-8
 AAC
 AAS
 ABI
@@ -3183,6 +3183,7 @@ spellcheck
 spellchecked
 splitaddr
 splitname
+sprintf
 squasher
 sr
 src

drivers/adelsystem_cbi.c

@@ -34,7 +34,7 @@
 #endif
 
 #define DRIVER_NAME	"NUT ADELSYSTEM DC-UPS CB/CBI driver (libmodbus link type: " NUT_MODBUS_LINKTYPE_STR ")"
-#define DRIVER_VERSION	"0.06"
+#define DRIVER_VERSION	"0.07"
 
 /* variables */
 static modbus_t *mbctx = NULL;							/* modbus memory context */
@@ -951,18 +951,20 @@ int get_dev_state(devreg_t regindx, devstate_t **dvstat)
 		case LVDC:					/* "output.voltage" */
 		case LCUR:					/* "output.current" */
 			if (reg_val != 0) {
-				char	*fval_s;
 				double	fval;
+				char	*fval_s;
+				size_t	fval_sz;
 
 				state->reg.val.ui16 = reg_val;
 				fval = reg_val / 1000.00; /* convert mV to V, mA to A */
 				n = snprintf(NULL, 0, "%.2f", fval);
 				if (ptr != NULL) {
 					free(ptr);
 				}
-				fval_s = (char *)xmalloc(sizeof(char) * (n + 1));
+				fval_sz = sizeof(char) * (n + 1);
+				fval_s = (char *)xmalloc(fval_sz);
 				ptr = fval_s;
-				sprintf(fval_s, "%.2f", fval);
+				snprintf(fval_s, fval_sz, "%.2f", fval);
 				state->reg.strval = fval_s;
 			} else {
 				state->reg.val.ui16 = 0;
@@ -976,15 +978,17 @@ int get_dev_state(devreg_t regindx, devstate_t **dvstat)
 		case VAC:					/* "input.voltage" */
 			if (reg_val != 0) {
 				char	*reg_val_s;
+				size_t	reg_val_sz;
 
 				state->reg.val.ui16 = reg_val;
 				n = snprintf(NULL, 0, "%u", reg_val);
 				if (ptr != NULL) {
 					free(ptr);
 				}
-				reg_val_s = (char *)xmalloc(sizeof(char) * (n + 1));
+				reg_val_sz = sizeof(char) * (n + 1);
+				reg_val_s = (char *)xmalloc(reg_val_sz);
 				ptr = reg_val_s;
-				sprintf(reg_val_s, "%u", reg_val);
+				snprintf(reg_val_s, reg_val_sz, "%u", reg_val);
 				state->reg.strval = reg_val_s;
 			} else {
 				state->reg.val.ui16 = 0;
@@ -996,16 +1000,18 @@ int get_dev_state(devreg_t regindx, devstate_t **dvstat)
 			if (reg_val != 0) {
 				double	fval;
 				char	*fval_s;
+				size_t	fval_sz;
 
 				state->reg.val.ui16 = reg_val;
 				fval = (double )reg_val * regs[BSOC].scale;
 				n = snprintf(NULL, 0, "%.2f", fval);
 				if (ptr != NULL) {
 					free(ptr);
 				}
-				fval_s = (char *)xmalloc(sizeof(char) * (n + 1));
+				fval_sz = sizeof(char) * (n + 1);
+				fval_s = (char *)xmalloc(fval_sz);
 				ptr = fval_s;
-				sprintf(fval_s, "%.2f", fval);
+				snprintf(fval_s, fval_sz, "%.2f", fval);
 				state->reg.strval = fval_s;
 			} else {
 				state->reg.val.ui16 = 0;
@@ -1018,16 +1024,18 @@ int get_dev_state(devreg_t regindx, devstate_t **dvstat)
 			{ /* scoping */
 				double	fval;
 				char	*fval_s;
+				size_t	fval_sz;
 
 				state->reg.val.ui16 = reg_val;
 				fval = reg_val - 273.15;
 				n = snprintf(NULL, 0, "%.2f", fval);
-				fval_s = (char *)xmalloc(sizeof(char) * (n + 1));
+				fval_sz = sizeof(char) * (n + 1);
+				fval_s = (char *)xmalloc(fval_sz);
 				if (ptr != NULL) {
 					free(ptr);
 				}
 				ptr = fval_s;
-				sprintf(fval_s, "%.2f", fval);
+				snprintf(fval_s, fval_sz, "%.2f", fval);
 				state->reg.strval = fval_s;
 			}
 			upsdebugx(3, "get_dev_state: variable: %s", state->reg.strval);
@@ -1221,14 +1229,17 @@ int get_dev_state(devreg_t regindx, devstate_t **dvstat)
 		default:
 			{ /* scoping */
 				char	*reg_val_s;
+				size_t	reg_val_sz;
+
 				state->reg.val.ui16 = reg_val;
 				n = snprintf(NULL, 0, "%u", reg_val);
 				if (ptr != NULL) {
 					free(ptr);
 				}
-				reg_val_s = (char *)xmalloc(sizeof(char) * (n + 1));
+				reg_val_sz = sizeof(char) * (n + 1);
+				reg_val_s = (char *)xmalloc(reg_val_sz);
 				ptr = reg_val_s;
-				sprintf(reg_val_s, "%u", reg_val);
+				snprintf(reg_val_s, reg_val_sz, "%u", reg_val);
 				state->reg.strval = reg_val_s;
 			}
 			break;
@@ -1326,7 +1337,7 @@ modbus_t *modbus_new(const char *port)
 		}
 	} else if ((sp = strchr(port, ':')) != NULL) {
 		char *tcp_port = xmalloc(sizeof(sp));
-		strcpy(tcp_port, sp + 1);
+		strncpy(tcp_port, sp + 1, sizeof(sp));
 		*sp = '\0';
 		mb = modbus_new_tcp(port, (int)strtoul(tcp_port, NULL, 10));
 		if (mb == NULL) {

drivers/bcmxcp_usb.c

@@ -11,7 +11,7 @@
 #include <unistd.h>
 
 #define SUBDRIVER_NAME    "USB communication subdriver"
-#define SUBDRIVER_VERSION "0.27"
+#define SUBDRIVER_VERSION "0.28"
 
 /* communication driver description structure */
 upsdrv_info_t comm_upsdrv_info = {
@@ -448,7 +448,7 @@ static usb_dev_handle *open_powerware_usb(void)
 			libusb_free_device_list(devlist, 1);
 			fatal_with_errno(EXIT_FAILURE, "Out of memory");
 		}
-		sprintf(curDevice.Bus, "%03d", bus_num);
+		snprintf(curDevice.Bus, 4, "%03d", bus_num);
 
 		/* FIXME: we should also retrieve
 		 * dev->descriptor.iManufacturer

drivers/belkinunv.c

@@ -94,7 +94,7 @@
 #include "serial.h"
 
 #define DRIVER_NAME	"Belkin 'Universal UPS' driver"
-#define DRIVER_VERSION	"0.13"
+#define DRIVER_VERSION	"0.14"
 
 /* driver description structure */
 upsdrv_info_t upsdrv_info = {
@@ -759,7 +759,7 @@ static void updatestatus(int smode, const char *fmt, ...) {
 	if (strcmp(oldbuf, buf)==0) {
 		return;
 	}
-	strcpy(oldbuf, buf);
+	strncpy(oldbuf, buf, sizeof(oldbuf));
 
 	if (smode==2) {
 		/* "dumbterm" version just prints a new line each time */

drivers/generic_gpio_libgpiod.c

@@ -31,7 +31,7 @@
 #endif
 
 #define DRIVER_NAME	"GPIO UPS driver (API " WITH_LIBGPIO_VERSION_STR ")"
-#define DRIVER_VERSION	"1.05"
+#define DRIVER_VERSION	"1.06"
 
 /* driver description structure */
 upsdrv_info_t upsdrv_info = {
@@ -128,9 +128,10 @@ void gpio_open(struct gpioups_t *gpioupsfdlocal) {
 	libgpiod_data->gpioChipHandle = gpiod_chip_open_by_name(gpioupsfdlocal->chipName);
 #else	/* #if WITH_LIBGPIO_VERSION >= 0x00020000 */
 	if(!strchr(gpioupsfdlocal->chipName, '/')) {
-		char *pathName = xcalloc(strlen(gpioupsfdlocal->chipName)+6, sizeof(char));
-		strcpy(pathName, "/dev/");
-		strcat(pathName, gpioupsfdlocal->chipName);
+		size_t	pathNameLen = strlen(gpioupsfdlocal->chipName)+6;
+		char	*pathName = xcalloc(pathNameLen, sizeof(char));
+		strncpy(pathName, "/dev/", pathNameLen);
+		strncat(pathName, gpioupsfdlocal->chipName, pathNameLen);
 		libgpiod_data->gpioChipHandle = gpiod_chip_open(pathName);
 		free(pathName);
 	} else {

drivers/generic_modbus.c

@@ -31,7 +31,7 @@
 #endif
 
 #define DRIVER_NAME	"NUT Generic Modbus driver (libmodbus link type: " NUT_MODBUS_LINKTYPE_STR ")"
-#define DRIVER_VERSION	"0.08"
+#define DRIVER_VERSION	"0.09"
 
 /* variables */
 static modbus_t *mbctx = NULL;                             /* modbus memory context */
@@ -1069,7 +1069,7 @@ modbus_t *modbus_new(const char *port)
 		}
 	} else if ((sp = strchr(port, ':')) != NULL) {
 		char *tcp_port = xmalloc(sizeof(sp));
-		strcpy(tcp_port, sp + 1);
+		strncpy(tcp_port, sp + 1, sizeof(sp));
 		*sp = '\0';
 		mb = modbus_new_tcp(port, (int)strtoul(tcp_port, NULL, 10));
 		if (mb == NULL) {