Opened 7 years ago

Closed 7 years ago

#10845 closed defect (fixed)

Make libeay32.dll and ssleay32.dll visible to pluggable transports

Reported by: dcf Owned by: erinn
Priority: Medium Milestone:
Component: Applications/Tor bundles/installation Version:
Severity: Keywords: tbb-3.5, MikePerry201402R
Cc: mikeperry, gk Actual Points:
Parent ID: #9444 Points:
Reviewer: Sponsor:

Description

When we moved the pluggable transport executables into a PluggableTransports subdirectory, it broke some pluggable transport programs on Windows because they can't find the OpenSSL DLLs. To wit, it broke flashproxy-reg-appspot and flashproxy-reg-email, because they use the M2Crypto Python module, which relies on OpenSSL. The effect was that the windows bundle was falling back to flashproxy-reg-http.

This patch makes a copy of libeay32.dll and ssleay32.dll in the PluggableTransports directory. This works because the directory containing an executable is part of the DLL search path of the executable.
http://msdn.microsoft.com/en-us/library/7d83bc18.aspx

Nothing is required for linux and mac because their RelativeLink scripts set LD_LIBRARY_PATH and DYLD_LIBRARY_PATH respectively to contain the directory with the OpenSSL dynamic libraries.

Alternative solutions may be to modify the PATH environment variable to include the directory containing tor.exe (which would have an effect similar to that of setting LD_LIBRARY_PATH in RelativeLink.sh), or to call SetDllDirectory (I don't know if SetDllDirectory is inherited by subprocesses).

Child Tickets

Attachments (4)

0001-Copy-libeay32.dll-and-ssleay32.dll-to-PluggableTrans.patch (1.3 KB) - added by dcf 7 years ago.
0001-Put-the-Tor-subdirectory-at-the-head-of-PATH-for-DLL.patch (4.8 KB) - added by dcf 7 years ago.
bug10845-apart.patch (11.6 KB) - added by dcf 7 years ago.
Separate patches (git am style) with fixes from comment:4 and comment:5.
bug10845-together.patch (5.0 KB) - added by dcf 7 years ago.
Combined patch with fixes from comment:4 and comment:5.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by gk

I think I'd prefer to not copy the files if that's possible to avoid unnecessary bloat. Modifying PATH sounds like a good alternative to me (if it works, that is).

comment:2 Changed 7 years ago by dcf

Here's a patch for RelativeLink.c that puts the Tor directory at the beginning of PATH. It has some groveling with C string concatenation and the Windows API. I'm not an expert at the Windows API but I checked the docs for the functions I used (GetCurrentDirectory, GetEnvironmentVariable).

comment:3 Changed 7 years ago by dcf

Parent ID: #9444
Status: newneeds_review

comment:4 Changed 7 years ago by cypherpunks

len is DWORD, it's unsigned. len should be int.
GetCurrentDirectory and GetEnvironmentVariable returns length of string not including nul-terminating character, but require output buffer with size including the null-terminating character. "n++" needs just after gets buffer size from those functions, else contents of buffer are undefined and no error return while fills buffer. Actually I can't get what values returned if NULL instead of buffer, it's unclear for me.

Last edited 7 years ago by cypherpunks (previous) (diff)

comment:5 in reply to:  4 ; Changed 7 years ago by dcf

Replying to cypherpunks:

len is DWORD, it's unsigned. len should be int.

You are right. I didn't know DWORD was unsigned. Compiling with -Wextra shows:

RelativeLink.c: In function ‘GetSubdirectory’:
RelativeLink.c:54:5: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
RelativeLink.c: In function ‘PrependToPath’:
RelativeLink.c:103:5: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

GetCurrentDirectory and GetEnvironmentVariable returns length of string not including nul-terminating character, but require output buffer with size including the null-terminating character.

I had to check this again because the interface is confusing. When these functions fail because the buffer is too small, they return the number of needed characters including the null terminator; i.e., the number of tchars you need to pass to malloc. When they succeed, they return the number of characters written not including the null terminator.
GetCurrentDirectory:

If the function succeeds, the return value specifies the number of characters that are written to the buffer, not including the terminating null character.
If the function fails, the return value is zero. To get extended error information, call GetLastError.
If the buffer that is pointed to by lpBuffer is not large enough, the return value specifies the required size of the buffer, in characters, including the null-terminating character.

GetEnvironmentVariable:

If the function succeeds, the return value is the number of characters stored in the buffer pointed to by lpBuffer, not including the terminating null character.
If lpBuffer is not large enough to hold the data, the return value is the buffer size, in characters, required to hold the string and its terminating null character and the contents of lpBuffer are undefined.
If the function fails, the return value is zero. If the specified environment variable was not found in the environment block, GetLastError returns ERROR_ENVVAR_NOT_FOUND.

See http://msdn.microsoft.com/en-us/library/windows/desktop/ms682009%28v=vs.85%29.aspx#example_2, where they pass the return value of GetEnvironmentVariable directly to malloc.

I'd like to know whether the TCHAR stuff looks right. I used TEXT on string literals, multiplied malloc arguments by sizeof(TCHAR), and used _tcslen and _sntprintf in place of strlen and snprintf. I don't know whether the program is getting compiled in _UNICODE mode or not, or how to check that.

I attached a new patch that uses int for len, adds a missing error check to the second calls to GetCurrentDirectory and GetEnvironmentVariable, and expresses the size of some memory allocations in a way that indicates what it going to be stored in them.

Changed 7 years ago by dcf

Attachment: bug10845-apart.patch added

Separate patches (git am style) with fixes from comment:4 and comment:5.

Changed 7 years ago by dcf

Attachment: bug10845-together.patch added

Combined patch with fixes from comment:4 and comment:5.

comment:6 in reply to:  5 Changed 7 years ago by cypherpunks

I'd like to know whether the TCHAR stuff looks right. I used TEXT on string literals, multiplied malloc arguments by sizeof(TCHAR), and used _tcslen and _sntprintf in place of strlen and snprintf. I don't know whether the program is getting compiled in _UNICODE mode or not, or how to check that.

I think it's right to do if planned to produce ANSI and UNICODE version of exe for RelativeLink.c. By default (if nothing defined) it builds as ANSI, and only side-effect is lack of support unicode-encoded paths if different code page.

If UNICODE version need then both UNICODE and _UNICODE should be defined before including windows specific headers (windows.h and tchar.h, as UNICODE for windows and _UNICODE for tchar). (At least for native mingw it fails to compile if only one defined.)

comment:7 Changed 7 years ago by mikeperry

Keywords: MikePerry201402R added

comment:8 Changed 7 years ago by mikeperry

We have had people complain in the past with issues when Unicode chars were used in path prefixes of the TBB. Do we want to build the unicode version of this for that reason?

Otherwise I think this looks good.

comment:9 in reply to:  8 Changed 7 years ago by dcf

Replying to mikeperry:

We have had people complain in the past with issues when Unicode chars were used in path prefixes of the TBB. Do we want to build the unicode version of this for that reason?

Otherwise I think this looks good.

I did try it in a path with a Chinese directory name, and something else failed before it got to the point where the pluggable transport would have started. (I just went to zh.wikipedia.org, copied some text, and pasted it as a directory name, if you want to try it.) Building it in Unicode mode seems like the right idea, but there might be other issues elsewhere.

comment:10 Changed 7 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, well I guess we may just need to file a different ticket for the unicode path issue then?

I went ahead and merged this as-is.

Note: See TracTickets for help on using tickets.