Skip to content

net: broken sendfile on SmartOS/Solaris? large files garbled when sent over network #13892

Closed
Closed
net: broken sendfile on SmartOS/Solaris? large files garbled when sent over network#13892
Labels
FrozenDueToAgeOS-Solaris
Milestone
Go1.7

Description

@doublerebel
doublerebel
opened on Jan 9, 2016

What version of Go are you using (go version)? go version go1.5.1 solaris/amd64
What operating system and processor architecture are you using? SunOS 5.11 joyent_20141030T081701Z i86pc i386 i86pc Solaris, SunOS 5.11 joyent_20151002T091442Z i86pc i386 i86pc Solaris
What did you do? Retrieved a >250K file over http from a golang server
What did you expect to see? The correct contents of the file
What did you see instead? A file with the correct size, but the first ~25% of the file repeated to fill the file.

If I access the file through localhost, the file is served correctly. If I send it over any network interface, the bug appears. Different lengths of the file are repeated to fill the space in each case. All smaller files come through without error.

I attempted to step through using godebug but ran into mailgun/godebug#12, since the issue is clearly somewhere in the standard library. So I have setup a temporary fileserver to demonstrate the issue. text.text.2 contains the beginning of Sherlock Holmes from Project Gutenberg (thanks Norvig).

Control: python -m SimpleHTTPServer
http://165.225.174.156:8000/
http://165.225.174.156:8000/test.txt.2
Serves correct contents of file.

Bug: gohttp (itang/gohttp)
http://165.225.174.156:8080/
http://165.225.174.156:8080/test.txt.2
Serves garbled file, every time.

Localhost: gohttp with a barebones nginx proxy from localhost
http://165.225.174.156/
http://165.225.174.156/test.txt.2
Serves correct contents of file.

I discovered this bug because we just recently have hashicorp/consul building on SmartOS (hashicorp/consul#159), and the Consul UI contains a ~750K minified JS file. Consul uses gorilla/mux and http.FileServer, but I am able to reproduce with the much simpler case of gohttp.

The test suite for the http response methods doesn't have tests from a remote machine, or use large files, so this bug can slip through in most cases unnoticed. Please let me know if I can provide more details. We really need a way to step through and debug the standard libs!

Activity

  1. bradfitz commented on Jan 9, 2016

    @bradfitz
    bradfitz
    on Jan 9, 2016
    Contributor

    I suspect the sendfile implementation for Solaris is broken, then.

    Can you modify src/net/sendfile_solaris.go and add a return statement on the first line of func sendfile?

    func sendFile(c *netFD, r io.Reader) (written int64, err error, handled bool) {
    return

    And see if that fixes it? (it would revert to Read+Write)

    /cc @4ad

  2. bradfitz
    added this to the Go1.6Maybe milestone on Jan 9, 2016
  3. bradfitz
    added
    OS-Solaris
    on Jan 9, 2016
  4. bradfitz
    changed the title [-]net/http: large files are garbled when sent to a network interface on SmartOS/Solaris[/-] [+]net: broken sendfile on SmartOS/Solaris? large files garbled when sent over network[/+] on Jan 9, 2016

  5. doublerebel commented on Jan 9, 2016

    @doublerebel
    doublerebel
    on Jan 9, 2016
    Author

    @bradfitz You nailed it. I added a log line and the return statement, and I can now see the file being served correctly. Really appreciate the fast diagnosis.

  6. 4ad commented on Jan 9, 2016

    @4ad
    4ad
    on Jan 9, 2016
    Member

    Could you please try with the latest SmartOS image? Thanks.

  7. doublerebel commented on Jan 9, 2016

    @doublerebel
    doublerebel
    on Jan 9, 2016
    Author

    @4ad sure thing, tested both versions on the latest base-64 image: SunOS 5.11 joyent_20151002T091442Z i86pc i386 i86pc Solaris

    The current version fails as on the earlier SmartOS image, and the patched version suggested by @bradfitz works.

  8. 4ad commented on Jan 10, 2016

    @4ad
    4ad
    on Jan 10, 2016
    Member

    Just for confirmation, you tested on a recent SmartOS image, a.i. a recent kernel, not an older SmartOS image with a recent base-64 image?

  9. 4ad
    self-assigned this
    on Jan 10, 2016

  10. doublerebel commented on Jan 10, 2016

    @doublerebel
    doublerebel
    on Jan 10, 2016
    Author

    @4ad the tests were all run on instances in the Joyent Public Cloud, base-64 is the name of their default basic 64-bit SmartOS image. joyent_20151002T091442Z is SmartOS Base Image v15.3.0. I can donate an instance for testing, if that helps.

    EDIT: FWIW, I just rebuilt Consul with net/sendfile patched, and it now too works as expected.

  11. bradfitz commented on Jan 11, 2016

    @bradfitz
    bradfitz
    on Jan 11, 2016
    Contributor

    Perhaps we should just disable sendfile on Solaris for Go 1.6 as a safe option. We can re-enable in Go 1.7 once we understand and fix the problem.

  12. davecheney commented on Jan 11, 2016

    @davecheney
    davecheney
    on Jan 11, 2016
    Contributor

    I think that is a good idea

    On Tue, 12 Jan 2016, 05:57 Brad Fitzpatrick notifications@github.com
    wrote:

    Perhaps we should just disable sendfile on Solaris for Go 1.6 as a safe
    option. We can re-enable in Go 1.7 once we understand and fix the problem.

    —
    Reply to this email directly or view it on GitHub
    #13892 (comment).

  13. gopherbot
    added a commit that references this issue on Jan 11, 2016

    net: disable sendfile on Solaris for now

    ba593d6
  14. bradfitz
    modified the milestones: Go1.7, Go1.6Maybe on Jan 13, 2016

  15. bradfitz commented on Jan 13, 2016

    @bradfitz
    bradfitz
    on Jan 13, 2016
    Contributor

    Moved milestone to Go1.7. In Go 1.6, sendfile will just be disabled for Solaris and it will fall back to the path that works.

  16. bradfitz commented on Mar 31, 2016

    @bradfitz
    bradfitz
    on Mar 31, 2016
    Contributor

    Any news here?

  17. 4ad commented on Mar 31, 2016

    @4ad
    4ad
    on Mar 31, 2016
    Member

    +cc @binarycrusader

  18. binarycrusader commented on Apr 1, 2016

    @binarycrusader
    binarycrusader
    on Apr 1, 2016
    Contributor

    I'll look into this.

  19. binarycrusader commented on Apr 1, 2016

    @binarycrusader
    binarycrusader
    on Apr 1, 2016
    Contributor

    Have reproduced on Solaris development build using decompressed Mark.Twain-Tom.Sawyer.txt.bz2 from src/compress/bzip2/testdata.

  20. binarycrusader commented on Apr 1, 2016

    @binarycrusader
    binarycrusader
    on Apr 1, 2016
    Contributor

    It consistently fails after the first 262,144 characters (256KiB), where it starts to the repeat the input all over again.

  21. binarycrusader commented on Apr 8, 2016

    @binarycrusader
    binarycrusader
    on Apr 8, 2016
    Contributor

    So the root cause of this issue is that the implementation of sendfile() for Solaris is not correct. In particular, for sizes beyond the size of the socket buffer, the same data will be sent (data_len / socket_buffer_size) + (data_len % socket_buffer_size) times.

    This is only vaguely referenced in the Solaris sendfile(3c) man page. The FreeBSD sendfile(2) man page however makes this quite clear:

    When using a socket marked for non-blocking I/O, sendfile() may send
fewer bytes than requested.  In this case, the number of bytes success-
fully written is returned in *sbytes (if specified), and the error    EAGAIN
is returned.

    As a result, I believe the implementations for the following platforms may not actually be correct:

    • dragonfly
    • freebsd
    • solaris

    I am currently testing to determine whether any other platforms need a similar fix.

  22. binarycrusader commented on Apr 8, 2016

    @binarycrusader
    binarycrusader
    on Apr 8, 2016
    Contributor

    So my new test works on Solaris, but did not fail as expected on DragonFly/FreeBSD, despite the fact that they clearly do not account for the EAGAIN scenario. I would welcome any insight others might have to as to why it did not fail. The only guess I have is that the file is not big enough to trigger the failure on FreeBSD.

  23. gopherbot commented on Apr 9, 2016

    @gopherbot
    gopherbot
    on Apr 9, 2016
    Contributor

    CL https://golang.org/cl/21769 mentions this issue.

  24. gopherbot
    closed this as completedin 98080a6on Apr 12, 2016
  25. golang
    locked and limited conversation to collaborators on Apr 13, 2017
  26. gopherbot
    added
    FrozenDueToAge
    on Apr 13, 2017
  27. rsc
    unassigned
    4ad
    on Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    FrozenDueToAgeOS-Solaris

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions