[PATCH conntrack-tools] src: fix strncpy -Wstringop-truncation warnings

* [PATCH conntrack-tools] src: fix strncpy -Wstringop-truncation warnings
@ 2019-08-16  9:25 Jose M. Guisado Gomez
  2019-08-17  1:04 ` Duncan Roe
  0 siblings, 1 reply; 3+ messages in thread
From: Jose M. Guisado Gomez @ 2019-08-16  9:25 UTC (permalink / raw)
  To: netfilter-devel

-Wstringop-truncation warning was introduced in GCC-8 as truncation
checker for strncpy and strncat.

Systems using gcc version >= 8 would receive the following warnings:

read_config_yy.c: In function ‘yyparse’:
read_config_yy.y:1594:2: warning: ‘strncpy’ specified bound 16 equals destination size [-Wstringop-truncation]
 1594 |  strncpy(policy->name, $2, CTD_HELPER_NAME_LEN);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
read_config_yy.y:1384:2: warning: ‘strncpy’ specified bound 256 equals destination size [-Wstringop-truncation]
 1384 |  strncpy(conf.stats.logfile, $2, FILENAME_MAXLEN);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
read_config_yy.y:692:2: warning: ‘strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
  692 |  strncpy(conf.local.path, $2, UNIX_PATH_MAX);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
read_config_yy.y:169:2: warning: ‘strncpy’ specified bound 256 equals destination size [-Wstringop-truncation]
  169 |  strncpy(conf.lockfile, $2, FILENAME_MAXLEN);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
read_config_yy.y:119:2: warning: ‘strncpy’ specified bound 256 equals destination size [-Wstringop-truncation]
  119 |  strncpy(conf.logfile, $2, FILENAME_MAXLEN);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

main.c: In function ‘main’:
main.c:168:5: warning: ‘strncpy’ specified bound 4096 equals destination size [-Wstringop-truncation]
  168 |     strncpy(config_file, argv[i], PATH_MAX);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fix the issue by checking for string length first. Also using
snprintf instead.

In addition, correct an off-by-one when warning about maximum config
file path length.

Signed-off-by: Jose M. Guisado Gomez <guigom@riseup.net>
---
 src/main.c           |  4 ++--
 src/read_config_yy.y | 35 +++++++++++++++++++++++++++--------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/src/main.c b/src/main.c
index 7062e12..d4fbbfa 100644
--- a/src/main.c
+++ b/src/main.c
@@ -165,13 +165,13 @@ int main(int argc, char *argv[])
 			break;
 		case 'C':
 			if (++i < argc) {
-				strncpy(config_file, argv[i], PATH_MAX);
 				if (strlen(argv[i]) >= PATH_MAX){
 					config_file[PATH_MAX-1]='\0';
 					dlog(LOG_WARNING, "Path to config file"
 					     " to long. Cutting it down to %d"
-					     " characters", PATH_MAX);
+					     " characters", PATH_MAX - 1);
 				}
+				snprintf(config_file, PATH_MAX, "%.*s", PATH_MAX - 1, argv[i]);
 				break;
 			}
 			show_usage(argv[0]);
diff --git a/src/read_config_yy.y b/src/read_config_yy.y
index 4311cd6..696a86d 100644
--- a/src/read_config_yy.y
+++ b/src/read_config_yy.y
@@ -116,7 +116,12 @@ logfile_bool : T_LOG T_OFF
 
 logfile_path : T_LOG T_PATH_VAL
 {
-	strncpy(conf.logfile, $2, FILENAME_MAXLEN);
+	if (strlen($2) >= FILENAME_MAXLEN) {
+		dlog(LOG_ERR, "Filename is longer than %u characters",
+		     FILENAME_MAXLEN - 1);
+		exit(EXIT_FAILURE);
+	}
+	snprintf(conf.logfile, FILENAME_MAXLEN, "%.*s", FILENAME_MAXLEN - 1, $2);
 	free($2);
 };
 
@@ -166,7 +171,12 @@ syslog_facility : T_SYSLOG T_STRING
 
 lock : T_LOCK T_PATH_VAL
 {
-	strncpy(conf.lockfile, $2, FILENAME_MAXLEN);
+	if (strlen($2) >= FILENAME_MAXLEN) {
+		dlog(LOG_ERR, "Filename is longer than %u characters",
+		     FILENAME_MAXLEN - 1);
+		exit(EXIT_FAILURE);
+	}
+	snprintf(conf.lockfile, FILENAME_MAXLEN, "%.*s", FILENAME_MAXLEN - 1, $2);
 	free($2);
 };
 
@@ -689,13 +699,13 @@ unix_options:
 
 unix_option : T_PATH T_PATH_VAL
 {
-	strncpy(conf.local.path, $2, UNIX_PATH_MAX);
-	free($2);
-	if (conf.local.path[UNIX_PATH_MAX - 1]) {
+	if (strlen($2) >= UNIX_PATH_MAX) {
 		dlog(LOG_ERR, "UNIX Path is longer than %u characters",
 		     UNIX_PATH_MAX - 1);
 		exit(EXIT_FAILURE);
 	}
+	snprintf(conf.local.path, UNIX_PATH_MAX, "%.*s", UNIX_PATH_MAX - 1, $2);
+	free($2);
 };
 
 unix_option : T_BACKLOG T_NUMBER
@@ -1381,7 +1391,12 @@ stat_logfile_bool : T_LOG T_OFF
 
 stat_logfile_path : T_LOG T_PATH_VAL
 {
-	strncpy(conf.stats.logfile, $2, FILENAME_MAXLEN);
+	if (strlen($2) >= FILENAME_MAXLEN) {
+		dlog(LOG_ERR, "Log file path is longer than %u characters",
+		     FILENAME_MAXLEN - 1);
+		exit(EXIT_FAILURE);
+	}
+	snprintf(conf.stats.logfile, FILENAME_MAXLEN, "%.*s", FILENAME_MAXLEN - 1, $2);
 	free($2);
 };
 
@@ -1589,11 +1604,15 @@ helper_type: T_HELPER_POLICY T_STRING '{' helper_policy_list '}'
 		exit(EXIT_FAILURE);
 		break;
 	}
+	if (strlen($2) >= CTD_HELPER_NAME_LEN) {
+		dlog(LOG_ERR, "CTD helper name is longer than %u characters",
+		     CTD_HELPER_NAME_LEN - 1);
+		exit(EXIT_FAILURE);
+	}
 
 	policy = (struct ctd_helper_policy *) &e->data;
-	strncpy(policy->name, $2, CTD_HELPER_NAME_LEN);
+	snprintf(policy->name, CTD_HELPER_NAME_LEN, "%.*s", CTD_HELPER_NAME_LEN - 1, $2);
 	free($2);
-	policy->name[CTD_HELPER_NAME_LEN-1] = '\0';
 	/* Now object is complete. */
 	e->type = SYMBOL_HELPER_POLICY_EXPECT_ROOT;
 	stack_item_push(&symbol_stack, e);
-- 
2.23.0.rc1


^ permalink raw reply related	[flat|nested] 3+ messages in thread