On Sat, Apr 23, 2022 at 01:45:12PM +0900, Kinichiro Inoguchi wrote:
> I would like to do some clean up for openssl(1) pkcs12.
> This diff changes pointer value checking to explicit comparison with NULL,
> and no functional changes here.
> This works fine for me on my local pc.
>
> ok?
ok.
Two comments for a next diff below.
>
> Index: pkcs12.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/openssl/pkcs12.c,v
> retrieving revision 1.18
> diff -u -p -u -p -r1.18 pkcs12.c
> --- pkcs12.c 28 Mar 2022 11:02:49 -0000 1.18
> +++ pkcs12.c 23 Apr 2022 04:08:55 -0000
> @@ -556,7 +556,7 @@ pkcs12_main(int argc, char **argv)
> goto end;
> }
>
> - if (pkcs12_config.passarg) {
> + if (pkcs12_config.passarg != NULL) {
> if (pkcs12_config.export_cert)
> pkcs12_config.passargout = pkcs12_config.passarg;
> else
> @@ -567,13 +567,13 @@ pkcs12_main(int argc, char **argv)
> BIO_printf(bio_err, "Error getting passwords\n");
> goto end;
> }
> - if (!cpass) {
> + if (cpass == NULL) {
> if (pkcs12_config.export_cert)
> cpass = passout;
> else
> cpass = passin;
> }
> - if (cpass) {
> + if (cpass != NULL) {
> mpass = cpass;
> pkcs12_config.noprompt = 1;
> } else {
> @@ -581,22 +581,22 @@ pkcs12_main(int argc, char **argv)
> mpass = macpass;
> }
>
> - if (!pkcs12_config.infile)
> + if (pkcs12_config.infile == NULL)
> in = BIO_new_fp(stdin, BIO_NOCLOSE);
> else
> in = BIO_new_file(pkcs12_config.infile, "rb");
> - if (!in) {
> + if (in == NULL) {
> BIO_printf(bio_err, "Error opening input file %s\n",
> pkcs12_config.infile ? pkcs12_config.infile : "<stdin>");
> perror(pkcs12_config.infile);
> goto end;
> }
>
> - if (!pkcs12_config.outfile) {
> + if (pkcs12_config.outfile == NULL) {
> out = BIO_new_fp(stdout, BIO_NOCLOSE);
> } else
> out = BIO_new_file(pkcs12_config.outfile, "wb");
> - if (!out) {
> + if (out == NULL) {
> BIO_printf(bio_err, "Error opening output file %s\n",
> pkcs12_config.outfile ? pkcs12_config.outfile : "<stdout>");
> perror(pkcs12_config.outfile);
> @@ -637,10 +637,10 @@ pkcs12_main(int argc, char **argv)
> if (!(pkcs12_config.options & NOCERTS)) {
> certs = load_certs(bio_err, pkcs12_config.infile,
> FORMAT_PEM, NULL, "certificates");
> - if (!certs)
> + if (certs == NULL)
> goto export_end;
>
> - if (key) {
> + if (key != NULL) {
> /* Look for matching private key */
> for (i = 0; i < sk_X509_num(certs); i++) {
> x = sk_X509_value(certs, i);
> @@ -654,7 +654,7 @@ pkcs12_main(int argc, char **argv)
> break;
> }
> }
> - if (!ucert) {
> + if (ucert == NULL) {
> BIO_printf(bio_err,
> "No certificate matches private
> key\n");
> goto export_end;
> @@ -663,11 +663,11 @@ pkcs12_main(int argc, char **argv)
> }
>
> /* Add any more certificates asked for */
> - if (pkcs12_config.certfile) {
> + if (pkcs12_config.certfile != NULL) {
> STACK_OF(X509) *morecerts = NULL;
> - if (!(morecerts = load_certs(bio_err,
> + if ((morecerts = load_certs(bio_err,
> pkcs12_config.certfile, FORMAT_PEM, NULL,
> - "certificates from certfile")))
> + "certificates from certfile")) == NULL)
> goto export_end;
> while (sk_X509_num(morecerts) > 0)
> sk_X509_push(certs, sk_X509_shift(morecerts));
> @@ -680,7 +680,7 @@ pkcs12_main(int argc, char **argv)
> int vret;
> STACK_OF(X509) *chain2;
> X509_STORE *store = X509_STORE_new();
> - if (!store) {
> + if (store == NULL) {
> BIO_printf(bio_err,
> "Memory allocation error\n");
> goto export_end;
> @@ -720,12 +720,12 @@ pkcs12_main(int argc, char **argv)
> X509_alias_set1(sk_X509_value(certs, i), catmp, -1);
> }
>
> - if (pkcs12_config.csp_name && key)
> + if (pkcs12_config.csp_name != NULL && key != NULL)
> EVP_PKEY_add1_attr_by_NID(key, NID_ms_csp_name,
> MBSTRING_ASC,
> (unsigned char *) pkcs12_config.csp_name, -1);
>
> - if (pkcs12_config.add_lmk && key)
> + if (pkcs12_config.add_lmk && key != NULL)
> EVP_PKEY_add1_attr_by_NID(key, NID_LocalKeySet, 0, NULL,
> -1);
>
> @@ -743,13 +743,13 @@ pkcs12_main(int argc, char **argv)
> certs, pkcs12_config.key_pbe, pkcs12_config.cert_pbe,
> pkcs12_config.iter, -1, pkcs12_config.keytype);
>
> - if (!p12) {
> + if (p12 == NULL) {
> ERR_print_errors(bio_err);
> goto export_end;
> }
> - if (pkcs12_config.macalg) {
> + if (pkcs12_config.macalg != NULL) {
> macmd = EVP_get_digestbyname(pkcs12_config.macalg);
> - if (!macmd) {
> + if (macmd == NULL) {
> BIO_printf(bio_err,
> "Unknown digest algorithm %s\n",
> pkcs12_config.macalg);
> @@ -771,7 +771,7 @@ pkcs12_main(int argc, char **argv)
> goto end;
>
> }
> - if (!(p12 = d2i_PKCS12_bio(in, NULL))) {
> + if ((p12 = d2i_PKCS12_bio(in, NULL)) == NULL) {
> ERR_print_errors(bio_err);
> goto end;
> }
> @@ -784,7 +784,7 @@ pkcs12_main(int argc, char **argv)
> if (!pkcs12_config.twopass)
> strlcpy(macpass, pass, sizeof macpass);
>
> - if ((pkcs12_config.options & INFO) && p12->mac)
> + if ((pkcs12_config.options & INFO) && p12->mac != NULL)
I would also compare the first argument against 0:
if ((pkcs12_config.options & INFO) != 0 && p12->mac != NULL)
> BIO_printf(bio_err, "MAC Iteration %ld\n",
> p12->mac->iter ? ASN1_INTEGER_get(p12->mac->iter) : 1);
> if (pkcs12_config.macver) {
> @@ -829,7 +829,7 @@ dump_certs_keys_p12(BIO *out, PKCS12 *p1
> int ret = 0;
> PKCS7 *p7;
>
> - if (!(asafes = PKCS12_unpack_authsafes(p12)))
> + if ((asafes = PKCS12_unpack_authsafes(p12)) == NULL)
> return 0;
> for (i = 0; i < sk_PKCS7_num(asafes); i++) {
> p7 = sk_PKCS7_value(asafes, i);
> @@ -847,7 +847,7 @@ dump_certs_keys_p12(BIO *out, PKCS12 *p1
> bags = PKCS12_unpack_p7encdata(p7, pass, passlen);
> } else
> continue;
> - if (!bags)
> + if (bags == NULL)
> goto err;
> if (!dump_certs_pkeys_bags(out, bags, pass, passlen,
> options, pempass)) {
> @@ -915,9 +915,9 @@ dump_certs_pkeys_bag(BIO *out, PKCS12_SA
> if (options & NOKEYS)
> return 1;
> print_attribs(out, bag->attrib, "Bag Attributes");
> - if (!(p8 = PKCS12_decrypt_skey(bag, pass, passlen)))
> + if ((p8 = PKCS12_decrypt_skey(bag, pass, passlen)) == NULL)
> return 0;
> - if (!(pkey = EVP_PKCS82PKEY(p8))) {
> + if ((pkey = EVP_PKCS82PKEY(p8)) == NULL) {
> PKCS8_PRIV_KEY_INFO_free(p8);
> return 0;
> }
> @@ -933,7 +933,7 @@ dump_certs_pkeys_bag(BIO *out, PKCS12_SA
> BIO_printf(bio_err, "Certificate bag\n");
> if (options & NOCERTS)
> return 1;
> - if (PKCS12_get_attr(bag, NID_localKeyID)) {
> + if (PKCS12_get_attr(bag, NID_localKeyID) != NULL) {
> if (options & CACERTS)
> return 1;
> } else if (options & CLCERTS)
> @@ -941,7 +941,7 @@ dump_certs_pkeys_bag(BIO *out, PKCS12_SA
> print_attribs(out, bag->attrib, "Bag Attributes");
> if (OBJ_obj2nid(bag->value.bag->type) != NID_x509Certificate)
> return 1;
> - if (!(x509 = PKCS12_certbag2x509(bag)))
> + if ((x509 = PKCS12_certbag2x509(bag)) == NULL)
> return 0;
> dump_cert_text(out, x509);
> PEM_write_bio_X509(out, x509);
> @@ -999,7 +999,7 @@ alg_print(BIO *x, const X509_ALGOR *alg)
>
> p = alg->parameter->value.sequence->data;
> pbe = d2i_PBEPARAM(NULL, &p, alg->parameter->value.sequence->length);
> - if (!pbe)
> + if (pbe == NULL)
> return 1;
> BIO_printf(bio_err, "%s, Iteration %ld\n",
> OBJ_nid2ln(OBJ_obj2nid(alg->algorithm)),
> @@ -1050,7 +1050,7 @@ print_attribs(BIO *out, const STACK_OF(X
> ASN1_TYPE *av;
> int i, j, attr_nid;
>
> - if (!attrlst) {
> + if (attrlst == NULL) {
> BIO_printf(out, "%s: <No Attributes>\n", name);
> return 1;
> }
> @@ -1095,7 +1095,7 @@ hex_prin(BIO *out, unsigned char *buf, i
> static int
> set_pbe(BIO *err, int *ppbe, const char *str)
> {
> - if (!str)
> + if (str == NULL)
> return 0;
> if (!strcmp(str, "NONE")) {
if (strcmp(str, "NONE") != 0) {
> *ppbe = -1;
>
