Tor cleans out environment before launching obfsproxy

This means obfsproxy can't launch in TBB, because LD_LIBRARY_PATH is not used. We should probably fix this along with #5076 (moved) by just using Tor's environment, and appending the new env vars we need.

changed milestone to %Tor: 0.2.3.x-final

added component::core tor/tor milestone::Tor: 0.2.3.x-final priority::high resolution::fixed status::closed tor-client type::task labels

So basically, you want tor to pass **environ to execve or CreateProcess?

Doesn't set_managed_proxy_environment do exactly this?

Only on Windows.

bug5082 in my repo for a fix for this and also for #5076 (moved). Please review

Trac:
Status: new to needs_review

Along with this goes an update for proposal 180 in my prop180 branch in my torspec repo

I think Tor should not fail to try launching obfsproxy just because it doesn't find HOME or PATH in its env. That's definitely a bug in Tor.

Does obfsproxy actually need HOME or PATH? I guess there's a question of which one should try to reconstruct it if it isn't defined. But I think currently it doesn't, so maybe we don't need to answer that question yet.

Is it dangerous to pass Tor's env vars to obfsproxy? I'm not sure. I think the benefits outweight the risks.

You'll definitely want to change the sentence "We can do this only if the spec change to require HOME and PATH to be set is accepted" because it implies that you want to change the spec to require HOME and PATH to be set.

I'd suggest a follow-up cleanup commit to get rid of the r, and change the function to return a void, clean up launch_managed_proxy() so it knows set_managed_proxy_environment() can't fail, etc.

I have not evaluated the fiddly pointer arithmetic yet.

Trac:
Cc: N/A to twilde@cymru.com

Pointer arithmetic (ugh) looks plausible.

I also tested it and it works.

Trac:
Status: needs_review to needs_revision

Replying to arma:

Does obfsproxy actually need HOME or PATH? I guess there's a question of which one should try to reconstruct it if it isn't defined. But I think currently it doesn't, so maybe we don't need to answer that question yet.

At least currently, it doesn't. Needing HOME would be quite strange indeed for a daemon at least. And what would it do with PATH?

Is it dangerous to pass Tor's env vars to obfsproxy? I'm not sure. I think the benefits outweight the risks.

I can't think of a reason why it'd be dangerous.

You'll definitely want to change the sentence "We can do this only if the spec change to require HOME and PATH to be set is accepted" because it implies that you want to change the spec to require HOME and PATH to be set.

fixed. that doesn't belong in the changelog anyway.

I'd suggest a follow-up cleanup commit to get rid of the r, and change the function to return a void, clean up launch_managed_proxy() so it knows set_managed_proxy_environment() can't fail, etc.

added

I have not evaluated the fiddly pointer arithmetic yet.

also changed as suggested on irc.

fixup + follow up commit added.

Trac:
Status: needs_revision to needs_review

Update looks good to me. I'll let Nick merge it since he should opine on the "get rid of $HOME and $PATH requirement" too.

Replying to arma:

I also tested it and it works.

For posterity, here was my obfsproxy patch for testing it:

diff --git a/src/managed.c b/src/managed.c
index 01814f8..ffa40a8 100644
--- a/src/managed.c
+++ b/src/managed.c
@@ -26,6 +26,9 @@
 
 #include "container.h"
 
+#include <unistd.h>
+extern char **environ;
+
 #include <event2/event.h>
 #include <event2/listener.h>
 
@@ -430,6 +433,12 @@ handle_environment(managed_proxy_t *proxy)
   char *tmp;
   enum env_parsing_status status=ST_ENV_SMOOTH;
 
+  char **environ_tmp = environ;
+  while (*environ_tmp) {
+    log_info("env: %s", *environ_tmp);
+    environ_tmp++;
+  }
+
   tmp = getenv("TOR_PT_STATE_LOCATION");
   if (!tmp) {
     status = ST_ENV_FAIL_STATE_LOC;

Squashed and merged this. #5105 (moved) should clean it up a fair bit, too. Thanks, all!

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Trac:
Keywords: N/A deleted, tor-client added

Trac:
Component: Tor Client to Tor

closed

mentioned in issue #5105 (moved)

moved to tpo/core/tor#5082 (closed)