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: sendFile can't handle os.fileWithoutWriteTo #66988

Closed
philwo opened this issue Apr 23, 2024 · 3 comments
Closed

net: sendFile can't handle os.fileWithoutWriteTo #66988

philwo opened this issue Apr 23, 2024 · 3 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@philwo
Copy link
Contributor

philwo commented Apr 23, 2024

Go version

go version go1.22.2 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/philwo/Library/Caches/go-build'
GOENV='/Users/philwo/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/philwo/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/philwo/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.22.2/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.22.2/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.2'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/philwo/src/philtools/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/ft/3rpc4dkj09b3lw6pbm_j0j7w005v1x/T/go-build2443549281=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

  1. Open a regular file f for reading.
  2. Connect to a TCP socket s.
  3. Copy f into s by calling io.Copy(s, f)

Example: https://go.dev/play/p/xJbcSMVNLqy (deadlocks when run on Go Playground due to go.dev/issue/48394)

What did you see happen?

io.Copy does not use sendfile on macOS to copy f into s, but instead falls back to using a buffer due to a regression caused by a side-effect of go.dev/cl/472475.

Stepping through the code with a debugger, I think this is what happens:

io.Copy implements three different strategies to copy src to `dst:

  1. Call src.WriteTo(dst) if src implements the WriterTo interface.
  2. Call dst.ReadFrom(src) if dst implements the ReaderFrom interface.
  3. Use a buffer to handle the copying ourselves.

In the first iteration, this goes through os.File's WriteTo method, which calls into zero_copy_stub.go's writeTo, which is just a stub and thus can't do anything. It thus falls back to calling io.Copy again with itself wrapped into fileWithoutWriteTo, preventing it from taking the first branch.

In this second iteration, it thus calls ReadFrom on s and passes it f wrapped into fileWithoutWriteTo. tcpsock_posix.go attempts to use sendFile now, passing f into its r io.Reader parameter. sendFile needs to ensure that the passed Reader supports Stat, Seek and SyscallConn, and thus attempts a type assertion to *os.File.

However, this fails, because r isn't a File, it's a fileWithoutWriteTo.

Prior to go.dev/cl/472475 this all happened to work, because we never took the first branch in io.Copy, because os.File didn't implement io.WriterTo, so we passed the unmodified arg to the socket's ReadFrom.

What did you expect to see?

io.Copy should use sendfile on macOS to copy f into s, like it did prior to go.dev/cl/472475 (and still does on Linux, because os/zero_copy_linux.go handles it there and is not affected by the regression).

@gopherbot
Copy link

Change https://go.dev/cl/581035 mentions this issue: net: fix sendfile regression with io.Copy on macOS

@odeke-em
Copy link
Member

Thank you for this report and catching this regression @philwo! Indeed restricting the reader to be an *os.File deprived other use cases IMHO. One way to ensure that the fix is for ever caught and doesn't trivially regress is by using a test such as one that I have crafted down below inside a new file net/sendfile_unix_alt_test.go

// Copyright 2024 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

//go:build (darwin && !ios) || dragonfly || freebsd || solaris

package net

import (
	"errors"
	"io"
	"io/fs"
	"syscall"
	"testing"
	"time"
)

type sendFileMocker struct {
	cursor  int64
	content []byte

	seekInvoked        bool
	syscallConnInvoked bool
}

var _ fs.File = (*sendFileMocker)(nil)
var _ syscall.Conn = (*sendFileMocker)(nil)

func TestSendFileWorksOnNonOsFilesToo(t *testing.T) {
	ln, err := Listen("tcp", ":0")
	if err != nil {
		t.Fatal(err)
	}
	defer ln.Close()

	go func() {
		conn, err := ln.Accept()
		if err != nil {
			panic(err)
		}
		_, _ = io.Copy(io.Discard, conn)
	}()

	c, err := Dial("tcp", ln.Addr().String())
	if err != nil {
		t.Fatal(err)
	}
	defer c.Close()

	sfm := &sendFileMocker{content: []byte("abcdefghijklmno")}

	tcpConn := c.(*TCPConn)
	_, err, _ = sendFile(tcpConn.fd, sfm)
	if err == nil || err.Error() != "purposefully not available" {
		t.Fatal("Purposefully expected .SyscallConn to return an error")
	}

	// After this invocation, sfm's variables must be set to true.
	if !sfm.seekInvoked {
		t.Error(".Seek was not invoked!")
	}
	if !sfm.syscallConnInvoked {
		t.Error(".SyscallConn was not invoked!")
	}
}

func (sfm *sendFileMocker) SyscallConn() (syscall.RawConn, error) {
	sfm.syscallConnInvoked = true
	return nil, errors.New("purposefully not available")
}

func (sfm *sendFileMocker) Seek(offset int64, whence int) (int64, error) {
	sfm.seekInvoked = true
	if whence == io.SeekEnd {
		offset = -offset
	} else if whence == io.SeekStart {
		sfm.cursor = 0
	}
	sfm.cursor += offset
	return sfm.cursor, nil
}

func (sfm *sendFileMocker) Size() int64 {
	return int64(len(sfm.content))
}

func (sfm *sendFileMocker) Close() error {
	return nil
}

func (sfm *sendFileMocker) Stat() (fs.FileInfo, error) {
	return &mockFileInfo{sfm.Size()}, nil
}

type mockFileInfo struct{ size int64 }

var _ fs.FileInfo = (*mockFileInfo)(nil)

func (mfi *mockFileInfo) Name() string       { return "in-memory" }
func (mfi *mockFileInfo) Size() int64        { return mfi.size }
func (mfi *mockFileInfo) IsDir() bool        { return false }
func (mfi *mockFileInfo) Sys() any           { return nil }
func (mfi *mockFileInfo) Mode() fs.FileMode  { return 0 }
func (mfi *mockFileInfo) ModTime() time.Time { return time.Now() }

func (sfm *sendFileMocker) Read(b []byte) (int, error) {
	n := copy(b, sfm.content[sfm.cursor:])
	return n, nil
}

@gopherbot
Copy link

Change https://go.dev/cl/581778 mentions this issue: net, os, internal/poll: test for use of sendfile

gopherbot pushed a commit that referenced this issue Apr 26, 2024
The net package's sendfile tests exercise various paths where
we expect sendfile to be used, but don't verify that sendfile
was in fact used.

Add a hook to internal/poll.SendFile to let us verify that
sendfile was called when expected. Update os package tests
(which use their own hook mechanism) to use this hook as well.

For #66988

Change-Id: I7afb130dcfe0063d60c6ea0f8560cf8665ad5a81
Reviewed-on: https://go-review.googlesource.com/c/go/+/581778
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@dmitshur dmitshur added this to the Go1.23 milestone Apr 26, 2024
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants