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

net: parseProcNetIGMP is broken on linux-2.6 #2826

Closed
gopherbot opened this issue Feb 1, 2012 · 8 comments
Closed

net: parseProcNetIGMP is broken on linux-2.6 #2826

gopherbot opened this issue Feb 1, 2012 · 8 comments
Milestone

Comments

@gopherbot
Copy link

by borman@google.com:

The parseProcNetIGMP function in pkg/net/interface_linux.go is broken.  It incorrectly
parses each line into fields and then uses wrong heuristics to determine if this is a
new interface line or not.

What steps will reproduce the problem?
1. Create a tap interface whose name is length of 10
2. run "make test" inside of pkg/net
3. watch panic

What is the expected output?

Not a panic

What do you see instead?

a panic

Which compiler are you using (5g, 6g, 8g, gccgo)?

6g (does not matter)

Which operating system are you using?

Linux

Which revision are you using?  (hg identify)

f79343c8a479

Please provide any additional information below.

The problem is that the code naively thinks there will be 5 whitespace separated words
on a line that contains an interface name.  This is not the format of this line.  It has
2 words, a colon, and then 2 more words.  The colon may or may not have whitespace in
front of it.  For interface names of length 10 (or greater?) there are only 4 whitespace
fields since the colon nestles up to the interface name.

The quick solution is to add the following after the "case 4:" in
parseProcNetIGMP:

                        if f[1][len(f[1])-1] ==
                                name = f[1]
                                continue
                        }

A more correct fix would be to properly parse this file.  If the first character is not
a space then the line defines an interface, otherwise it does not.  For example:

                if len(l) > 0 && l[0] == ' ' {
                        if len(f) == 4 {
                                if ifi == nil || name == ifi.Name {
                                        fmt.Sscanf(f[0], "%08x", &b)
                                        ifma := IPAddr{IP: IPv4(b[3], b[2], b[1], b[0])}
                                        ifmat = append(ifmat, ifma.toAddr())
                                }
                        }
                } else if f[1][len(f[1])-1] == ':' {
                        name = f[1][:len(f[1])-1]
                } else {
                        name = f[1]
                }

This also demonstrates a problem with the networking tests.  They attempt to use live
system data for the tests.  This is not wise as it only tests what is configured for the
system.  For example, this function should start:

func parseProcNetIGMP(ifi *Interface) []Addr {
        fd, err := open("/proc/net/igmp")
        if err != nil {
                return nil
        }
        defer fd.close()
        return _parseProcNetIGMP(ifi, fd)
}

