Opened 3 years ago

Last modified 11 months ago

#15826 needs_revision defect

Check and return error values in goptlib

Reported by: gsathya Owned by: asn
Priority: Medium Milestone:
Component: Obfuscation/Pluggable transport Version:
Severity: Normal Keywords: goptlib
Cc: dcf Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Make goptlib check and return error values of functions, instead of silently ignoring them.

git.torproject.org/pluggable-transports/goptlib.git/pt.go:557:15        defer f.Close()
git.torproject.org/pluggable-transports/goptlib.git/pt.go:629:16        io.WriteString(h, "ExtORPort authentication server-to-client hash")
git.torproject.org/pluggable-transports/goptlib.git/pt.go:630:9 h.Write(clientNonce)
git.torproject.org/pluggable-transports/goptlib.git/pt.go:631:9 h.Write(serverNonce)
git.torproject.org/pluggable-transports/goptlib.git/pt.go:638:16        io.WriteString(h, "ExtORPort authentication client-to-server hash")
git.torproject.org/pluggable-transports/goptlib.git/pt.go:639:9 h.Write(clientNonce)
git.torproject.org/pluggable-transports/goptlib.git/pt.go:640:9 h.Write(serverNonce)
git.torproject.org/pluggable-transports/goptlib.git/pt.go:857:15        s.SetDeadline(time.Now().Add(5 * time.Second))
git.torproject.org/pluggable-transports/goptlib.git/pt.go:868:15        s.SetDeadline(time.Time{})

Child Tickets

Attachments (1)

0001-Return-error-values.patch (5.8 KB) - added by gsathya 3 years ago.

Download all attachments as: .zip

Change History (5)

Changed 3 years ago by gsathya

comment:1 in reply to:  description ; Changed 3 years ago by dcf

Keywords: goptlib added
Status: newneeds_revision

git.torproject.org/pluggable-transports/goptlib.git/pt.go:629:16 io.WriteString(h, "ExtORPort authentication server-to-client hash")
git.torproject.org/pluggable-transports/goptlib.git/pt.go:630:9 h.Write(clientNonce)
git.torproject.org/pluggable-transports/goptlib.git/pt.go:631:9 h.Write(serverNonce)
git.torproject.org/pluggable-transports/goptlib.git/pt.go:638:16 io.WriteString(h, "ExtORPort authentication client-to-server hash")
git.torproject.org/pluggable-transports/goptlib.git/pt.go:639:9 h.Write(clientNonce)
git.torproject.org/pluggable-transports/goptlib.git/pt.go:640:9 h.Write(serverNonce)

These are deliberate. Hash.Write "never returns an error."

git.torproject.org/pluggable-transports/goptlib.git/pt.go:857:15 s.SetDeadline(time.Now().Add(5 * time.Second))
git.torproject.org/pluggable-transports/goptlib.git/pt.go:868:15 s.SetDeadline(time.Time{})

These ones look good.

git.torproject.org/pluggable-transports/goptlib.git/pt.go:557:15 defer f.Close()

I think I would like this better as

        defer func() {
                ferr := f.Close()
                if err == nil {
                        err = ferr
                }
        }()

Please add tests with a mock Closer that fails. Add test cases for:

  • readAuthCookie succeeds, Close fails.
  • readAuthCookie fails, Close fails (e.g., readAuthCookie gets a bytes.Reader that is too short; Close error should not overwrite readAuthCookie error)

Please make a new patch with just the SetDeadline and Close changes.

Last edited 3 years ago by dcf (previous) (diff)

comment:2 in reply to:  1 Changed 3 years ago by dcf

Replying to dcf:

git.torproject.org/pluggable-transports/goptlib.git/pt.go:857:15 s.SetDeadline(time.Now().Add(5 * time.Second))
git.torproject.org/pluggable-transports/goptlib.git/pt.go:868:15 s.SetDeadline(time.Time{})

These ones look good.

Now that I look again, these need to s.Close() in the error case.

comment:3 Changed 3 years ago by dcf

As for adding a test for a failed Close in readAuthCookieFile, I realized that doesn't make sense because readAuthCookieFile takes a string filename, not an io.ReadCloser. But I still hate to add error-handling code that doesn't have a test, or even ever been run.

One way is to expose an intermediate level of abstraction for the purpose of testing:

  • pt.go

    a b func readAuthCookie(f io.Reader) ([]byte, error) { 
    546546
    547547       return cookie, nil
    548548}
    549549
     550func readAndCloseAuthCookie(f io.ReadCloser) ([]byte, error) {
     551       defer f.Close()
     552       return readAuthCookieFile(f)
     553}
     554
    550555// Read and validate the contents of an auth cookie file. Returns the 32-byte
    551556// cookie. See section 4.2.1.2 of pt-spec.txt.
    552557func readAuthCookieFile(filename string) ([]byte, error) {
    553558       f, err := os.Open(filename)
    554559       if err != nil {
    555560               return nil, err
    556561       }
    557        defer f.Close()
    558 
    559        return readAuthCookie(f)
     562       return readAndCloseAuthCookie(f)
    560563}
    561564
    562565// This structure is returned by ServerSetup. It consists of a list of
    563566// Bindaddrs, an address for the ORPort, an address for the extended ORPort (if

Then you can write a test that passes a mock ReadCloser to readAndCloseAuthCookie:

// An io.ReadCloser that returns the given error on Close.
type errorReadCloser struct {
        io.Reader
        err error
}

func (rc *errorReadCloser) Close() error {
        return rc.err
}

func TestAuthCookieCloseError(t *testing.T) {
        closeErr := errors.New("failed Close")
        // Error reading cookie, also error in Close. The Close error should not
        // shadow the read error.
        _, err := readAndCloseAuthCookie(&errorReadCloser{bytes.NewReader([]byte("")), closeErr})
        if err == closeErr {
                t.Errorf("readAndCloseAuthCookie allowed Close error to shadow other error")
        }
        // Error in Close after reading cookie.
        _, err = readAndCloseAuthCookie(&errorReadCloser{bytes.NewReader([]byte("! Extended ORPort Auth Cookie !\x0a0123456789ABCDEF0123456789ABCDEF")), closeErr})   
        if err != closeErr {
                t.Errorf("readAndCloseAuthCookie did not report Close error: %v", err)
        }
}

comment:4 Changed 11 months ago by teor

Severity: Normal

Set all open tickets without a severity to "Normal"

Note: See TracTickets for help on using tickets.