[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 10:09:50 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 yawning):

 (Ha, me going out?  Never)

 Commenting on the things that required changes.
 (`40af21e70672ee2dac74f08709d6043e93ffbe71`)

 Replying to [comment:18 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
 > }}}

 Fixed.

 > 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.

 Fixed.

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

 Fixed.

 > 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?

 Comment added.  I could/should provide a helper that formats the response
 in the way that is expected, but since UPnP is the only protocol that will
 support this interface, having the actual formatting live there is
 probably fine for now.

 > 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?

 There were 3 things that can be returned from that function:
  * `*requestMappingResp`
  * `*externalAddressResp`

 In all cases the opcode in the common header is validated to correspond to
 the request that was issued, and the calling code uses a type assertion
 then does what it needs to with the actual response.  I could change this
 to return a response interface, but I'll still need the type assertion(s),
 so I don't see it as buying much.

 I changed the return the raw response packet to return an error, since it
 was there as a catchall that I used during development.

 > 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.

 Comments added.

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

 Fixed.

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

 Fixed.

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


More information about the tor-bugs mailing list