[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #13338 [Tor]: Rewrite tor-fw-helper in Go (or another memory-safe language)
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs