[rrd-developers] fix str{cpy,cat} and sprintf safety warnings
Martin Pelikan
martin.pelikan at gmail.com
Mon Aug 13 01:07:44 CEST 2012
Hi!
So far the cooperation has been perfect, so I'm gonna dig deeper :-)
OpenBSD has a modified toolchain that (among other cool features) warns
you about certain programming constructs that have been found unsafe,
like the use of strcpy/strcat/sprintf with non-const strings where the
length isn't either known or properly estimated.
To get rid of such warnings, I replaced the calls with strndup/strncat
or snprintf. The OpenBSD variants strlcpy/strlcat aren't present in
GNU libc and aren't likely to appear there because of Ulrich Drepper.
My recent observations regarding portability:
- Windows have _snprintf() defined in stdio.h. Does autocrap pick it up?
- if there's a system without snprintf(), it can be easily replaced by
dropping the second parameter, like so:
#define snprintf(a, b, ...) sprintf(a, __VA_ARGS__)
- I found just one #ifdef HAVE_SNPRINTF, but snprintf's already exist
without being ifdef'd, which probably means people finally have this
function on their systems and we can safely use it
Most of the cases the destination is fixed-size/on stack and the change
is trivial. There are some malloc+memset+sprintf calls in rrd_cgi.c,
that can be replaced with strndup(), which guarantees us it will use
malloc(), and therefore the memory is compatible with realloc and free.
Also, the string is said to be NUL-terminated:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strdup.html
Note that the original stralloc() in rrd_cgi.c was wrong, malloc() can
in theory return NULL that would be strcpy'd into. But malloc/realloc/
strdup NULL checks are missing all over the place, leaving that behind.
Multiple subsequent calls to strlen() may or may not be optimized out
to just one, however explicitly creating a "const size_t" variable will
probably help the compiler more.
I have also been told that using '* sizeof(char)' in malloc is pointless
so I removed it along the way (its use was inconsistent anyway).
Any questions or suggestions? It would be nice if anyone tested whether
or not does it break anything :-)
--
Martin Pelikan
Index: src/rrd_getopt.c
===================================================================
--- src/rrd_getopt.c (revision 2298)
+++ src/rrd_getopt.c (working copy)
@@ -396,7 +396,7 @@
considered as options. */
char var[100];
- sprintf(var, "_%d_GNU_nonoption_argv_flags_", getpid());
+ snprintf(var, sizeof var, "_%d_GNU_nonoption_argv_flags_", getpid());
nonoption_flags = getenv(var);
if (nonoption_flags == NULL)
nonoption_flags_len = 0;
Index: src/rrd_daemon.c
===================================================================
--- src/rrd_daemon.c (revision 2298)
+++ src/rrd_daemon.c (working copy)
@@ -599,7 +599,7 @@
else
lines = -1;
- rclen = sprintf(buffer, "%d ", lines);
+ rclen = snprintf(buffer, sizeof buffer, "%d ", lines);
va_start(argp, fmt);
#ifdef HAVE_VSNPRINTF
len = vsnprintf(buffer+rclen, sizeof(buffer)-rclen, fmt, argp);
Index: src/rrd_rpncalc.c
===================================================================
--- src/rrd_rpncalc.c (revision 2298)
+++ src/rrd_rpncalc.c (working copy)
@@ -117,7 +117,7 @@
#if defined(_WIN32) && !defined(__CYGWIN__) && !defined(__CYGWIN32__)
_itoa(rpnc[i].val, buffer, 10);
#else
- sprintf(buffer, "%d", rpnc[i].val);
+ snprintf(buffer, sizeof buffer, "%d", rpnc[i].val);
#endif
add_op(OP_NUMBER, buffer)
}
Index: src/rrd_graph.c
===================================================================
--- src/rrd_graph.c (revision 2298)
+++ src/rrd_graph.c (working copy)
@@ -1676,14 +1676,9 @@
im->gdes[i].format);
return -1;
}
-#ifdef HAVE_SNPRINTF
snprintf(im->gdes[i].legend,
FMT_LEG_LEN - 2,
im->gdes[i].format, printval, si_symb);
-#else
- sprintf(im->gdes[i].legend,
- im->gdes[i].format, printval, si_symb);
-#endif
}
graphelement = 1;
}
@@ -1771,7 +1766,7 @@
for (i = 0; i < im->gdes_c; i++) {
char prt_fctn; /*special printfunctions */
if(calc_width){
- strcpy(saved_legend, im->gdes[i].legend);
+ strncpy(saved_legend, im->gdes[i].legend, sizeof saved_legend);
}
fill_last = fill;
@@ -1938,7 +1933,7 @@
}
if(calc_width){
- strcpy(im->gdes[i].legend, saved_legend);
+ strncpy(im->gdes[i].legend, saved_legend, sizeof im->gdes[0].legend);
}
}
@@ -2021,7 +2016,7 @@
if (im->unitslength < len + 2)
im->unitslength = len + 2;
- sprintf(im->ygrid_scale.labfmt,
+ snprintf(im->ygrid_scale.labfmt, sizeof im->ygrid_scale.labfmt,
"%%%d.%df%s", len,
-fractionals, (im->symbol != ' ' ? " %c" : ""));
} else {
@@ -2029,7 +2024,7 @@
if (im->unitslength < len + 2)
im->unitslength = len + 2;
- sprintf(im->ygrid_scale.labfmt,
+ snprintf(im->ygrid_scale.labfmt, sizeof im->ygrid_scale.labfmt,
"%%%d.0f%s", len, (im->symbol != ' ' ? " %c" : ""));
}
} else { /* classic rrd grid */
@@ -2093,15 +2088,15 @@
&& (YN < im->yorigin - im->ysize || YN > im->yorigin))) {
if (im->symbol == ' ') {
if (im->extra_flags & ALTYGRID) {
- sprintf(graph_label,
+ snprintf(graph_label, sizeof graph_label,
im->ygrid_scale.labfmt,
scaledstep * (double) i);
} else {
if (MaxY < 10) {
- sprintf(graph_label, "%4.1f",
+ snprintf(graph_label, sizeof graph_label, "%4.1f",
scaledstep * (double) i);
} else {
- sprintf(graph_label, "%4.0f",
+ snprintf(graph_label, sizeof graph_label, "%4.0f",
scaledstep * (double) i);
}
}
@@ -2109,15 +2104,15 @@
char sisym = (i == 0 ? ' ' : im->symbol);
if (im->extra_flags & ALTYGRID) {
- sprintf(graph_label,
+ snprintf(graph_label, sizeof graph_label,
im->ygrid_scale.labfmt,
scaledstep * (double) i, sisym);
} else {
if (MaxY < 10) {
- sprintf(graph_label, "%4.1f %c",
+ snprintf(graph_label, sizeof graph_label, "%4.1f %c",
scaledstep * (double) i, sisym);
} else {
- sprintf(graph_label, "%4.0f %c",
+ snprintf(graph_label, sizeof graph_label, "%4.0f %c",
scaledstep * (double) i, sisym);
}
}
@@ -2134,13 +2129,13 @@
sval /= second_axis_magfact;
if(MaxY < 10) {
- sprintf(graph_label_right,"%5.1f %s",sval,second_axis_symb);
+ snprintf(graph_label_right, sizeof graph_label_right, "%5.1f %s",sval,second_axis_symb);
} else {
- sprintf(graph_label_right,"%5.0f %s",sval,second_axis_symb);
+ snprintf(graph_label_right, sizeof graph_label_right, "%5.0f %s",sval,second_axis_symb);
}
}
else {
- sprintf(graph_label_right,im->second_axis_format,sval,"");
+ snprintf(graph_label_right, sizeof graph_label_right, im->second_axis_format,sval,"");
}
gfx_text ( im,
X1+7, Y0,
@@ -2326,9 +2321,9 @@
symbol = si_symbol[scale + si_symbcenter];
else
symbol = '?';
- sprintf(graph_label, "%3.0f %c", pvalue, symbol);
+ snprintf(graph_label, sizeof graph_label, "%3.0f %c", pvalue, symbol);
} else {
- sprintf(graph_label, "%3.0e", value);
+ snprintf(graph_label, sizeof graph_label, "%3.0e", value);
}
if (im->second_axis_scale != 0){
char graph_label_right[100];
@@ -2338,14 +2333,14 @@
double mfac = 1;
char *symb = "";
auto_scale(im,&sval,&symb,&mfac);
- sprintf(graph_label_right,"%4.0f %s", sval,symb);
+ snprintf(graph_label_right, sizeof graph_label_right, "%4.0f %s", sval,symb);
}
else {
- sprintf(graph_label_right,"%3.0e", sval);
+ snprintf(graph_label_right, sizeof graph_label_right, "%3.0e", sval);
}
}
else {
- sprintf(graph_label_right,im->second_axis_format,sval,"");
+ snprintf(graph_label_right, sizeof graph_label_right, im->second_axis_format,sval,"");
}
gfx_text ( im,
@@ -4051,9 +4046,7 @@
return 0;
}
/* imginfo goes to position 0 in the prdata array */
- (*prdata)[prlines - 1] = (char*)malloc((strlen(walker->value.u_str)
- + 2) * sizeof(char));
- strcpy((*prdata)[prlines - 1], walker->value.u_str);
+ (*prdata)[prlines - 1] = strdup(walker->value.u_str);
(*prdata)[prlines] = NULL;
}
/* skip anything else */
@@ -4081,10 +4074,8 @@
rrd_set_error("realloc prdata");
return 0;
}
- (*prdata)[prlines - 1] = (char*)malloc((strlen(walker->value.u_str)
- + 2) * sizeof(char));
+ (*prdata)[prlines - 1] = strdup(walker->value.u_str);
(*prdata)[prlines] = NULL;
- strcpy((*prdata)[prlines - 1], walker->value.u_str);
} else if (strcmp(walker->key, "image") == 0) {
if ( fwrite(walker->value.u_blo.ptr, walker->value.u_blo.size, 1,
(stream ? stream : stdout)) == 0 && ferror(stream ? stream : stdout)){
Index: src/rrd_client.c
===================================================================
--- src/rrd_client.c (revision 2298)
+++ src/rrd_client.c (working copy)
@@ -913,8 +913,7 @@
break;
case RD_I_STR:
chomp(s);
- info.u_str = (char*)malloc(sizeof(char) * (strlen(s) + 1));
- strcpy(info.u_str,s);
+ info.u_str = strdup(s);
break;
case RD_I_BLO:
rrd_set_error ("rrdc_info: BLOB objects are not supported");
Index: src/rrd_info.c
===================================================================
--- src/rrd_info.c (revision 2298)
+++ src/rrd_info.c (working copy)
@@ -71,8 +71,7 @@
next->value.u_int = value.u_int;
break;
case RD_I_STR:
- next->value.u_str = (char*)malloc(sizeof(char) * (strlen(value.u_str) + 1));
- strcpy(next->value.u_str, value.u_str);
+ next->value.u_str = strdup(value.u_str);
break;
case RD_I_BLO:
next->value.u_blo.size = value.u_blo.size;
Index: src/rrd_graph_helper.c
===================================================================
--- src/rrd_graph_helper.c (revision 2298)
+++ src/rrd_graph_helper.c (working copy)
@@ -73,17 +73,16 @@
char *res=NULL;
for(int i=0;i<pa->kv_cnt;i++) {
if (!pa->kv_args[i].flag) {
- int len=0;
- if (res) {len=strlen(res); }
- char* t=realloc(res,len+3
- +strlen(pa->kv_args[i].key)
- +strlen(pa->kv_args[i].value)
- );
+ const size_t klen = strlen(pa->kv_args[i].key);
+ const size_t vlen = strlen(pa->kv_args[i].value);
+ const size_t len = res ? strlen(res) : 0;
+
+ char *t = realloc(res,len + 3 + klen + vlen);
if (! t) { return res; }
res=t;
- strcat(res,pa->kv_args[i].key);
+ strncat(res,pa->kv_args[i].key, klen);
strcat(res,"=");
- strcat(res,pa->kv_args[i].value);
+ strncat(res,pa->kv_args[i].value, vlen);
strcat(res,":");
}
}
Index: src/rrd_xport.c
===================================================================
--- src/rrd_xport.c (revision 2298)
+++ src/rrd_xport.c (working copy)
@@ -308,10 +308,9 @@
*step_list_ptr = im->gdes[im->gdes[i].vidx].step;
/* printf("%s:%lu\n",im->gdes[i].legend,*step_list_ptr); */
step_list_ptr++;
+
/* reserve room for one legend entry */
- /* is FMT_LEG_LEN + 5 the correct size? */
- if ((legend_list[j] =
- (char*)malloc(sizeof(char) * (FMT_LEG_LEN + 5))) == NULL) {
+ if ((legend_list[j] = strdup(im->gdes[i].legend)) == NULL) {
free(ref_list);
*data = NULL;
while (--j > -1)
@@ -322,11 +321,9 @@
return (-1);
}
- if (im->gdes[i].legend)
- /* omit bounds check, should have the same size */
- strcpy(legend_list[j++], im->gdes[i].legend);
- else
- legend_list[j++][0] = '\0';
+ if (im->gdes[i].legend == 0)
+ legend_list[j][0] = '\0';
+ ++j;
}
}
*step_list_ptr=0;
Index: src/rrd_cgi.c
===================================================================
--- src/rrd_cgi.c (revision 2298)
+++ src/rrd_cgi.c (working copy)
@@ -387,14 +387,10 @@
char *stralloc(
const char *str)
{
- char *nstr;
-
if (!str) {
return NULL;
}
- nstr = malloc((strlen(str) + 1));
- strcpy(nstr, str);
- return (nstr);
+ return strdup(str);
}
static int readfile(
@@ -595,12 +591,13 @@
const char **args)
{
if (argc >= 2) {
- char *xyz = malloc((strlen(args[0]) + strlen(args[1]) + 2));
+ const size_t len = strlen(args[0]) + strlen(args[1]) + 2;
+ char *xyz = malloc(len);
if (xyz == NULL) {
return stralloc("[ERROR: allocating setenv buffer]");
};
- sprintf(xyz, "%s=%s", args[0], args[1]);
+ snprintf(xyz, len, "%s=%s", args[0], args[1]);
if (putenv(xyz) == -1) {
free(xyz);
return stralloc("[ERROR: failed to do putenv]");
@@ -788,9 +785,10 @@
readfile(filename, &buffer, 0);
if (rrd_test_error()) {
- char *err = malloc((strlen(rrd_get_error()) + DS_NAM_SIZE));
+ const size_t len = strlen(rrd_get_error()) + DS_NAM_SIZE;
+ char *err = malloc(len);
- sprintf(err, "[ERROR: %s]", rrd_get_error());
+ snprintf(err, len, "[ERROR: %s]", rrd_get_error());
rrd_clear_error();
return err;
} else {
@@ -954,10 +952,9 @@
return stralloc(calcpr[0]);
} else {
if (rrd_test_error()) {
- char *err =
- malloc((strlen(rrd_get_error()) +
- DS_NAM_SIZE) * sizeof(char));
- sprintf(err, "[ERROR: %s]", rrd_get_error());
+ const size_t len = strlen(rrd_get_error()) + DS_NAM_SIZE;
+ char *err = malloc(len);
+ snprintf(err, len, "[ERROR: %s]", rrd_get_error());
rrd_clear_error();
return err;
}
@@ -998,10 +995,9 @@
last = rrd_last(argc, (char **) args - 1);
if (rrd_test_error()) {
- char *err =
- malloc((strlen(rrd_get_error()) +
- DS_NAM_SIZE) * sizeof(char));
- sprintf(err, "[ERROR: %s]", rrd_get_error());
+ const size_t len = strlen(rrd_get_error()) + DS_NAM_SIZE;
+ char *err = malloc(len);
+ snprintf(err, len, "[ERROR: %s]", rrd_get_error());
rrd_clear_error();
return err;
}
@@ -1388,6 +1384,7 @@
s_var **result;
int i, k, len;
char tmp[101];
+ size_t tmplen;
cp = getenv("REQUEST_METHOD");
ip = getenv("CONTENT_LENGTH");
@@ -1404,9 +1401,8 @@
} else if (cp && !strcmp(cp, "GET")) {
esp = getenv("QUERY_STRING");
if (esp && strlen(esp)) {
- if ((line = (char *) malloc(strlen(esp) + 2)) == NULL)
+ if ((line = strdup(esp)) == NULL)
return NULL;
- sprintf(line, "%s", esp);
} else
return NULL;
} else {
@@ -1414,22 +1410,18 @@
printf("(offline mode: enter name=value pairs on standard input)\n");
memset(tmp, 0, sizeof(tmp));
while ((cp = fgets(tmp, 100, stdin)) != NULL) {
- if (strlen(tmp)) {
- if (tmp[strlen(tmp) - 1] == '\n')
- tmp[strlen(tmp) - 1] = '&';
- if (length) {
- length += strlen(tmp);
- len = (length + 1) * sizeof(char);
+ if ((tmplen = strlen(tmp)) != 0) {
+ if (tmp[tmplen - 1] == '\n')
+ tmp[tmplen - 1] = '&';
+ length += tmplen;
+ len = (length + 1) * sizeof(char);
+ if ((unsigned) length > tmplen) {
if ((line = (char *) realloc(line, len)) == NULL)
return NULL;
- strcat(line, tmp);
+ strncat(line, tmp, tmplen);
} else {
- length = strlen(tmp);
- len = (length + 1) * sizeof(char);
- if ((line = (char *) malloc(len)) == NULL)
+ if ((line = strdup(tmp)) == NULL)
return NULL;
- memset(line, 0, len);
- strcpy(line, tmp);
}
}
memset(tmp, 0, sizeof(tmp));
@@ -1527,14 +1519,10 @@
i++;
} else { /* There is already such a name, suppose a mutiple field */
cp = ++esp;
- len =
- (strlen(result[k]->value) + (ip - esp) +
- 2) * sizeof(char);
- if ((sptr = (char *) malloc(len)) == NULL)
+ len = strlen(result[k]->value) + (ip - esp) + 2;
+ if ((sptr = (char *) calloc(len, sizeof(char))) == NULL)
return NULL;
- memset(sptr, 0, len);
- sprintf(sptr, "%s\n", result[k]->value);
- strncat(sptr, cp, ip - esp);
+ snprintf(sptr, len, "%s%s\n", result[k]->value, cp);
free(result[k]->value);
result[k]->value = rrdcgiDecodeString(sptr);
}
Index: src/rrd_parsetime.c
===================================================================
--- src/rrd_parsetime.c (revision 2298)
+++ src/rrd_parsetime.c (working copy)
@@ -599,7 +599,7 @@
scc = scc_sv;
sct = sct_sv;
sc_tokid = sc_tokid_sv;
- sprintf(sc_token, "%d", hour);
+ snprintf(sc_token, sc_len, "%d", hour);
return TIME_OK;
}
if (sc_tokid == COLON) {
@@ -631,7 +631,7 @@
scc = scc_sv;
sct = sct_sv;
sc_tokid = sc_tokid_sv;
- sprintf(sc_token, "%d", hour);
+ snprintf(sc_token, sc_len, "%d", hour);
return TIME_OK;
}
ptv->tm. tm_hour = hour;
More information about the rrd-developers
mailing list