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);
^^^^
Fail!
>
> - 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);
> + mval = reallocarray(NULL, max_ne, sizeof(int));
why not use calloc(2)?
>
> if (!buf || !ne_types || !ne_values || !mval) {
> BIO_printf(bio_err, "malloc error\n");
> goto error;
> }
> +
> + sp = subject;
> + bp = buf;
> +
> if (*subject != '/') {
> BIO_printf(bio_err, "Subject does not start with '/'.\n");
> goto error;
> Index: ca.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/ca.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 ca.c
> --- ca.c 2 May 2014 17:06:46 -0000 1.48
> +++ ca.c 4 May 2014 06:07:45 -0000
> @@ -1980,17 +1980,17 @@ again2:
> goto err;
>
> /* We now just add it to the database */
> - row[DB_type] = (char *) malloc(2);
> + row[DB_type] = malloc(2);
>
> tm = X509_get_notAfter(ret);
> - row[DB_exp_date] = (char *) malloc(tm->length + 1);
> + row[DB_exp_date] = malloc(tm->length + 1);
> memcpy(row[DB_exp_date], tm->data, tm->length);
> row[DB_exp_date][tm->length] = '\0';
>
> row[DB_rev_date] = NULL;
>
> /* row[DB_serial] done already */
> - row[DB_file] = (char *) malloc(8);
> + row[DB_file] = malloc(8);
> row[DB_name] = X509_NAME_oneline(X509_get_subject_name(ret), NULL, 0);
>
> if ((row[DB_type] == NULL) || (row[DB_exp_date] == NULL) ||
> @@ -2002,8 +2002,8 @@ again2:
> row[DB_type][0] = 'V';
> row[DB_type][1] = '\0';
>
> - if ((irow = (char **)malloc(sizeof(char *) * (DB_NUMBER + 1))) ==
> - NULL) {
> + irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
why not use calloc(2)?
> + if (irow == NULL) {
> BIO_printf(bio_err, "Memory allocation failure\n");
> goto err;
> }
> @@ -2244,17 +2244,17 @@ do_revoke(X509 * x509, CA_DB * db, int t
> row[DB_serial], row[DB_name]);
>
> /* We now just add it to the database */
> - row[DB_type] = (char *) malloc(2);
> + row[DB_type] = malloc(2);
>
> tm = X509_get_notAfter(x509);
> - row[DB_exp_date] = (char *) malloc(tm->length + 1);
> + row[DB_exp_date] = malloc(tm->length + 1);
> memcpy(row[DB_exp_date], tm->data, tm->length);
> row[DB_exp_date][tm->length] = '\0';
>
> row[DB_rev_date] = NULL;
>
> /* row[DB_serial] done already */
> - row[DB_file] = (char *) malloc(8);
> + row[DB_file] = malloc(8);
>
> /* row[DB_name] done already */
>
> @@ -2267,8 +2267,8 @@ do_revoke(X509 * x509, CA_DB * db, int t
> row[DB_type][0] = 'V';
> row[DB_type][1] = '\0';
>
> - if ((irow = (char **)malloc(sizeof(char *) *
> - (DB_NUMBER + 1))) == NULL) {
> + irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
why not use calloc(2)?
> + if (irow == NULL) {
> BIO_printf(bio_err, "Memory allocation failure\n");
> goto err;
> }
> @@ -2404,7 +2404,7 @@ do_updatedb(CA_DB * db)
>
> /* get actual time and make a string */
> a_tm = X509_gmtime_adj(a_tm, 0);
> - a_tm_s = (char *) malloc(a_tm->length + 1);
> + a_tm_s = malloc(a_tm->length + 1);
> if (a_tm_s == NULL) {
> cnt = -1;
> goto err;
> @@ -2761,7 +2761,7 @@ bin2hex(unsigned char * data, size_t len
> char hex[] = "0123456789ABCDEF";
> int i;
>
> - if ((ret = malloc(len * 2 + 1))) {
> + if ((ret = malloc(len * 2 + 1)) != NULL) {
> for (i = 0; i < len; i++) {
> ret[i * 2 + 0] = hex[data[i] >> 4];
> ret[i * 2 + 1] = hex[data[i] & 0x0F];
> Index: dgst.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/dgst.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 dgst.c
> --- dgst.c 2 May 2014 17:06:46 -0000 1.29
> +++ dgst.c 4 May 2014 06:07:45 -0000
> @@ -131,7 +131,7 @@ dgst_main(int argc, char **argv)
>
> signal(SIGPIPE, SIG_IGN);
>
> - if ((buf = (unsigned char *) malloc(BUFSIZE)) == NULL) {
> + if ((buf = malloc(BUFSIZE)) == NULL) {
> BIO_printf(bio_err, "out of memory\n");
> goto end;
> }
> Index: dh.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/dh.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 dh.c
> --- dh.c 24 Apr 2014 12:22:22 -0000 1.18
> +++ dh.c 4 May 2014 06:07:45 -0000
> @@ -251,7 +251,7 @@ bad:
>
> len = BN_num_bytes(dh->p);
> bits = BN_num_bits(dh->p);
> - data = (unsigned char *) malloc(len);
> + data = malloc(len);
> if (data == NULL) {
> perror("malloc");
> goto end;
> Index: dhparam.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/dhparam.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 dhparam.c
> --- dhparam.c 24 Apr 2014 12:39:02 -0000 1.24
> +++ dhparam.c 4 May 2014 06:07:45 -0000
> @@ -410,7 +410,7 @@ bad:
>
> len = BN_num_bytes(dh->p);
> bits = BN_num_bits(dh->p);
> - data = (unsigned char *) malloc(len);
> + data = malloc(len);
> if (data == NULL) {
> perror("malloc");
> goto end;
> Index: dsaparam.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/dsaparam.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 dsaparam.c
> --- dsaparam.c 24 Apr 2014 12:39:02 -0000 1.22
> +++ dsaparam.c 4 May 2014 06:07:45 -0000
> @@ -307,7 +307,7 @@ bad:
>
> len = BN_num_bytes(dsa->p);
> bits_p = BN_num_bits(dsa->p);
> - data = (unsigned char *) malloc(len + 20);
> + data = malloc(len + 20);
> if (data == NULL) {
> perror("malloc");
> goto end;
> Index: ecparam.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/ecparam.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 ecparam.c
> --- ecparam.c 24 Apr 2014 12:22:22 -0000 1.10
> +++ ecparam.c 4 May 2014 06:07:45 -0000
> @@ -312,7 +312,7 @@ bad:
>
> crv_len = EC_get_builtin_curves(NULL, 0);
>
> - curves = malloc((int) (sizeof(EC_builtin_curve) * crv_len));
> + curves = reallocarray(NULL, crv_len, sizeof(EC_builtin_curve));
calloc(2)?
>
> if (curves == NULL)
> goto end;
> @@ -465,7 +465,7 @@ bad:
> if ((tmp_len = (size_t) BN_num_bytes(ec_cofactor)) > buf_len)
> buf_len = tmp_len;
>
> - buffer = (unsigned char *) malloc(buf_len);
> + buffer = malloc(buf_len);
>
> if (buffer == NULL) {
> perror("malloc");
> Index: enc.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/enc.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 enc.c
> --- enc.c 3 May 2014 16:03:54 -0000 1.28
> +++ enc.c 4 May 2014 06:07:45 -0000
> @@ -345,7 +345,7 @@ enc_main(int argc, char **argv)
> BIO_printf(bio_err, "bufsize=%d\n", bsize);
> }
> strbuf = malloc(SIZE);
> - buff = (unsigned char *) malloc(EVP_ENCODE_LENGTH(bsize));
> + buff = malloc(EVP_ENCODE_LENGTH(bsize));
> if ((buff == NULL) || (strbuf == NULL)) {
> BIO_printf(bio_err, "malloc failure %ld\n", (long)
> EVP_ENCODE_LENGTH(bsize));
> goto end;
> Index: rsa.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/rsa.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 rsa.c
> --- rsa.c 24 Apr 2014 12:22:22 -0000 1.17
> +++ rsa.c 4 May 2014 06:07:45 -0000
> @@ -349,7 +349,7 @@ bad:
>
> i = 1;
> size = i2d_RSA_NET(rsa, NULL, NULL, sgckey);
> - if ((p = (unsigned char *) malloc(size)) == NULL) {
> + if ((p = malloc(size)) == NULL) {
> BIO_printf(bio_err, "Memory allocation failure\n");
> goto end;
> }
> Index: rsautl.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/rsautl.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 rsautl.c
> --- rsautl.c 23 Apr 2014 19:09:48 -0000 1.16
> +++ rsautl.c 4 May 2014 06:07:45 -0000
> @@ -256,6 +256,11 @@ rsautl_main(int argc, char **argv)
>
> rsa_in = malloc(keysize * 2);
> rsa_out = malloc(keysize);
> + if (rsa_in == NULL || rsa_out == NULL) {
> + BIO_printf(bio_err, "Memory allocation failure\n");
> + ERR_print_errors(bio_err);
> + goto end;
> + }
>
> /* Read the input data */
> rsa_inlen = BIO_read(in, rsa_in, keysize * 2);
> Index: s_client.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/s_client.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 s_client.c
> --- s_client.c 2 May 2014 17:06:46 -0000 1.48
> +++ s_client.c 4 May 2014 06:07:46 -0000
> @@ -472,10 +472,13 @@ ssl_srp_verify_param_cb(SSL * s, void *a
> static char *
> ssl_give_srp_client_pwd_cb(SSL * s, void *arg)
> {
> - SRP_ARG *srp_arg = (SRP_ARG *) arg;
> - char *pass = (char *) malloc(PWD_STRLEN + 1);
> + SRP_ARG *srp_arg;
> + char *pass;
> PW_CB_DATA cb_tmp;
> int l;
> +
> + srp_arg = arg;
> + pass = malloc(PWD_STRLEN + 1);
>
> cb_tmp.password = (char *) srp_arg->srppassin;
> cb_tmp.prompt_info = "SRP user";
> Index: speed.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/speed.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 speed.c
> --- speed.c 2 May 2014 17:06:46 -0000 1.38
> +++ speed.c 4 May 2014 06:07:46 -0000
> @@ -563,11 +563,11 @@ speed_main(int argc, char **argv)
> rsa_key[i] = NULL;
> #endif
>
> - if ((buf = (unsigned char *) malloc((int) BUFSIZE)) == NULL) {
> + if ((buf = malloc(BUFSIZE)) == NULL) {
> BIO_printf(bio_err, "out of memory\n");
> goto end;
> }
> - if ((buf2 = (unsigned char *) malloc((int) BUFSIZE)) == NULL) {
> + if ((buf2 = malloc(BUFSIZE)) == NULL) {
> BIO_printf(bio_err, "out of memory\n");
> goto end;
> }
> @@ -2178,7 +2178,8 @@ do_multi(int multi)
> int *fds;
> static char sep[] = ":";
>
> - fds = malloc(multi * sizeof *fds);
> + fds = reallocarray(NULL, multi, sizeof(int));
> + /* If the reallocarray failed, it looks acceptable to just die here. */
huh?
> for (n = 0; n < multi; ++n) {
> if (pipe(fd) == -1) {
> fprintf(stderr, "pipe failure\n");
> Index: srp.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/apps/srp.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 srp.c
> --- srp.c 24 Apr 2014 12:22:22 -0000 1.10
> +++ srp.c 4 May 2014 06:07:46 -0000
> @@ -176,7 +176,8 @@ update_index(CA_DB * db, BIO * bio, char
> char **irow;
> int i;
>
> - if ((irow = (char **) malloc(sizeof(char *) * (DB_NUMBER + 1))) ==
> NULL) {
> + irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
calloc(2)?
> + if (irow == NULL)
> BIO_printf(bio_err, "Memory allocation failure\n");
> return 0;
> }
>
