Add bindings for ClientSession, plus demo program #1

jsha · 2020-12-09T19:27:18Z

The demo program here connects via HTTPS to a host, makes an HTTP request, and prints the response to stdout.

alex

One meta note: It's my understanding that the end goal here is for curl to be able to use rustls as a TLS backend.

The approach here seems to be to write C binings to rustls so that curl can contain C code which calls them.

An alternate approach would be to have the curl backend be written in Rust, and have it expose the struct of function pointers that curl wants.

alex · 2020-12-09T19:33:50Z

src/lib.rs

+
+static mut RUSTLS_CONFIG: Option<Arc<rustls::ClientConfig>> = None;
+
+#[no_mangle]


I'd strongly encourage you not to include global state, it makes things incredibly painful when multiple dependencies want to use this in single project.

alex · 2020-12-09T19:37:04Z

src/lib.rs

+        slice::from_raw_parts(buf, count as usize)
+    };
+    let mut cursor = Cursor::new(input_buf);
+    let n_read: usize = match session.read_tls(&mut cursor) {


It's not clear to me that this is sound -- the caller is quite likely to provide a buf which contains uninitialized memory, I believe turning those until a slice is technically not sound.

Any tips on where I can read about the soundness of turning uninitialized memory into a slice? Do I just need to initialize it? Before or after from_raw_parts?

I don't have a great citation (boo me!), it's only a soundness issue if read_tls ever reads from &mut cursor, as long as it writes to it only, then it's find.

This is, classically, a big issue in the design of io::Read, which led to this whole thing rust-lang/rust#42002

jsha · 2020-12-09T19:47:47Z

One meta note: It's my understanding that the end goal here is for curl to be able to use rustls as a TLS backend.

An alternate approach would be to have the curl backend be written in Rust, and have it expose the struct of function pointers that curl wants.

Your understanding is correct, and I like your proposed alternate approach. Note that we now have an additional goal: We'd like for Apache (and someday Nginx?) to be able to use rustls. To me, that seems to suggest writing a more general "C API for rustls" rather than "Curl API for rustls." But if we do "Curl API for rustls," we could similarly do "Apache API for rustls" (etc).

alex · 2020-12-09T19:49:19Z

Exactly, it's a big trade-off in genericness vs. ergonomics for the developers I think.

…

On Wed, Dec 9, 2020 at 2:48 PM Jacob Hoffman-Andrews < ***@***.***> wrote: One meta note: It's my understanding that the end goal here is for curl to be able to use rustls as a TLS backend. An alternate approach would be to have the curl backend be written in Rust, and have it expose the struct of function pointers that curl wants. Your understanding is correct, and I like your proposed alternate approach. Note that we now have an additional goal: We'd like for Apache (and someday Nginx?) to be able to use rustls. To me, that seems to suggest writing a more general "C API for rustls" rather than "Curl API for rustls." But if we do "Curl API for rustls," we could similarly do "Apache API for rustls" (etc). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBC3XIOSUH723OJNPMDST7H7JANCNFSM4UT6MPCQ> .

-- All that is necessary for evil to succeed is for good people to do nothing.

Also remove print_ciphersuites.

tgeoghegan · 2020-12-10T19:31:35Z

Makefile

+	cbindgen --lang C --output src/lib.h
+
+target/crustls-demo: target/main.o target/debug/libcrustls.a
+	$(CC) -Werror -o $@ $^ $(LDFLAGS)


Since you're starting out a brand new C project, you could turn on -Wall and/or -Wextra and/or -Wpedantic, at the risk of having builds break when C compilers add new warnings. This page is a decent discussion of different warning options in gcc and clang. Additionally, you could turn on gcc or clang's static analyzers for more checks. I'm not sure if there are common flags across those compilers for static analysis so you might have to check what $CC is.

tgeoghegan · 2020-12-10T19:52:04Z

src/lib.rs

+        .root_store
+        .add_server_trust_anchors(&webpki_roots::TLS_SERVER_ROOTS);
+    env_logger::init();
+    Arc::into_raw(Arc::new(config)) as *const c_void


I haven't done and Rust/C interop so I'm not familiar with the idioms and best practices, but out of curiosity: it doesn't look like you're using the thread safety features of Arc here (since the Arc values never escape either this function or rustls_client_config_free, below). And even if they did, I would guess that none of the guarantees provided by Arc would hold while the pointer is loaned out to C-land. Is there a reason not to use an Rc instead, and maybe avoid taking a lock in the Arc?

Oh, wait, I think I misunderstood how into_raw and from_raw work: the Arc and its reference count aren't thrown away, they are still in the buffer passed back to C-land. But suppose you have two threads, and one of them calls rustls_client_config_free at the same time that the other one calls rustls_client_session_new with the same pointer. The two functions will separately use Arc::from_raw to recreate an Arc. Will those two distinct Arcs give you the runtime mutual exclusivity you want? That is, is calling Arc::from_raw on the same raw buffer twice the same as doing std::clone::clone() on an Arc?

You're correct that we're not getting any of the semantics of Arc while the pointer is owned by C-land. We don't get the reference counting, let alone the locking. The reason I'm using an Arc rather than an Rc or a Box is that rustls' ClientSession::new requires an Arc as part of its type signature. Also, we need C's copy of the pointer to remain valid long-term, so we can't do Arc::new(ptr) before each call to ClientSession::new, because that would move the value pointed to by ptr, invalidating C's copy.

Also a small note: When we do Arc::into_raw, we actually get a pointer to the inner value (i.e. ClientConfig). The strong and weak counters stay in memory right where they are, a few bytes lower than ClientConfig. When we do from_raw, it does a negative offset to reconstruct the Arc, including those strong and weak counters that were hiding out at a lower memory location.

Ok, so presumably the same goes for the mutex protecting those counters, so two separate Arcs that were from_rawed into existence will be using the same lock and incrementing/decrementing the same reference count(s). Wow, that's pretty cool!

Yes, though to be pedantic, there's no lock, just atomic::AtomicUsize counts: https://doc.rust-lang.org/src/alloc/sync.rs.html#281-294

tgeoghegan · 2020-12-10T19:53:55Z

src/lib.rs

+//
+// Unsafety:
+//
+// v must be a non-null pointer that resulted from previously calling `Arc::into_raw`.


If these are meant to be doccomments, then they need ///

tgeoghegan · 2020-12-10T19:55:08Z

src/main.c

+#include <errno.h>
+#include <arpa/inet.h>
+
+#include "lib.h"


nit: the makefile makes it clear this header gets generated from lib.rs, but it'd be nice to have a comment to that effect here in the C program

tgeoghegan · 2020-12-10T19:58:27Z

src/main.c

+    m = write(fd, buf, n);
+    if(m < 0) {
+      perror("writing to stdout");
+      exit(1);


nit: calling exit(3) from a function isn't great style but it's fine for a demo/test program like this.

tgeoghegan · 2020-12-10T20:22:25Z

Makefile

@@ -0,0 +1,26 @@
+ifeq ($(shell uname),Darwin)
+    LDFLAGS := -Wl,-dead_strip


I don't think you get epoll(7) on Darwin so you won't be able to compile there anyway.

tgeoghegan · 2020-12-10T20:24:57Z

src/main.c

+  struct epoll_event ev, events[MAX_EVENTS];
+  int conn_sock, nfds, epollfd;
+
+  epollfd = epoll_create1(0);


epollfd is leaked

tgeoghegan · 2020-12-10T20:32:09Z

src/lib.rs

+    unsafe {
+        match (session as *const ClientSession).as_ref() {
+            Some(cs) => cs.wants_read(),
+            None => false,


In what case would we see None here? if session were NULL?

Yep, as_ref turns a raw pointer into an Option<&T>, which is None if the pointer is NULL.

tgeoghegan · 2020-12-10T20:35:03Z

src/lib.rs

+    n_written as ssize_t
+}
+
+// Read plaintext bytes from the ClientSession. This acts like


nit: doccomment should explain that plaintext is written into buf.

tgeoghegan · 2020-12-10T20:36:57Z

src/lib.rs

+type CrustlsResult = c_int;
+
+pub const CRUSTLS_OK: c_int = 0;
+pub const CRUSTLS_ERROR: c_int = 1;


Could this be a Rust enum that has an appropriate Into implementation to convert into c_int?

I started on this and could not get cbindgen to generate the definitions right. I'm sure it's possible, but I'd like to punt to another PR.

jsha · 2020-12-11T21:12:47Z

Thanks for the detailed comments. They led to some big improvements. This is ready for re-review.

tgeoghegan

LGTM, just make sure to initialize getaddrinfo_output to NULL in make_conn.

tgeoghegan · 2020-12-11T22:30:03Z

src/main.c

+int
+make_conn(const char *hostname)
+{
+  struct addrinfo *getaddrinfo_output;


You need to initialize this to NULL

alex · 2020-12-11T23:55:41Z

src/lib.rs

+            // and the inner ClientConfig will be dropped.
+            let arc: Arc<ClientConfig> = Arc::from_raw(c);
+            let strong_count = Arc::strong_count(&arc);
+            if strong_count < 1 {


FWIW, I'm pretty sure this is unreachable -- if it was 0 then the value would already have been freed.

Hm, good point. My goal here was to catch a double free. And indeed, I can produce unexpected results by double freeing - but they are unsound. I.e. the Arc gets freed on the first call; on the second call, the Arc (including strong_count) is uninitialized. For instance in a test run it winds up as 94281166761504. I guess there really isn't anything I can do to guard against unsound usage on the C side and I should just delete this.

Thanks for pointing out this unreachable code. Looks like I neglected to delete it in this PR. Followed up in #32.

…te_base Update base.rs.

jsha added 11 commits December 9, 2020 09:29

initial commit

2965051

Round-trip ClientSession.

95a2c5e

Fix pointer types

0cef369

Add cbindgen and fix up crate name.

31b010d

Implement more methods.

3bb0dea

It works!!

fc0d1b1

Cleanups from self-review.

6b96a4a

Handle CloseNotify.

bd9a769

Read all available bytes from ClientSession.

d139d88

Tidy up variables, and return error on EOF for write_tls

dfb0668

Make dep versions wider.

58e96b9

alex reviewed Dec 9, 2020

View reviewed changes

jsha added 7 commits December 9, 2020 13:04

Clean up some error messages.

fef350b

Also remove print_ciphersuites.

Zeroize memory before passing to read.

65dc871

Merge branch 'main' of github.com:abetterinternet/crustls into first

e352c7a

Switch from global init to returning ClientConfig.

40b0480

Add some Arc and mem::forget

479ee39

Add arc_with_incref_from_raw

398317b

Add rustls_client_config_free

cd3aeea

tgeoghegan reviewed Dec 10, 2020

View reviewed changes

jsha added 3 commits December 10, 2020 18:23

Detect more warnings.

03b3c67

Review feedback.

ed48091

Fix up "free" comment.

5dddca3

tgeoghegan approved these changes Dec 11, 2020

View reviewed changes

jsha added 2 commits December 11, 2020 14:40

Remove Darwin from Makefile.

053ecd2

Initialize getaddrinfo_output = NULL

56c7d73

jsha merged commit baafb0e into main Dec 11, 2020

jsha deleted the first branch December 11, 2020 23:48

alex reviewed Dec 11, 2020

View reviewed changes

jsha mentioned this pull request Jan 30, 2021

Remove check on strong_count < 1. #32

Merged

jsha pushed a commit that referenced this pull request Mar 1, 2021

Merge pull request #1 from abetterinternet/client_hello_callback_upda…

24731ef

…te_base Update base.rs.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bindings for ClientSession, plus demo program #1

Add bindings for ClientSession, plus demo program #1

jsha commented Dec 9, 2020

alex left a comment

alex Dec 9, 2020

alex Dec 9, 2020

jsha Dec 9, 2020

alex Dec 9, 2020

jsha commented Dec 9, 2020

alex commented Dec 9, 2020 via email

tgeoghegan Dec 10, 2020

tgeoghegan Dec 10, 2020

tgeoghegan Dec 10, 2020

jsha Dec 10, 2020

tgeoghegan Dec 10, 2020

jsha Dec 10, 2020

tgeoghegan Dec 10, 2020

tgeoghegan Dec 10, 2020

tgeoghegan Dec 10, 2020

tgeoghegan Dec 10, 2020

tgeoghegan Dec 10, 2020

tgeoghegan Dec 10, 2020

jsha Dec 10, 2020

tgeoghegan Dec 10, 2020

tgeoghegan Dec 10, 2020

jsha Dec 11, 2020

jsha commented Dec 11, 2020

tgeoghegan left a comment

tgeoghegan Dec 11, 2020

alex Dec 11, 2020

jsha Dec 12, 2020

jsha Jan 30, 2021


		static mut RUSTLS_CONFIG: Option<Arc<rustls::ClientConfig>> = None;

		#[no_mangle]

		@@ -0,0 +1,26 @@
		ifeq ($(shell uname),Darwin)
		LDFLAGS := -Wl,-dead_strip

Add bindings for ClientSession, plus demo program #1

Add bindings for ClientSession, plus demo program #1

Conversation

jsha commented Dec 9, 2020

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsha commented Dec 9, 2020

alex commented Dec 9, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsha commented Dec 11, 2020

tgeoghegan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment