New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: fix malloc mismatch in SSL_set_tlsext_status_ocsp_resp call #25706

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@davidben
Copy link
Contributor

davidben commented Jan 25, 2019

SSL_set_tlsext_status_ocsp_resp expects the data to be allocated with OPENSSL_malloc, not libc malloc, so use OpenSSLMalloc.

Additionally, though OpenSSL doesn't type-check due to it being a macro, the function is documented to take an unsigned char pointer:
https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_tlsext_status_ocsp_resp.html

(By default, OPENSSL_malloc is the same as libc malloc, but it is possible to customize this.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
tls: fix malloc mismatch in SSL_set_tlsext_status_ocsp_resp call
SSL_set_tlsext_status_ocsp_resp expects the data to be allocated with
OPENSSL_malloc, not libc malloc, so use OpenSSLMalloc.

Additionally, though OpenSSL doesn't type-check due to it being a macro,
the function is documented to take an unsigned char pointer:
https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_tlsext_status_ocsp_resp.html

(By default, OPENSSL_malloc is the same as libc malloc, but it is
possible to customize this.)
@ofrobots

This comment has been minimized.

Copy link
Contributor

ofrobots commented Jan 25, 2019

@davidben

This comment has been minimized.

Copy link
Contributor Author

davidben commented Jan 27, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20323/

This is probably a silly question. I'm having a hard time navigating this page to find what's failing there. What's the error?

@ofrobots

This comment has been minimized.

Copy link
Contributor

ofrobots commented Jan 28, 2019

@davidben test-performance failed, but that is a suspected flake as per #23291. I've lunched a 'Resume CI': https://ci.nodejs.org/job/node-test-pull-request/20380/ which should get this re-run.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 28, 2019

@antsmartian

This comment has been minimized.

Copy link
Contributor

antsmartian commented Jan 29, 2019

Landed in b530466 🎉

antsmartian added a commit that referenced this pull request Jan 29, 2019

tls: fix malloc mismatch in SSL_set_tlsext_status_ocsp_resp call
SSL_set_tlsext_status_ocsp_resp expects the data to be allocated with
OPENSSL_malloc, not libc malloc, so use OpenSSLMalloc.

Additionally, though OpenSSL doesn't type-check due to it being a macro,
the function is documented to take an unsigned char pointer:
https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_tlsext_status_ocsp_resp.html

(By default, OPENSSL_malloc is the same as libc malloc, but it is
possible to customize this.)

PR-URL: #25706
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos added a commit that referenced this pull request Jan 29, 2019

tls: fix malloc mismatch in SSL_set_tlsext_status_ocsp_resp call
SSL_set_tlsext_status_ocsp_resp expects the data to be allocated with
OPENSSL_malloc, not libc malloc, so use OpenSSLMalloc.

Additionally, though OpenSSL doesn't type-check due to it being a macro,
the function is documented to take an unsigned char pointer:
https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_tlsext_status_ocsp_resp.html

(By default, OPENSSL_malloc is the same as libc malloc, but it is
possible to customize this.)

PR-URL: #25706
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

@targos targos referenced this pull request Jan 29, 2019

Merged

v11.9.0 proposal #25802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment