Won't cert be leaked here (and in the next error condition)?

You're right. I should really test with memory leaks reporting =S actually report_memleaks is on by default, so maybe this just needs different reproduce code than what's already in the test suite.

You use int (0/1) return value (I suspect instead of bool) here like in the other openssl patch. Whilt it seemed OK for a match() I don't like it here. We've got SUCCESS/FAILURE for that.

To add to that, parameters and return values that are boolean should be denoted as such, e.g. the raw parameter here and the return value of match should be zend_bool. Furthermore I'd suggest to use minimal necessary amount of indirection, e.g. the zval **val argument of match could just as well be zval *val unless I'm missing something.

+1

Hi,

On 4 Oct, 2013, at 5:29 PM, nikic notifications@github.com wrote:

In ext/openssl/openssl.c:

• } else if (!X509_digest(peer, mdtype, md, &n)) {

•   php_error_docref(NULL TSRMLS_CC, E_ERROR, "Could not generate signature");
  

•   return 0;
  

• }

• if (raw) {

•   *out_len = n;
  

•   *out = estrndup(md, n);
  

• } else {

•   *out_len = n \* 2;
  

•   _out = emalloc(_out_len + 1);
  

•   make_digest_ex(*out, md, n);
  

• }

• return 1;
  To add to that, parameters and return values that are boolean should be denoted as such, e.g. the raw parameter here and the return value of match should be zend_bool. Furthermore I'd suggest to use minimal necessary amount of indirection, e.g. the zval **val argument of match could just as well be zval *val unless I'm missing something.

Thanks for reviewing the code, I'll make the changes when I'm hooked up to the "real" Internet again :)

—
Reply to this email directly or view it on GitHub.