func _parseProcNetIGMP(ifi *Interface, fd *file) []Addr {

However the use of the internal concreate type "file" rather than an interface
does make testing more difficult.  The parse.go package really should be refactored to
allow proper and complete testing.
@mikioh
Copy link
Contributor

mikioh commented Feb 2, 2012

Comment 1:

Thank you for the report.
I just tried to repro the issue w/ 855fc8e73259 tip on ubuntu-11.10 
but failed to get a panic. Could you pls show a good output example 
of /proc/net/igmp for repro?
mine% cat /proc/net/igmp
Idx Device    : Count Querier   Group    Users Timer    Reporter
1   lo        :     1      V3
                010000E0     1 0:00000000       0
2   eth0      :     1      V3
                010000E0     1 0:00000000       0
5   br0       :     2      V2
                FB0000E0     1 0:00000000       1
                010000E0     1 0:00000000       0
6   taptaptaptaptap:     2      V3
                FB0000E0     1 0:00000000       0
                010000E0     1 0:00000000       0
8   tap1234567:     2      V3
                FB0000E0     1 0:00000000       0
                010000E0     1 0:00000000       0
P.S. please don't hesitate to submit a fix patch. ;)

@mikioh
Copy link
Contributor

mikioh commented Feb 2, 2012

Comment 2:

Owner changed to @mikioh.

@gopherbot
Copy link
Author

Comment 3 by borman@google.com:

I will look at a solution to allow proper testing.
That table should have cause the panic.  I am not sure why you didn't see
it.  Perhaps it is also interface name sensitive.  I will do the ability to
test it first, reproduce the error, and then work on a fix for the error.
    -Paul

@gopherbot
Copy link
Author

Comment 4 by borman@google.com:

Please look at CL 5617051
This CL demonstrates the problem and the fix, but as I have stated in the
CL description I am having new failures:
--- FAIL: TestListenMulticastUDP (0.00 seconds)
multicast_test.go:126: IPv4 multicast interface: <nil>
multicast_test.go:136: IPv4 multicast TTL: 1
multicast_test.go:146: IPv4 multicast loopback: false
multicast_test.go:82: "224.0.0.254:12345" not found in RIB
These failures are not related to the changes I made.  It is too much to
determine why this test is now failing.
I think the redesign of the file type will allow much faster and better
testing of the various functions by allowing you to use a bytes.Buffer
instead of opening a file that may or may not have what you need to test in
it.  The refactoring of parseProcNetIGMP into parseProcNetIGMP and
_parseProcNetIGMP demonstrates how to make functions like this testable.
This CL is not intended to be submitted as is.  At a minimum the
_oldParseProcNetIGMP function must be removed.  This is only provided to
demonstrate the parsing problem.  Also, the igmp_test.go test should only
be run on linux but I do not know the magic at the moment to make this
happen.  That can be addressed if we want to keep the test around.
Many other functions in net should be refactored to allow solid testing.

@mikioh
Copy link
Contributor

mikioh commented Feb 8, 2012

Comment 5:

Status changed to Accepted.

@mikioh
Copy link
Contributor

mikioh commented Feb 9, 2012

Comment 6:

I think I've succeeded to repro the issue.
% tunctl -t device1tap2
% ifconfig device1tap2 1.2.3.4
% go test -v net
Without crash on linux-3.0.0:
    interface_test.go:46: "device1tap2": flags "up|broadcast|multicast", ifindex 6, mtu 1500
    interface_test.go:47:   hardware address "aa:bb:cc:dd:ee:ff"
    interface_test.go:82:   interface address "1.2.3.4/8"
    interface_test.go:93:   joined group address "ff02::1"
With crash on linux-2.6.32:
    testing.go:266: runtime error: index out of range
    testing.go:236: /home/mikioh/go/src/pkg/runtime/proc.c:1388 (0x411172)
    testing.go:237:     runtime.panic
    testing.go:236: /home/mikioh/go/src/pkg/runtime/runtime.c:128 (0x411df5)
    testing.go:237:     runtime.panicstring
    testing.go:236: /home/mikioh/go/src/pkg/runtime/runtime.c:85 (0x411c9c)
    testing.go:237:     runtime.panicindex
    testing.go:236: /home/mikioh/go/src/pkg/net/interface_linux.go:193 (0x435bf3)
    testing.go:237:     net.parseProcNetIGMP
    testing.go:236: /home/mikioh/go/src/pkg/net/interface_linux.go:169 (0x435846)
    testing.go:237:     net.interfaceMulticastAddrTable
    testing.go:236: /home/mikioh/go/src/pkg/net/interface.go:153 (0x433ec7)
    testing.go:237:     net.(*Interface).MulticastAddrs
    testing.go:236: /home/mikioh/go/src/pkg/net/interface_test.go:71 (0x451bc2)
    testing.go:237:     net.testInterfaceMulticastAddrs
    testing.go:236: /home/mikioh/go/src/pkg/net/interface_test.go:49 (0x451877)
    testing.go:237:     net.TestInterfaces
    testing.go:236: /home/mikioh/go/src/pkg/testing/testing.go:274 (0x421f6d)
    testing.go:237:     testing.tRunner
    testing.go:236: /home/mikioh/go/src/pkg/runtime/proc.c:258 (0x40f2e3)
    testing.go:237:     runtime.goexit

Labels changed: added priority-go1, removed priority-triage.

@mikioh
Copy link
Contributor

mikioh commented Feb 10, 2012

Comment 7:

Owner changed to ---.

@mikioh
Copy link
Contributor

mikioh commented Feb 22, 2012

Comment 8:

This issue was closed by revision 9765325.

Status changed to Fixed.

@rsc rsc added this to the Go1 milestone Apr 10, 2015
@rsc rsc removed the priority-go1 label Apr 10, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants