[tor-bugs] #7956 [Tor]: Tor uses Roaming (remote) AppData, not Local

[Tor Bug Tracker & Wiki]

#7956: Tor uses Roaming (remote) AppData, not Local
-------------------------------------------------------------------------+--
 Reporter:  cypherpunks                                                  |          Owner:                    
     Type:  defect                                                       |         Status:  new               
 Priority:  major                                                        |      Milestone:  Tor: 0.2.4.x-final
Component:  Tor                                                          |        Version:  Tor: unspecified  
 Keywords:  tor-client win32 023-backport AppData Roaming Local Windows  |         Parent:                    
   Points:                                                               |   Actualpoints:                    
-------------------------------------------------------------------------+--
Changes (by nickm):

 * cc: erinn (added)


Comment:

 Longer reply:

 Replying to [ticket:7956 cypherpunks]:
 > [First tor-bug post, hope I got this right...]

 So far so good!  I'd recommend making an account if you're planning to
 stick around -- there are a lot of people who use the cypherpunks account,
 and it can get a little confusing when everybody's talking to each other
 with the same names.


  [...]
 > This bug was probably added to Tor in 0.2.1.11-alpha - 2009-01-20:

 Actually, that's not the case.  That commit was 87124f54d0bb84 (svn
 revision r18122), which added LOCAL_APPDATA as an option.  Previously, it
 was APPDATA unconditionally.

 Have you tried picking up git at all?  Once you know how, it's easy to use
 git to find out which commit actually introduced a change.

  [...]
 > First, this comment is wrong on many levels. It begins with "X:"
 (instead of  "C:") which would likely be remote, not local. It talks about
 IE4 which shipped aeons ago, maybe time to revisit this hesitation.  And
 you you *do* use this COM API, anyway.

 Yeah; we used to care about some really ancient versions of Windows when
 we wrote the code.  The "X" there was probably meant as "whatever drive it
 happens to be on".

 > I am pretty sure that it is the above definitions that're causing you to
 install the Roaming data location on my Win7 box.
 >
 >   CSIDL_LOCAL_APPDATA == FOLDERID_LocalAppData
 >
 >   CSIDL_APPDATA == FOLDERID_RoamingAppData
 >
 > I am not great at Autotools, but I don't believe anyone is setting
 > ENABLE_LOCAL_APPDATA, so the directive might be 0. It is defined/used in
 config.ac and orconfig.h.in.

 It'll get set if --enable-local-appdata is passed to autoconf.

 > ----snip----
 > /* Defined if we default to host local appdata paths on Windows */
 > #undef ENABLE_LOCAL_APPDATA
 > ----snip----
 >
 > I don't understand why this Windows-centric feature needs to be
 abstracted with GNU/autotools in the first place. Can Erin's Mac OSX-
 based, MinGW cross-compiled Autotools determine that I'm running WinXP or
 Win7, etc? This abstraction, using legacy APIs, is probably why this bug
 has been there since 2009.

 It's not autotools that causing the issue; it's the lack of a migration
 plan when we added the fix from making it "optional, off by default" to
 "on by default."

 > Tor already abstracts this Windows-centric code into it's own function,
 it would be clearer if you removed all the autotools absraction to the
 Windows Special Folder COM API that Tor uses, IMO.
 >
 > Also, this function doesn't check this return code, and assumes to be
 true:
 >
 >   strlcpy(p,get_windows_conf_root(),MAX_PATH);

 okay, better fix that one when we change this.

 > Also, in this Windows-centric error codepath, there is no warning to the
 > user, AFAICT:
 >
 >   if (!SUCCEEDED(result)) {
 >     return NULL;
 >   }

 Agreed; there should be one.

 > Also, elsewhere in get_windows_conf_root(), there are 2 comments, with
 potentially-open questions. Here are updated comments stated more clearly.
 Replace:
 >
 >  /* Convert the path from an "ID List" (whatever that is!) to a path. */
 >
 > with:
 >
 > /* Windows Explorer, aka 'Windows 'shell', uses IDLists to access
 various things, including special folders. We need to convert this COM-
 based explorer-centric array list to a Windows path. */
 >
 > And replace:
 >
 >  /* Now we need to free the memory that the path-idl was stored in.
 >   * In typical Windows fashion, we can't just call 'free()' on it. */
 >
 > with:
 >
 > /* Windows explorer special folders APIs are COM APIs, not Win32 or C
 runtime library calls. To use special folders, we have to use the COM
 memory mgmt APIs, to allocate, and now release that memory. Read the SDK
 ShObj.h or MSDN for more info. */

 Those seem sensible; getting the changes in patch format would be great.

 > Note also that you are using legacy, deprecated version of the Special
 folders API, there have been revisions, apparently during Vista and Win7.
 The newer API has more features than the one you're using, including the
 local/roaming granularity. I think f you explicitly stick to local data, I
 think you can stick with legacy APIs.

 I think we will want to; we're still supporting XP at least.

 > You can't just test this with one version of Windows, you'll need to
 test with each version you are supporting.

 agreed

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7956#comment:2>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online