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

Comments

Projects
None yet
9 participants
@fjl
Copy link

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Jan 23, 2016

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

@mikioh

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Jan 23, 2016

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

@fjl

This comment has been minimized.

Copy link
Author

commented Jan 23, 2016

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

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jan 29, 2016

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

This comment has been minimized.

Copy link
Author

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 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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

commented Feb 7, 2018

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

@m4ns0ur

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2018

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

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2018

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

  • fixing #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).

@gopherbot gopherbot closed this in 79fe895 Feb 21, 2018

@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.
You can’t perform that action at this time.