Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syscall: ParseRoutingSockaddr crushes notified network addresses #8203

Closed
gopherbot opened this issue Jun 13, 2014 · 10 comments
Closed

syscall: ParseRoutingSockaddr crushes notified network addresses #8203

gopherbot opened this issue Jun 13, 2014 · 10 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 13, 2014

by p@google.com:

What does 'go version' print?

go version devel +0f7c69d6c367 Mon May 12 17:19:02 2014 -0400 darwin/amd64

What steps reproduce the problem?

Parse RTM_NEWADDR messages on a mac.

If possible, include a link to a program on play.golang.org. (I attached one as a file)

1.  read the PF_ROUTE socket
2.  parse messages using syscall.ParseRoutingMessage and syscall.ParseRoutingSockaddr
3.  turn wifi off and on


What happened?

You'll get back a netmask and a bogus address.
The pkg code is wrong, see fix below.

What should have happened instead?

You get back a netmask and a good address.

Please provide any additional information below.

go/src/pkg/syscall/route_bsd.go:156

was:
        if m.Header.Addrs&rtaIfaMask&(1<<i) == 0 {

should be:
        if m.Header.Addrs&(1<<i) == 0 {

Otherwise the subsequent loop gets confused about what address it is parsing, i.e, it
doesn't skip over the ones its trying to ignore.   Easy fix.

Attachments:

  1. broken.go (1474 bytes)
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jun 13, 2014

Comment 1 by p@google.com:

Looking more closely, the same bug also exists in:
route_darwin.go, route_dragonfly.go and route_freebsd.go.
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 13, 2014

Comment 2:

Labels changed: added repo-main, release-go1.4.

@mikioh
Copy link
Contributor

@mikioh mikioh commented Jun 14, 2014

Comment 3:

I'm not sure what you are suggesting. So which are you suggesting; a)
ParseRoutingSockaddr should parse SockaddrDatalink, b) ParseRoutingSockaddr should not
parse prefix state change announcements that hold only prefix mask, c) both of a and b,
d) other.
I'm fine with (a) but am not keen on other.
P.S. I'd recommend you to capture not only RTM_{NEW,DEL}ADDR but RTM_{NEW,DEL}ROUTE for
dealing with network facility (especially link and its belongings) changes on Darwin. On
other BSDs, interface announcements (a series of IFAN_XXX messages) would be great help.

Attachments:

  1. soso.go (1894 bytes)
@mikioh
Copy link
Contributor

@mikioh mikioh commented Jun 14, 2014

Comment 4:

Ah, I now see your point. At least ParseRoutingSockaddr should not crush the notified
network addresses.
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jun 18, 2014

Comment 6:

CL https://golang.org/cl/105220043 mentions this issue.
@mikioh
Copy link
Contributor

@mikioh mikioh commented Jun 18, 2014

Comment 7:

i've just sent a cl for fixing this issue. pls have a look at
https://golang.org/cl/105220043 if you have a spare time.
> Easy fix.
it needs a bit more work for parsing conventional bsd routing messages and socket
addresses.
@rsc
Copy link
Contributor

@rsc rsc commented Oct 28, 2014

Comment 8:

mikioh, what happened with this? It looks like the CL 105220043 was deleted because it
was in syscall. Did go.sys get the fix?

Status changed to Accepted.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 28, 2014

Comment 9:

CL https://golang.org/cl/164140044 mentions this issue.
@rsc
Copy link
Contributor

@rsc rsc commented Oct 28, 2014

Comment 10:

This issue was closed by revision 96e9e81.

Status changed to Fixed.

@mikioh
Copy link
Contributor

@mikioh mikioh commented Oct 29, 2014

Comment 11:

thanks for the fix. seems like there are still a few bugs related to the kernel-internal
form which is similar to nlri encoding. fortunately it won't hurt the net package and
its network interface api because looks like nlri-like encoding is used for both
unicast/multicast route messages and network-layer adj. messages. perhaps we might need
to encourage people not to use ParseRoutingMessage for scraping various routing messages.
ps: i now have no plan to contribute for go.sys.
mikioh pushed a commit that referenced this issue Feb 20, 2015
This changes fixes two issues with regard to handling routing messages
as follows:
- Misparsing on platforms (such as FreeBSD) supporting multiple
  architectures in the same kernel (kern.supported_archs="amd64 i386")
- Misparsing with unimplemented messages such as route, interface
  address state notifications

To fix those issues, this change implements all the required socket
address parsers, adds a processor architecture identifying function to
FreeBSD and tests.

Fixes #9707.
Fixes #8203.

Change-Id: I7ed7b4a0b6f10f54b29edc681a2f35603f2d8d45
Reviewed-on: https://go-review.googlesource.com/4330
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman added a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
No easy way to test (would have to actually trigger some routing
events from kernel) but the code is clearly wrong as written.
If the header says there is a submessage, we need to at least
skip over its bytes, not just continue to the next iteration.

Fixes golang#8203.

LGTM=r
R=r
CC=golang-codereviews, mikioh.mikioh, p
https://golang.org/cl/164140044
wheatman added a commit to wheatman/go-akaros that referenced this issue Jun 26, 2018
No easy way to test (would have to actually trigger some routing
events from kernel) but the code is clearly wrong as written.
If the header says there is a submessage, we need to at least
skip over its bytes, not just continue to the next iteration.

Fixes golang#8203.

LGTM=r
R=r
CC=golang-codereviews, mikioh.mikioh, p
https://golang.org/cl/164140044
wheatman added a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
No easy way to test (would have to actually trigger some routing
events from kernel) but the code is clearly wrong as written.
If the header says there is a submessage, we need to at least
skip over its bytes, not just continue to the next iteration.

Fixes golang#8203.

LGTM=r
R=r
CC=golang-codereviews, mikioh.mikioh, p
https://golang.org/cl/164140044
wheatman added a commit to wheatman/go-akaros that referenced this issue Jul 20, 2018
No easy way to test (would have to actually trigger some routing
events from kernel) but the code is clearly wrong as written.
If the header says there is a submessage, we need to at least
skip over its bytes, not just continue to the next iteration.

Fixes golang#8203.

LGTM=r
R=r
CC=golang-codereviews, mikioh.mikioh, p
https://golang.org/cl/164140044
wheatman added a commit to wheatman/go-akaros that referenced this issue Jul 30, 2018
No easy way to test (would have to actually trigger some routing
events from kernel) but the code is clearly wrong as written.
If the header says there is a submessage, we need to at least
skip over its bytes, not just continue to the next iteration.

Fixes golang#8203.

LGTM=r
R=r
CC=golang-codereviews, mikioh.mikioh, p
https://golang.org/cl/164140044
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.