On Sat, Apr 23, 2022 at 08:31:50AM +0200, Theo Buehler wrote:
> 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.
Thanks for checking this.
>
> 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)
Agree and I will check other pkcs12_config.options by follow up commit.
>
> > 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) {
Yes I would like to do this too, and I thought this should be "== 0"
if (strcmp(str, "NONE") == 0) {
>
> > *ppbe = -1;
> >
