Merge pull request #854 from rhenium/ky/do-not-use-null-sk

Avoid calling sk_*() with NULL

ext/openssl/ossl.c

@@ -69,16 +69,9 @@ ossl_##name##_sk2ary(const STACK_OF(type) *sk)	\
     int i, num;					\
     VALUE ary;					\
 						\
-    if (!sk) {					\
-	OSSL_Debug("empty sk!");		\
-	return Qnil;				\
-    }						\
+    RUBY_ASSERT(sk != NULL);			\
     num = sk_##type##_num(sk);			\
-    if (num < 0) {				\
-	OSSL_Debug("items in sk < -1???");	\
-	return rb_ary_new();			\
-    }						\
-    ary = rb_ary_new2(num);			\
+    ary = rb_ary_new_capa(num);			\
 						\
     for (i=0; i<num; i++) {			\
 	t = sk_##type##_value(sk, i);		\

ext/openssl/ossl_pkcs7.c

@@ -557,21 +557,16 @@ ossl_pkcs7_get_signer(VALUE self)
 {
     PKCS7 *pkcs7;
     STACK_OF(PKCS7_SIGNER_INFO) *sk;
-    PKCS7_SIGNER_INFO *si;
     int num, i;
     VALUE ary;
 
     GetPKCS7(self, pkcs7);
-    if (!(sk = PKCS7_get_signer_info(pkcs7))) {
-	OSSL_Debug("OpenSSL::PKCS7#get_signer_info == NULL!");
-	return rb_ary_new();
-    }
-    if ((num = sk_PKCS7_SIGNER_INFO_num(sk)) < 0) {
-	ossl_raise(ePKCS7Error, "Negative number of signers!");
-    }
-    ary = rb_ary_new2(num);
+    if (!(sk = PKCS7_get_signer_info(pkcs7)))
+        return rb_ary_new();
+    num = sk_PKCS7_SIGNER_INFO_num(sk);
+    ary = rb_ary_new_capa(num);
     for (i=0; i<num; i++) {
-	si = sk_PKCS7_SIGNER_INFO_value(sk, i);
+	PKCS7_SIGNER_INFO *si = sk_PKCS7_SIGNER_INFO_value(sk, i);
 	rb_ary_push(ary, ossl_pkcs7si_new(si));
     }
 
@@ -604,7 +599,6 @@ ossl_pkcs7_get_recipient(VALUE self)
 {
     PKCS7 *pkcs7;
     STACK_OF(PKCS7_RECIP_INFO) *sk;
-    PKCS7_RECIP_INFO *si;
     int num, i;
     VALUE ary;
 
@@ -615,13 +609,11 @@ ossl_pkcs7_get_recipient(VALUE self)
 	sk = pkcs7->d.signed_and_enveloped->recipientinfo;
     else sk = NULL;
     if (!sk) return rb_ary_new();
-    if ((num = sk_PKCS7_RECIP_INFO_num(sk)) < 0) {
-	ossl_raise(ePKCS7Error, "Negative number of recipient!");
-    }
-    ary = rb_ary_new2(num);
+    num = sk_PKCS7_RECIP_INFO_num(sk);
+    ary = rb_ary_new_capa(num);
     for (i=0; i<num; i++) {
-	si = sk_PKCS7_RECIP_INFO_value(sk, i);
-	rb_ary_push(ary, ossl_pkcs7ri_new(si));
+        PKCS7_RECIP_INFO *ri = sk_PKCS7_RECIP_INFO_value(sk, i);
+        rb_ary_push(ary, ossl_pkcs7ri_new(ri));
     }
 
     return ary;
@@ -701,7 +693,10 @@ ossl_pkcs7_set_certificates(VALUE self, VALUE ary)
     X509 *cert;
 
     certs = pkcs7_get_certs(self);
-    while((cert = sk_X509_pop(certs))) X509_free(cert);
+    if (certs) {
+        while ((cert = sk_X509_pop(certs)))
+            X509_free(cert);
+    }
     rb_block_call(ary, rb_intern("each"), 0, 0, ossl_pkcs7_set_certs_i, self);
 
     return ary;
@@ -710,7 +705,10 @@ ossl_pkcs7_set_certificates(VALUE self, VALUE ary)
 static VALUE
 ossl_pkcs7_get_certificates(VALUE self)
 {
-    return ossl_x509_sk2ary(pkcs7_get_certs(self));
+    STACK_OF(X509) *certs = pkcs7_get_certs(self);
+    if (!certs)
+        return Qnil;
+    return ossl_x509_sk2ary(certs);
 }
 
 static VALUE
@@ -741,7 +739,10 @@ ossl_pkcs7_set_crls(VALUE self, VALUE ary)
     X509_CRL *crl;
 
     crls = pkcs7_get_crls(self);
-    while((crl = sk_X509_CRL_pop(crls))) X509_CRL_free(crl);
+    if (crls) {
+        while ((crl = sk_X509_CRL_pop(crls)))
+            X509_CRL_free(crl);
+    }
     rb_block_call(ary, rb_intern("each"), 0, 0, ossl_pkcs7_set_crls_i, self);
 
     return ary;
@@ -750,7 +751,10 @@ ossl_pkcs7_set_crls(VALUE self, VALUE ary)
 static VALUE
 ossl_pkcs7_get_crls(VALUE self)
 {
-    return ossl_x509crl_sk2ary(pkcs7_get_crls(self));
+    STACK_OF(X509_CRL) *crls = pkcs7_get_crls(self);
+    if (!crls)
+        return Qnil;
+    return ossl_x509crl_sk2ary(crls);
 }
 
 static VALUE

ext/openssl/ossl_ssl.c

@@ -2450,7 +2450,7 @@ ossl_ssl_get_peer_finished(VALUE self)
 
 /*
  * call-seq:
- *    ssl.client_ca => [x509name, ...]
+ *    ssl.client_ca => [x509name, ...] or nil
  *
  * Returns the list of client CAs. Please note that in contrast to
  * SSLContext#client_ca= no array of X509::Certificate is returned but
@@ -2468,6 +2468,8 @@ ossl_ssl_get_client_ca_list(VALUE self)
     GetSSL(self, ssl);
 
     ca = SSL_get_client_CA_list(ssl);
+    if (!ca)
+        return Qnil;
     return ossl_x509name_sk2ary(ca);
 }
 

ext/openssl/ossl_x509cert.c

@@ -619,10 +619,7 @@ ossl_x509_get_extensions(VALUE self)
 
     GetX509(self, x509);
     count = X509_get_ext_count(x509);
-    if (count < 0) {
-	return rb_ary_new();
-    }
-    ary = rb_ary_new2(count);
+    ary = rb_ary_new_capa(count);
     for (i=0; i<count; i++) {
 	ext = X509_get_ext(x509, i); /* NO DUP - don't free! */
 	rb_ary_push(ary, ossl_x509ext_new(ext));

ext/openssl/ossl_x509crl.c

@@ -276,21 +276,19 @@ ossl_x509crl_get_revoked(VALUE self)
 {
     X509_CRL *crl;
     int i, num;
-    X509_REVOKED *rev;
-    VALUE ary, revoked;
+    STACK_OF(X509_REVOKED) *sk;
+    VALUE ary;
 
     GetX509CRL(self, crl);
-    num = sk_X509_REVOKED_num(X509_CRL_get_REVOKED(crl));
-    if (num < 0) {
-	OSSL_Debug("num < 0???");
-	return rb_ary_new();
-    }
-    ary = rb_ary_new2(num);
+    sk = X509_CRL_get_REVOKED(crl);
+    if (!sk)
+        return rb_ary_new();
+
+    num = sk_X509_REVOKED_num(sk);
+    ary = rb_ary_new_capa(num);
     for(i=0; i<num; i++) {
-	/* NO DUP - don't free! */
-	rev = sk_X509_REVOKED_value(X509_CRL_get_REVOKED(crl), i);
-	revoked = ossl_x509revoked_new(rev);
-	rb_ary_push(ary, revoked);
+	X509_REVOKED *rev = sk_X509_REVOKED_value(sk, i);
+	rb_ary_push(ary, ossl_x509revoked_new(rev));
     }
 
     return ary;
@@ -451,11 +449,7 @@ ossl_x509crl_get_extensions(VALUE self)
 
     GetX509CRL(self, crl);
     count = X509_CRL_get_ext_count(crl);
-    if (count < 0) {
-	OSSL_Debug("count < 0???");
-	return rb_ary_new();
-    }
-    ary = rb_ary_new2(count);
+    ary = rb_ary_new_capa(count);
     for (i=0; i<count; i++) {
 	ext = X509_CRL_get_ext(crl, i); /* NO DUP - don't free! */
 	rb_ary_push(ary, ossl_x509ext_new(ext));

ext/openssl/ossl_x509name.c

@@ -354,11 +354,7 @@ ossl_x509name_to_a(VALUE self)
 
     GetX509Name(self, name);
     entries = X509_NAME_entry_count(name);
-    if (entries < 0) {
-	OSSL_Debug("name entries < 0!");
-	return rb_ary_new();
-    }
-    ret = rb_ary_new2(entries);
+    ret = rb_ary_new_capa(entries);
     for (i=0; i<entries; i++) {
 	if (!(entry = X509_NAME_get_entry(name, i))) {
 	    ossl_raise(eX509NameError, NULL);

ext/openssl/ossl_x509revoked.c

@@ -194,11 +194,7 @@ ossl_x509revoked_get_extensions(VALUE self)
 
     GetX509Rev(self, rev);
     count = X509_REVOKED_get_ext_count(rev);
-    if (count < 0) {
-	OSSL_Debug("count < 0???");
-	return rb_ary_new();
-    }
-    ary = rb_ary_new2(count);
+    ary = rb_ary_new_capa(count);
     for (i=0; i<count; i++) {
 	ext = X509_REVOKED_get_ext(rev, i);
 	rb_ary_push(ary, ossl_x509ext_new(ext));

test/openssl/test_pkcs7.rb

@@ -160,6 +160,34 @@ def test_enveloped
     }
   end
 
+  def test_data
+    asn1 = OpenSSL::ASN1::Sequence([
+      OpenSSL::ASN1::ObjectId("pkcs7-data"),
+      OpenSSL::ASN1::OctetString("content", 0, :EXPLICIT),
+    ])
+    p7 = OpenSSL::PKCS7.new
+    p7.type = :data
+    p7.data = "content"
+    assert_raise(OpenSSL::PKCS7::PKCS7Error) { p7.add_certificate(@ee1_cert) }
+    assert_raise(OpenSSL::PKCS7::PKCS7Error) { p7.certificates = [@ee1_cert] }
+    assert_raise(OpenSSL::PKCS7::PKCS7Error) { p7.cipher = "aes-128-cbc" }
+    assert_equal(asn1.to_der, p7.to_der)
+
+    p7 = OpenSSL::PKCS7.new(asn1)
+    assert_equal(:data, p7.type)
+    assert_equal(false, p7.detached?)
+    # Not applicable
+    assert_nil(p7.certificates)
+    assert_nil(p7.crls)
+    # Not applicable. Should they return nil or raise an exception instead?
+    assert_equal([], p7.signers)
+    assert_equal([], p7.recipients)
+    # PKCS7#verify can't distinguish verification failure and other errors
+    store = OpenSSL::X509::Store.new
+    assert_equal(false, p7.verify([@ee1_cert], store))
+    assert_raise(OpenSSL::PKCS7::PKCS7Error) { p7.decrypt(@rsa1024) }
+  end
+
   def test_empty_signed_data_ruby_bug_19974
     data = "-----BEGIN PKCS7-----\nMAsGCSqGSIb3DQEHAg==\n-----END PKCS7-----\n"
     assert_raise(ArgumentError) { OpenSSL::PKCS7.new(data) }