[tor-bugs] #13338 [Tor]: Rewrite tor-fw-helper in Go (or another memory-safe language)

Tor Bug Tracker & Wiki blackhole at torproject.org
Fri Oct 31 05:52:17 UTC 2014


#13338: Rewrite tor-fw-helper in Go (or another memory-safe language)
-----------------------------+------------------------------
     Reporter:  arma         |      Owner:  yawning
         Type:  enhancement  |     Status:  needs_review
     Priority:  minor        |  Milestone:  Tor: unspecified
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  flashproxy
Actual Points:               |  Parent ID:  #5213
       Points:               |
-----------------------------+------------------------------

Comment (by dcf):

 Here are my comments and questions from reading the code. Aside, it's
 stupendous what you've singlehandedly accomplished with this code.

 Typo: "failed to external address"
 {{{
 $ ./go-fw-helper -v -l
 ...
 V: NAT-PMP: failed to external address: connection timed out
 }}}

 It's nice if [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/natclient/natclient.go#L31
 natclient.New]'s comment describes legal values for `protocol`; i.e.
 `"UPnP"` and `"NAT-PMP"`, and that "unspecified" means an empty string. On
 that note, [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/main.go#L93 main.go]
 has
 {{{
         protocol := "auto"
 }}}
 But `"auto"` appears not to have any special meaning, and anyway it is
 overriden with an empty string by flag.StringVar.

 Define what [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/main.go#L26
 mappingDuration = 0] means in main.go.

 What's the return value of [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/natclient/base/base.go#L46
 ListOfPortMappings]? What does each individual returned string look like?

 I was confused by this in [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/natclient/natpmp/packet.go
 natclient/natpmp/packet.go]:
 {{{
         opRespOffset = 128
         ...
         if h.op != opExternalAddress+opRespOffset {
 }}}
 I think it would be more clear that you are checking the most-significant
 bit if it were
 {{{
         opRespFlag = 0x80
         ...
         if h.op != opExternalAddress|opRespFlag {
 }}}

 Why does [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/natclient/natpmp/packet.go#L229
 issueRequest] return `interface{}` instead of `*requestMappingResp`? It
 looks like it can return some raw byte contents if an unexpected response
 is received, is that weird?

 What is this stuff at the bottom of some files?
 {{{
 var _ packetReq = (*externalAddressReq)(nil)
 var _ packetReq = (*requestMappingReq)(nil)
 }}}

 What happens in [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/natclient/natpmp/getgateway_windows.go
 natclient/natpmp/getgateway_windows.go] if the primary route is IPv6? It
 looks like it at least won't crash; does it still do anything meaningful?

 The use of [https://golang.org/pkg/unsafe/ unsafe] in
 [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/natclient/natpmp/getgateway_windows.go
 natclient/natpmp/getgateway_windows.go] looks okay to me. Maybe add MSDN
 links for the data structures and functions you use.

 Typoed function name [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/natclient/upnp/ssdp.go#L317
 retreiveDeviceDescription].

 Typo: "[https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/natclient/upnp/igd.go#L154
 IDG2]".

 Technically, it seems like [https://github.com/Yawning/go-fw-
 helper/blob/4ffcbdde7063ac343e7f02a3e31c7243ec0f2bad/natclient/upnp/httpu/httpu.go
 httpu] should do [https://golang.org/pkg/net/http/#RoundTripper RoundTrip]
 rather than [https://golang.org/pkg/net/http/#Client.Do Do], because
 RoundTrip is for a single transaction and Do "follows policy (e.g.
 redirects, cookies, auth)." That is, you might have to do less HTTP if you
 introduce the UDP stuff at a lower layer (RoundTripper rather than
 Client). But if it's at all awkward to do, then it doesn't matter; I'm
 sure what's there will work perfectly well.

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


More information about the tor-bugs mailing list