On Sat, Aug 17, 2019 at 11:04:51AM +1000, Duncan Roe wrote:
> On Fri, Aug 16, 2019 at 11:25:11AM +0200, Jose M. Guisado Gomez wrote:
[...]
> There is absolutely no need to change code to eliminate GCC warnings.
>
> If you are satisfied that the code is good, put these lines near the start, e.g.
> before any #include lines:
>
> > #pragma GCC diagnostic ignored "-Wpragmas"
> > #pragma GCC diagnostic ignored "-Wstringop-truncation"
>
> The first pragma stops old GCC compilers warning about the unrecognised second
> pragma
>
> The second pragma suppresses the new warning.
Thanks for the hint.
I like this patch because it makes configuration parser slightly more
robust, ie. it bails out in case path is too long rather than silent
truncation.
Probably this approach that I'm attaching is more simple, by adding
one extra byte to ensure there's room for the nul-termination.
>From f196de88cdd9764ddc2e4de737a960972d82fe9d Mon Sep 17 00:00:00 2001
From: "Jose M. Guisado Gomez" <guigom@xxxxxxxxxx>
Date: Fri, 16 Aug 2019 11:25:11 +0200
Subject: [PATCH] src: fix strncpy -Wstringop-truncation warnings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
-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@xxxxxxxxxx>
Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
---
include/conntrackd.h | 6 +++---
include/helper.h | 2 +-
include/local.h | 4 ++--
src/main.c | 7 +++----
src/read_config_yy.y | 39 +++++++++++++++++++++++++++++----------
5 files changed, 38 insertions(+), 20 deletions(-)
diff --git a/include/conntrackd.h b/include/conntrackd.h
index 81dff221e96d..fe9ec1854a7d 100644
--- a/include/conntrackd.h
+++ b/include/conntrackd.h
@@ -85,9 +85,9 @@ union inet_address {
#define CONFIG(x) conf.x
struct ct_conf {
- char logfile[FILENAME_MAXLEN];
+ char logfile[FILENAME_MAXLEN + 1];
int syslog_facility;
- char lockfile[FILENAME_MAXLEN];
+ char lockfile[FILENAME_MAXLEN + 1];
int hashsize; /* hashtable size */
int channel_num;
int channel_default;
@@ -132,7 +132,7 @@ struct ct_conf {
int prio;
} sched;
struct {
- char logfile[FILENAME_MAXLEN];
+ char logfile[FILENAME_MAXLEN + 1];
int syslog_facility;
size_t buffer_size;
} stats;
diff --git a/include/helper.h b/include/helper.h
index d15c1c62c053..d5406674cb13 100644
--- a/include/helper.h
+++ b/include/helper.h
@@ -13,7 +13,7 @@ struct pkt_buff;
#define CTD_HELPER_POLICY_MAX 4
struct ctd_helper_policy {
- char name[CTD_HELPER_NAME_LEN];
+ char name[CTD_HELPER_NAME_LEN + 1];
uint32_t expect_timeout;
uint32_t expect_max;
};
diff --git a/include/local.h b/include/local.h
index 22859d7ab60a..9379446732ee 100644
--- a/include/local.h
+++ b/include/local.h
@@ -7,12 +7,12 @@
struct local_conf {
int reuseaddr;
- char path[UNIX_PATH_MAX];
+ char path[UNIX_PATH_MAX + 1];
};
struct local_server {
int fd;
- char path[UNIX_PATH_MAX];
+ char path[UNIX_PATH_MAX + 1];
};
/* callback return values */
diff --git a/src/main.c b/src/main.c
index 7062e12085f1..31e0eed950b4 100644
--- a/src/main.c
+++ b/src/main.c
@@ -120,8 +120,8 @@ do_chdir(const char *d)
int main(int argc, char *argv[])
{
+ char config_file[PATH_MAX + 1] = {};
int ret, i, action = -1;
- char config_file[PATH_MAX] = {};
int type = 0;
struct utsname u;
int version, major, minor;
@@ -165,13 +165,12 @@ 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';
+ if (strlen(argv[i]) > PATH_MAX) {
dlog(LOG_WARNING, "Path to config file"
" to long. Cutting it down to %d"
" characters", PATH_MAX);
}
+ snprintf(config_file, PATH_MAX, "%s", argv[i]);
break;
}
show_usage(argv[0]);
diff --git a/src/read_config_yy.y b/src/read_config_yy.y
index 4311cd6c9a2f..a4aa7f57bdef 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, "LogFile path is longer than %u characters",
+ FILENAME_MAXLEN);
+ exit(EXIT_FAILURE);
+ }
+ snprintf(conf.logfile, FILENAME_MAXLEN, "%s", $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, "LockFile path is longer than %u characters",
+ FILENAME_MAXLEN);
+ exit(EXIT_FAILURE);
+ }
+ snprintf(conf.lockfile, FILENAME_MAXLEN, "%s", $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]) {
- dlog(LOG_ERR, "UNIX Path is longer than %u characters",
- UNIX_PATH_MAX - 1);
+ if (strlen($2) > UNIX_PATH_MAX) {
+ dlog(LOG_ERR, "Path is longer than %u characters",
+ UNIX_PATH_MAX);
exit(EXIT_FAILURE);
}
+ snprintf(conf.local.path, UNIX_PATH_MAX, "%s", $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, "stats LogFile path is longer than %u characters",
+ FILENAME_MAXLEN);
+ exit(EXIT_FAILURE);
+ }
+ snprintf(conf.stats.logfile, FILENAME_MAXLEN, "%s", $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, "Helper Policy is longer than %u characters",
+ CTD_HELPER_NAME_LEN);
+ 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", $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.11.0
