[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