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: UDPConn.ReadFrom returns error for oversized datagram on Windows #14074

Closed
fjl opened this issue Jan 22, 2016 · 13 comments
Closed

net: UDPConn.ReadFrom returns error for oversized datagram on Windows #14074

fjl opened this issue Jan 22, 2016 · 13 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@fjl
Copy link

fjl commented Jan 22, 2016

On all UNIXy platforms I've tried, ReadFrom(buf) consistently returns a nil error
and n == len(buf) when a datagram larger than buf arrives. Excess data is discarded
silently. On Windows, ReadFrom returns n == 0 and error WSAEMSGSIZE in this case
(see error table here).

This is an unfortunate inconsistency. The following test case fails on Windows
but succeeds everywhere else:

func TestWSAEMSGSIZE(t *testing.T) {
    listener, err := net.ListenPacket("udp", "127.0.0.1:0")
    if err != nil {
        t.Fatal(err)
    }
    defer listener.Close()
    sender, err := net.Dial("udp", listener.LocalAddr().String())
    if err != nil {
        t.Fatal(err)
    }
    defer sender.Close()

    sendN := 1800
    recvN := 300
    for i := 0; i < 20; i++ {
        go func() {
            buf := make([]byte, sendN)
            for i := range buf {
                buf[i] = byte(i)
            }
            sender.Write(buf)
        }()
        buf := make([]byte, recvN)
        listener.SetDeadline(time.Now().Add(1 * time.Second))
        n, _, err := listener.ReadFrom(buf)
        if err != nil {
            if nerr, ok := err.(net.Error); ok && nerr.Timeout() {
                continue
            }
            t.Fatal("unexpected read error:", err)
        }
        if n != recvN {
            t.Fatalf("short read: %d, want %d", n, recvN)
        }
        for i := range buf {
            if buf[i] != byte(i) {
                t.Fatal("error in pattern")
                break
            }
        }
    }
}

The trivial fix would be to add a check for WSAEMSGSIZE in the Windows implementation of recvfrom
and returning n == len(buf) and a nil error instead. Another fix could be to mark the error as temporary.

Is this something we want to fix? The UDPConn documentation does not dictate any particular behavior for oversized datagrams, so what happens on Windows is technically valid. Yet working around this issue
is very inconvenient and requires unpacking/comparing of errno and use of package syscall.

@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Jan 22, 2016
@mikioh
Copy link
Contributor

mikioh commented Jan 23, 2016

I don't think it's worth to hide error messages from the platform. Also I don't think it's a temporary error. I personally hope to leave this as it is. Because it is application responsibility to validate application-level packet integrity when the application uses UDP-like datagram protocols.

b := make([]byte, appLevelProtocolDefinedMaximumPayloadLength)
n, err := c.Read(b) // or c.ReadFrom(b)
if err != nil {
        // an error on protocol or protocol service access point occurred
}
if err := validateAppLevelProtocol(b[:n]); err != nil {
        // an error on application-level protocol occurred
}

FWIW, you may find it inconsistent to read a UDP datagram with zero-sized payload between platforms and/or kernel versions.

@bradfitz
Copy link
Contributor

I would prefer to be consistent and hide the error on Windows.

@mikioh
Copy link
Contributor

mikioh commented Jan 23, 2016

I'm not fine hiding errors, but feel configuring MSG_PARTIAL for WSARecv on UDP might be a compromise. Even in that case please make sure the behavior is consistent on both Read/Write-like generic ops and Read{From,Msg}/Write{To,Msg}-like specific ops between platforms.

@bradfitz
Copy link
Contributor

I'm not proposing hiding all errors. Just this specific one. If there's a flag to get unixy semantics, that works too.

@fjl
Copy link
Author

fjl commented Jan 23, 2016

Can we at least make it return non-zero n? The data is written to the buffer after all.

@alexbrainman
Copy link
Member

Can we at least make it return non-zero n?

Sounds reasonable to me. @fjl you want to send change when Go tree opens for go1.7 development?

Thank you.

Alex

@fjl
Copy link
Author

fjl commented Jan 30, 2016

Yes, I'll send a CL then.

@rsc rsc modified the milestones: Go1.8, Go1.7 May 18, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@rsc rsc modified the milestones: Go1.9, Go1.8 Nov 11, 2016
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jul 6, 2017
@rsc
Copy link
Contributor

rsc commented Nov 22, 2017

I think it is fine for someone to send a CL making this case return n, err instead of 0, err.
As far as I can see that has not happened, so moving to Go 1.11.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@gopherbot
Copy link

Change https://golang.org/cl/92475 mentions this issue: net: fix UDPConn.ReadFrom to return correct read data size

@m4ns0ur
Copy link
Contributor

m4ns0ur commented Feb 11, 2018

Just sent a CL to return n, err instead of 0, err.
Documentation already mentioned It returns the number of bytes copied into b. Do we need to add more details in documentation?

@alexbrainman
Copy link
Member

@m4ns0ur your CL 92475 is changing net.UDPConn.ReadFrom to return "correct read data size" when the function returns error. When I use any Go function, I expect returned values to be meaningless if error is returned. If returned values are meaningful while error is not nil, that is explicitly documented - for example io.Reader.Read function. Where does net.UDPConn.ReadFrom documentation describes how to use returned values when it returns error?

Alex

@m4ns0ur
Copy link
Contributor

m4ns0ur commented Feb 13, 2018

@alexbrainman it makes sense, thanks for the tip. Will update the CL.

@mikioh
Copy link
Contributor

mikioh commented Feb 14, 2018

I haven't look at your CL yet but please don't forget to consider that;

  • fixing net: document PacketConn IO operations in relation to packet sizes  #18056 at the same time if you want to change or define the new behavior of net.Conn and net.PacketConn interfaces,
  • UDPConn has three methods for per-packet basis reading; ReadFrom, ReadFromUDP and ReadMsgUDP,
  • IPConn also has three methods; ReadFrom, ReadFromIP and ReadMsgIP,
  • adding new test cases into net/{udpsock,iprawsock}_test.go (and net/error_test.go if necessary).

@golang golang locked and limited conversation to collaborators Feb 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Development

No branches or pull requests

9 participants