Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

crypto: export externals to internal structs#8355

Closed
indutny wants to merge 1 commit intonodejs:v0.12from
indutny:feature/externify
Closed

crypto: export externals to internal structs#8355
indutny wants to merge 1 commit intonodejs:v0.12from
indutny:feature/externify

Conversation

@indutny
Copy link
Copy Markdown
Member

@indutny indutny commented Sep 12, 2014

Export External getters for a internal structs: SSL, SSL_CTX.

cc @tjfontaine , as discussed on nodeconf.

Export External getters for a internal structs: SSL, SSL_CTX.
@indutny
Copy link
Copy Markdown
Member Author

indutny commented Sep 12, 2014

cc @trevnorris ;)

@indutny indutny added the tls label Sep 12, 2014
@indutny indutny added this to the v0.12 milestone Sep 12, 2014
@indutny
Copy link
Copy Markdown
Member Author

indutny commented Sep 12, 2014

For those, who just came over it. I'm going to export some External wrapped pointers to internal fields, to make it easier for addon authors to interact with internal structs like SSL and SSL_CTX.

If you guys do need some other pointers - please comment here.

@bnoordhuis
Copy link
Copy Markdown
Member

Why the indirect approach? There is no reference counting going on which is asking for use-after-free bugs. If the goal is to expose internals to native add-ons, then why not expose them directly through node.h?

@indutny
Copy link
Copy Markdown
Member Author

indutny commented Sep 12, 2014

@bnoordhuis because it is very easy? :)

On a more serious note: there are some classes like Connection and TLSWrap that share the base class, so the SSL couldn't really be extracted out of them without knowing the exact class that is being used.

Anyway, I'm open to suggestions and please paste some example methods if you a better idea ;)

@bnoordhuis
Copy link
Copy Markdown
Member

Is the idea to expose the SSL and SSL_CTX pointers with TLSCallbacks::Wrap()-wrapped JS objects as the input? What about lifetimes? Is the caller allowed to borrow the references for longer periods of time? I think I'd start out conservative with something like:

Local<Object> js_tls_object = /* ... */;
{
  node::Borrow<SSL> ssl(js_tls_object);
  CHECK_NE(*ssl, NULL);
  do_something_with(*ssl);
}

It's not perfect but it makes it clear that the lifetime of the SSL* is tied to the lifetime of the Borrow object.

@indutny
Copy link
Copy Markdown
Member Author

indutny commented Sep 12, 2014

Hm... in such context - object will be alive anyway, even without on-stack guard, right?

Let's be honest about SSL, SSL_CTX. What I was thinking about - is providing some SSL_xxx methods in addons, that are currently not exposed in node_crypto.cc. All of them, do not really need the SSL to be retained. So, at least, with OpenSSL's handles - it is not a big deal to just assume that _external will be used immediately, and the people who will not want it to happen could always retain the object by keeping the reference manually.

Anyway, this is not a documented feature, and is mostly a way for some people to write useful addons without caring much about stable version API/ABI.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

info.Holder().

@indutny
Copy link
Copy Markdown
Member Author

indutny commented Sep 23, 2014

Landed in 6e08bb9, ignoring most of the feedback. Thank you :)

@indutny indutny closed this Sep 23, 2014
@indutny indutny deleted the feature/externify branch September 23, 2014 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants