[rrd-developers] fix str{cpy,cat} and sprintf safety warnings
Tobias Oetiker
tobi at oetiker.ch
Mon Aug 13 08:21:25 CEST 2012
Hi Martin,
Today Martin Pelikan wrote:
> 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?
on windows, people do not run autoconf ... :-)
> - 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 :-)
from reading the patch it looks good ... I have added it to trun
r2299
cheers
tobi
> --
> 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;
>
> _______________________________________________
> rrd-developers mailing list
> rrd-developers at lists.oetiker.ch
> https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
>
>
--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch tobi at oetiker.ch ++41 62 775 9902 / sb: -9900
More information about the rrd-developers
mailing list