| Message ID | 20190816092511.830-1-guigom@riseup.net |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [conntrack-tools] src: fix strncpy -Wstringop-truncation warnings | expand |
Hi Jose, PLEASE DO NOT DO THIS! See at end On Fri, Aug 16, 2019 at 11:25:11AM +0200, Jose M. Guisado Gomez wrote: > -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 > 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. Cheers ... Duncan.
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.
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);
-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(-)