Process termination test for SGX #70416

mzohreva · 2020-03-26T02:41:28Z

The issue is described in fortanix/rust-sgx#109

rust-highfive · 2020-03-26T02:41:31Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nikomatsakis · 2020-03-26T10:25:16Z

I have no particular problem adding this test to the rust repo, but I'm curious if the Rust repo is the best place for it, since it seems more like a bug in https://github.com/fortanix/rust-sgx/.

(I'm not sure how things are setup or exactly what the role of the rust-sgx project is, to be clear.)

jethrogb · 2020-03-26T08:10:41Z

src/test/run-make/sgx-process-termination/Makefile

+all:
+	$(RUSTC) accept.rs --target x86_64-fortanix-unknown-sgx
+	$(FTXSGX_ELF2SGXS) $(TMPDIR)/accept --heap-size 0x1001000 --stack-size 0x20000 --threads 10
+	timeout 10s $(FTXSGX_RUNNER) $(TMPDIR)/accept.sgxs


Why are you not using timeout 10s cargo run?

I looked at the other tests in run-make and they almost all use rustc directly with some machinery in run-make-fulldeps/tools.mk. The 2 exceptions using cargo that I found looked hacky:

https://github.com/rust-lang/rust/blob/master/src/test/run-make/thumb-none-cortex-m/Makefile

https://github.com/rust-lang/rust/blob/master/src/test/run-make/thumb-none-qemu/script.sh

I think those hacks (i.e. setting RUSTC_BOOTSTRAP=1) may be removed, as I don't see any nightly features set in Cargo.toml

jethrogb · 2020-03-26T08:11:15Z

src/test/run-make/sgx-process-termination/accept.rs

+    thread::spawn(move || {
+        let listen = TcpListener::bind("0:0").unwrap();
+        tx.send(()).unwrap();
+        while let Ok(_) = listen.accept() {}


So there's technically a race here where the channel receiver might wake up before accept is called...

True, but I think there is no reasonable way to avoid that race: we need one thread to be blocked on some I/O and another thread to cause process exit. While the first thread is blocked on I/O it cannot notify the other thread (by definition), and the second thread does not have a way of knowing the first thread is truly blocked on I/O on its own unless we are willing to check /proc/self/task/*/stat or something similar.

I think we can reasonably expect that the main thread does not exit before the spawned thread blocks on accept() in most cases.

Maybe you can do thread::yield_now a bunch after recv

I think the println! is sufficient, but thread::yield_now maybe more readable.

jethrogb · 2020-03-26T10:31:04Z

src/test/run-make/sgx-process-termination/Makefile

@@ -0,0 +1,8 @@
+-include ../../run-make-fulldeps/tools.mk
+
+# only-sgx


As discussed, I think this should run on all platforms?

Some platforms may need a platform-specific way of running the code (see wasm-* or thumb-* tests in run-make for example), do you want to have platform-specific code here to run the test for those?

I think we should just add ignore statements for platforms that have special requirements. Everything that can just run with cargo run should get tested.

mzohreva · 2020-03-26T20:34:10Z

It is true that there is a bug in rust-sgx at the moment, but it is possible to inadvertently create a bug in rust itself w.r.t this test. We think it is better to include this test here, so we can catch both kinds.

nikomatsakis · 2020-04-01T14:28:59Z

OK, let me know when y'all feel this is ready for review.

jethrogb · 2020-04-01T14:46:00Z

LGTM

mzohreva · 2020-04-01T16:15:36Z

I squashed the commits, it's ready for review

nikomatsakis · 2020-04-03T18:03:21Z

src/test/run-make/process-termination/Makefile

+	cp -a $(HERE)/$(CRATE) $(WORK_DIR)/$(CRATE)
+	cd $(WORK_DIR)/$(CRATE) && \
+	$(CARGO) build --target $(TARGET) --verbose && \
+	timeout 10s $(CARGO) run --target $(TARGET)


So why exactly does this need to be a run-make test -- is it this timeout 10s call? i.e., why can't we just make this a normal run-pass test?

Yes, exactly. The failure case would be the program not exiting and hanging indefinitely due to listener.accept().

Sure but the timeout is not needed in the success case, right? So then it's not needed ;)

run-pass tests will eventually timeout too, though I don't know how long it takes. Or maybe they don't, but instead just print some warnings and things.

I think an ordinary run-pass test would be better, personally, run-make tests are kind of a pain.

joelpalmer · 2020-04-14T09:11:16Z

Ping from Triage: Hi @mzohreva, any updates?

Related issues: - fortanix/rust-sgx#109

mzohreva · 2020-04-23T00:25:28Z

sorry for the delay, I rebased and restructured the test according to our previous discussion. It is now in src/test/ui/process-termination. I also added another test which is in the same category (and has already found a bug in our rust-sgx implementation)

nikomatsakis · 2020-04-28T13:49:45Z

@bors r+ rollup

Thanks!

bors · 2020-04-28T13:49:47Z

📌 Commit db1fbd4 has been approved by nikomatsakis

@jethrogb

Process termination test for SGX The issue is described in fortanix/rust-sgx#109 cc @jethrogb

@jethrogb

Process termination test for SGX The issue is described in fortanix/rust-sgx#109 cc @jethrogb

nikomatsakis · 2020-05-04T22:07:42Z

@bors r+

bors · 2020-05-04T22:07:43Z

📌 Commit 2e749a5 has been approved by nikomatsakis

bors · 2020-05-05T14:04:08Z

⌛ Testing commit 2e749a5 with merge 26604521fcbc365ab54a3966a4cd0254d6de7a0f...

bors · 2020-05-05T14:22:41Z

💔 Test failed - checks-azure

mzohreva · 2020-05-05T22:32:10Z

The failure seems unrelated to the PR. It failed during an LLVM build

nikomatsakis · 2020-05-06T13:40:00Z

@bors retry

bors · 2020-05-07T05:13:42Z

⌛ Testing commit 2e749a5 with merge 4249dae4d0c22b8ef962ef97829c6c315a31d3a3...

bors · 2020-05-07T05:56:18Z

💔 Test failed - checks-actions

jethrogb · 2020-05-07T06:21:44Z

src/test/ui/process-termination/process-termination-blocking-io.rs

+fn main() {
+    let (tx, rx) = mpsc::channel();
+    thread::spawn(move || {
+        let listen = TcpListener::bind("0:0").unwrap();


Should probably specify 0.0.0.0:0 here

Thanks, fixed

nikomatsakis · 2020-05-11T14:32:56Z

@bors r+

bors · 2020-05-11T14:32:57Z

📌 Commit 2b3adc9 has been approved by nikomatsakis

bors · 2020-05-13T03:29:06Z

⌛ Testing commit 2b3adc9 with merge a2e0b48...

bors · 2020-05-13T06:54:08Z

☀️ Test successful - checks-actions, checks-azure
Approved by: nikomatsakis
Pushing a2e0b48 to master...

rust-highfive assigned nikomatsakis Mar 26, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2020

jethrogb reviewed Mar 26, 2020

View reviewed changes

mzohreva force-pushed the mz/sgx-test branch from 67fdcb0 to 6a4cb13 Compare April 1, 2020 16:15

nikomatsakis reviewed Apr 3, 2020

View reviewed changes

nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2020

joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2020

Process termination tests

db1fbd4

Related issues: - fortanix/rust-sgx#109

mzohreva force-pushed the mz/sgx-test branch from 6a4cb13 to db1fbd4 Compare April 23, 2020 00:22

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 28, 2020

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 28, 2020

Rollup merge of rust-lang#70416 - mzohreva:mz/sgx-test, r=nikomatsakis

75f59ba

Process termination test for SGX The issue is described in fortanix/rust-sgx#109 cc @jethrogb

Dylan-DPC-zz mentioned this pull request Apr 28, 2020

Rollup of 5 pull requests #71653

Closed

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 28, 2020

Rollup merge of rust-lang#70416 - mzohreva:mz/sgx-test, r=nikomatsakis

5f49ff7

Process termination test for SGX The issue is described in fortanix/rust-sgx#109 cc @jethrogb

Dylan-DPC-zz mentioned this pull request Apr 28, 2020

Rollup of 5 pull requests #71656

Closed

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 2, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 5, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 7, 2020

jethrogb reviewed May 7, 2020

View reviewed changes

Fix bind address in process-termination-blocking-io test

2b3adc9

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 11, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label May 13, 2020

bors merged commit a2e0b48 into rust-lang:master May 13, 2020

		@@ -0,0 +1,8 @@
		-include ../../run-make-fulldeps/tools.mk

		# only-sgx

Process termination test for SGX #70416

Process termination test for SGX #70416

Conversation

mzohreva commented Mar 26, 2020

rust-highfive commented Mar 26, 2020

nikomatsakis commented Mar 26, 2020

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

mzohreva commented Mar 26, 2020

nikomatsakis commented Apr 1, 2020

jethrogb commented Apr 1, 2020

mzohreva commented Apr 1, 2020

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

joelpalmer commented Apr 14, 2020

mzohreva commented Apr 23, 2020

nikomatsakis commented Apr 28, 2020

bors commented Apr 28, 2020

nikomatsakis commented May 4, 2020

bors commented May 4, 2020

bors commented May 5, 2020

bors commented May 5, 2020

mzohreva commented May 5, 2020

nikomatsakis commented May 6, 2020

bors commented May 7, 2020

bors commented May 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 11, 2020

bors commented May 11, 2020

bors commented May 13, 2020

bors commented May 13, 2020