[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