Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5114 closed defect (fixed)

Environ not available on all versions of OS X, segfaults tor with obfsproxy

Reported by: Sebastian Owned by:
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I have a patch that fixes it for OS X. Obviously this isn't portable, but should give us the right idea.

diff --git a/src/or/transports.c b/src/or/transports.c
index d279d7a..3393850 100644
--- a/src/or/transports.c
+++ b/src/or/transports.c
@@ -91,6 +91,8 @@
 #include "util.h"
 #include "router.h"
 
+#include <crt_externs.h>
+
 #ifdef _WIN32
 static void set_managed_proxy_environment(LPVOID *envp,
                                           const managed_proxy_t *mp);
@@ -1076,8 +1078,6 @@ set_managed_proxy_environment(LPVOID *envp, const managed_proxy_t *mp)
 
 #else /* _WIN32 */
 
-extern char **environ;
-
 /** Prepare the environment <b>envp</b> of managed proxy <b>mp</b>.
  *  <b>envp</b> is allocated on the heap and should be freed by the
  *  caller after its use. */
@@ -1090,7 +1090,7 @@ set_managed_proxy_environment(char ***envp, const managed_proxy_t *mp)
   char *transports_to_launch=NULL;
   char *bindaddr=NULL;
   int environ_size=0;
-  char **environ_tmp = environ;
+  char **environ_tmp = *_NSGetEnviron();
 
   int n_envs = mp->is_server ? ENVIRON_SIZE_SERVER : ENVIRON_SIZE_CLIENT;
 
@@ -1098,7 +1098,7 @@ set_managed_proxy_environment(char ***envp, const managed_proxy_t *mp)
     environ_size++;
     environ_tmp++;
   }
-  environ_tmp = environ;
+  environ_tmp = *_NSGetEnviron();
 
   /* allocate enough space for our env. vars and a NULL pointer */
   *envp = tor_malloc(sizeof(char*)*(environ_size+n_envs+1));

Child Tickets

Change History (12)

comment:1 Changed 8 years ago by Sebastian

Status: newneeds_review

bug5114 in my repo. Tested on OS X and Debian (Windows isn't affected).

comment:2 Changed 8 years ago by Sebastian

Linus tested on freebsd, and it works there, too.

comment:3 Changed 8 years ago by nickm

I really don't like "HAVE_CRT_EXTERNS_H" as a test here. We want to know about _NSGetEnviron, not about the header that it is declared in.

How do you like the "bug5114" branch in my public repository? It tests for _NSGetEnviron, and also refactors the environ/_NSGetEnviron decision into compat.c where it arguably belongs.

comment:4 Changed 8 years ago by Sebastian

I mostly like it, except the comment " /* OSX 10.6 and later appear to need this. */" is a bit misleading. It seems we need this for OS X 10.7 when building a mostly static Tor with backwards compatibility for 10.6. There might be more subtleties as to when the bug exactly applies, but that is the explanation I came up with (and this is also why I didn't notice the problem when I wrote the original patch).

Also it won't compile on FreeBSD, because we need extern char environ there. I pushed a bug5114 branch with an update, check it out please?

comment:5 Changed 8 years ago by Sebastian

hrm hang on, one question. You test for _NSGetEnviron() now in AC_CHECK_FUNCS, but later use HAVE_CRT_EXTERNS_H again. That's probably an accident?

comment:6 Changed 8 years ago by nickm

Pushed a slightly different fixup branch. how does that one look?

comment:7 Changed 8 years ago by Sebastian

see bug5114 in my repo, other than that, seems fine.

comment:8 in reply to:  7 Changed 8 years ago by nickm

Replying to Sebastian:

see bug5114 in my repo, other than that, seems fine.

-#ifdef HAVE_CRT_EXTERNS_H
+#ifdef HAVE__NSGETENVIRON
 #include <crt_externs.h>
 #endif

No, that's not autoconf practice. If you want to know whether you should call a function , you check for that function. If you want to know whether you should include a header, you check for that header's presence.

comment:9 Changed 8 years ago by Sebastian

oh. ok. Then it looks fine to me. Thanks!

comment:10 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Great; squashed and merged. Thanks!

comment:11 Changed 7 years ago by nickm

Keywords: tor-client added

comment:12 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.