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
