We are going to completely ignore diffs which change multiple idioms
at once.

That is how mistakes get made.

> On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
> > Hello,
> > 
> > I've gone through lib/libssl/src/apps with the goal of making {m,c,re}alloc
> > uses more idiomatic, adding error checking in some places where missing,
> > and some minor style unification.
> > 
> > Feedback appreciated, better patches to come after the semester ends.
> > 
> > 
> > Index: apps.c
> > ===================================================================
> > RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
> > retrieving revision 1.45
> > diff -u -p -r1.45 apps.c
> > --- apps.c  3 May 2014 16:03:54 -0000       1.45
> > +++ apps.c  4 May 2014 06:07:45 -0000
> > @@ -209,13 +209,12 @@ chopup_args(ARGS * arg, char *buf, int *
> >     *argc = 0;
> >     *argv = NULL;
> >  
> > -   i = 0;
> >     if (arg->count == 0) {
> >             arg->count = 20;
> > -           arg->data = (char **)malloc(sizeof(char *) * arg->count);
> > +           arg->data = calloc(arg->count, sizeof(char *));
> > +           if (arg->data == NULL)
> > +                   return 0;
> >     }
> > -   for (i = 0; i < arg->count; i++)
> > -           arg->data[i] = NULL;
> >  
> >     num = 0;
> >     p = buf;
> > @@ -232,8 +231,7 @@ chopup_args(ARGS * arg, char *buf, int *
> >             if (num >= arg->count) {
> >                     char **tmp_p;
> >                     int tlen = arg->count + 20;
> > -                   tmp_p = (char **) realloc(arg->data,
> > -                       sizeof(char *) * tlen);
> > +                   tmp_p = reallocarray(arg->data, tlen, sizeof(char *));
> >                     if (tmp_p == NULL)
> >                             return 0;
> >                     arg->data = tmp_p;
> > @@ -266,7 +264,7 @@ chopup_args(ARGS * arg, char *buf, int *
> >     }
> >     *argc = num;
> >     *argv = arg->data;
> > -   return (1);
> > +   return 1;
> >  }
> >  
> >  int
> > @@ -404,7 +402,28 @@ password_callback(char *buf, int bufsiz,
> >                     ok = UI_add_input_string(ui, prompt, ui_flags, buf,
> >                         PW_MIN_LENGTH, bufsiz - 1);
> >             if (ok >= 0 && verify) {
> > -                   buff = (char *) malloc(bufsiz);
> > +                   buff = malloc(bufsiz);
> > +                   /*
> > +                    * NULL returns appear to be handled by the following:
> > +                    *
> > +                    * UI_add_verify_string(x, x, x, buff=NULL, x, x, x) ->
> > +                    *   general_allocate_string(x, x, x, UIT_VERIFY, x, \
> > +                    *   result_buf=NULL, x, x, x) ->
> > +                    *     general_allocate_prompt(x, x, x, x, x, NULL) ->
> > +                    *       ret = NULL;
> > +                    *       if (type == UIT_VERIFY && result_buf == NULL)
> > +                    *         UIerr(...); and dont touch ret
> > +                    *     returns NULL
> > +                    *   returns -1
> > +                    * returns -1
> > +                    *
> > +                    * So, we /should/ (maybe) be good. Not calling UIerr()
> > +                    * could very well have some well-hidden side-effects
> > +                    * that I would definitly not notice myself, so I'll
> > +                    * leave this as is without an explicit check here.
> > +                    * Maybe somebody who knows better than I has a better
> > +                    * suggestion?
> > +                    */
> >                     ok = UI_add_verify_string(ui, prompt, ui_flags, buff,
> >                         PW_MIN_LENGTH, bufsiz - 1, buf);
> >             }
> > @@ -1830,26 +1849,30 @@ parse_yesno(const char *str, int def)
> >  X509_NAME *
> >  parse_name(char *subject, long chtype, int multirdn)
> >  {
> > -   size_t buflen = strlen(subject) + 1;    /* to copy the types and
> > -                                            * values into. due to
> > -                                            * escaping, the copy can
> > -                                            * only become shorter */
> > -   char *buf = malloc(buflen);
> > -   size_t max_ne = buflen / 2 + 1; /* maximum number of name elements */
> > -   char **ne_types = malloc(max_ne * sizeof(char *));
> > -   char **ne_values = malloc(max_ne * sizeof(char *));
> > -   int *mval = malloc(max_ne * sizeof(int));
> > +   size_t buflen, max_ne;
> > +   char **ne_types, **ne_values, *buf, *sp, *bp;
> > +   int *mval, i, nid, ne_num = 0;
> > +   X509_NAME *n = NULL;
> >  
> > -   char *sp = subject, *bp = buf;
> > -   int i, ne_num = 0;
> > +   /* Due to escaping, buf can never be bigger than subject. */
> > +   buflen = strlen(subject + 1);
>                                ^^^^
> In addition to already mentioned mistake in above strlen()
> 
> > -   X509_NAME *n = NULL;
> > -   int nid;
> > +   /* maximum number of name elements */
> > +   max_ne = buflen / 2 + 1;
> > +
> > +   buf = malloc(buflen);
> > +   ne_types = malloc(max_ne);
> > +   ne_values = malloc(max_ne);
> 
> above two malloc() calls are inconsistent with original
> code, which passed 'max_ne * sizeof(char *)' to malloc().
> 
> --patrick
> 

Reply via email to