From 47e3480d496c8e2e3a84c7551a6f7c0b1074c838 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Jun 2023 11:24:30 -0400 Subject: [PATCH 1/7] feat: return better error if file size is too big to upload --- provisionersdk/archive.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/provisionersdk/archive.go b/provisionersdk/archive.go index ec496b6f31592..2b6eb2041900d 100644 --- a/provisionersdk/archive.go +++ b/provisionersdk/archive.go @@ -54,6 +54,7 @@ func Tar(w io.Writer, directory string, limit int64) error { ) } + fileTooBigError := xerrors.Errorf("Archive too big. Must be <= %d bytes", limit) err = filepath.Walk(directory, func(file string, fileInfo os.FileInfo, err error) error { if err != nil { return err @@ -95,6 +96,10 @@ func Tar(w io.Writer, directory string, limit int64) error { if !fileInfo.Mode().IsRegular() { return nil } + // Before we even open the file, check if it is going to exceed our limit. + if fileInfo.Size()+totalSize >= limit { + return fileTooBigError + } data, err := os.Open(file) if err != nil { return err @@ -106,7 +111,7 @@ func Tar(w io.Writer, directory string, limit int64) error { } totalSize += wrote if limit != 0 && totalSize >= limit { - return xerrors.Errorf("Archive too big. Must be <= %d bytes", limit) + return fileTooBigError } return data.Close() }) From b34e9d8aaf92d2e0d9f7a297227a3ce7fc024793 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Jun 2023 12:19:45 -0400 Subject: [PATCH 2/7] Fix conditional --- provisionersdk/archive.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/provisionersdk/archive.go b/provisionersdk/archive.go index 2b6eb2041900d..8312af7de2152 100644 --- a/provisionersdk/archive.go +++ b/provisionersdk/archive.go @@ -97,7 +97,7 @@ func Tar(w io.Writer, directory string, limit int64) error { return nil } // Before we even open the file, check if it is going to exceed our limit. - if fileInfo.Size()+totalSize >= limit { + if fileInfo.Size()+totalSize > limit { return fileTooBigError } data, err := os.Open(file) @@ -110,7 +110,7 @@ func Tar(w io.Writer, directory string, limit int64) error { return err } totalSize += wrote - if limit != 0 && totalSize >= limit { + if limit != 0 && totalSize > limit { return fileTooBigError } return data.Close() From 0cd070ca085e61a2885ed8902b051f275bb06b7c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Jun 2023 12:31:52 -0400 Subject: [PATCH 3/7] Use a limit writer to capture actual tar size --- coderd/util/xio/limitwriter.go | 43 +++++++++ coderd/util/xio/limitwriter_test.go | 136 ++++++++++++++++++++++++++++ provisionersdk/archive.go | 21 ++--- 3 files changed, 189 insertions(+), 11 deletions(-) create mode 100644 coderd/util/xio/limitwriter.go create mode 100644 coderd/util/xio/limitwriter_test.go diff --git a/coderd/util/xio/limitwriter.go b/coderd/util/xio/limitwriter.go new file mode 100644 index 0000000000000..8357d5d97a5ca --- /dev/null +++ b/coderd/util/xio/limitwriter.go @@ -0,0 +1,43 @@ +package xio + +import ( + "io" + + "golang.org/x/xerrors" +) + +var ErrLimitReached = xerrors.Errorf("i/o limit reached") + +// LimitWriter will only write bytes to the underlying writer until the limit is reached. +type LimitWriter struct { + Limit int64 + N int64 + W io.Writer +} + +func NewLimitWriter(w io.Writer, n int64) *LimitWriter { + // If anyone tries this, just make a 0 writer. + if n < 0 { + n = 0 + } + return &LimitWriter{ + Limit: n, + N: 0, + W: w, + } +} + +func (l *LimitWriter) Write(p []byte) (int, error) { + if l.N >= l.Limit { + return 0, ErrLimitReached + } + + // Write 0 bytes if the limit is to be exceeded. + if int64(len(p)) > l.Limit-l.N { + return 0, ErrLimitReached + } + + n, err := l.W.Write(p) + l.N += int64(n) + return n, err +} diff --git a/coderd/util/xio/limitwriter_test.go b/coderd/util/xio/limitwriter_test.go new file mode 100644 index 0000000000000..4c6adefa9c0f1 --- /dev/null +++ b/coderd/util/xio/limitwriter_test.go @@ -0,0 +1,136 @@ +package xio_test + +import ( + "bytes" + cryptorand "crypto/rand" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/util/xio" +) + +func TestLimitWriter(t *testing.T) { + type writeCase struct { + N int + ExpN int + Err bool + } + + // testCases will do multiple writes to the same limit writer and check the output. + testCases := []struct { + Name string + L int64 + Writes []writeCase + N int + ExpN int + }{ + { + Name: "Empty", + L: 1000, + Writes: []writeCase{ + // A few empty writes + {N: 0, ExpN: 0}, {N: 0, ExpN: 0}, {N: 0, ExpN: 0}, + }, + }, + { + Name: "NotFull", + L: 1000, + Writes: []writeCase{ + {N: 250, ExpN: 250}, + {N: 250, ExpN: 250}, + {N: 250, ExpN: 250}, + }, + }, + { + Name: "Short", + L: 1000, + Writes: []writeCase{ + {N: 250, ExpN: 250}, + {N: 250, ExpN: 250}, + {N: 250, ExpN: 250}, + {N: 250, ExpN: 250}, + {N: 250, ExpN: 0, Err: true}, + }, + }, + { + Name: "Exact", + L: 1000, + Writes: []writeCase{ + { + N: 1000, + ExpN: 1000, + }, + { + N: 1000, + Err: true, + }, + }, + }, + { + Name: "Over", + L: 1000, + Writes: []writeCase{ + { + N: 5000, + ExpN: 0, + Err: true, + }, + { + N: 5000, + Err: true, + }, + { + N: 5000, + Err: true, + }, + }, + }, + { + Name: "Strange", + L: -1, + Writes: []writeCase{ + { + N: 5, + ExpN: 0, + Err: true, + }, + { + N: 0, + ExpN: 0, + Err: true, + }, + }, + }, + } + + for _, c := range testCases { + t.Run(c.Name, func(t *testing.T) { + buf := bytes.NewBuffer([]byte{}) + allBuff := bytes.NewBuffer([]byte{}) + w := xio.NewLimitWriter(buf, c.L) + + for _, wc := range c.Writes { + data := make([]byte, wc.N) + + n, err := cryptorand.Read(data) + require.NoError(t, err, "crand read") + require.Equal(t, wc.N, n, "correct bytes read") + max := data[:wc.ExpN] + n, err = w.Write(data) + if wc.Err { + require.Error(t, err, "exp error") + } else { + require.NoError(t, err, "write") + } + + // Need to use this to compare across multiple writes. + // Each write appends to the expected output. + allBuff.Write(max) + + require.Equal(t, wc.ExpN, n, "correct bytes written") + require.Equal(t, allBuff.Bytes(), buf.Bytes(), "expected data") + } + }) + } +} diff --git a/provisionersdk/archive.go b/provisionersdk/archive.go index 8312af7de2152..b71829b2c23f2 100644 --- a/provisionersdk/archive.go +++ b/provisionersdk/archive.go @@ -8,6 +8,8 @@ import ( "strings" "golang.org/x/xerrors" + + "github.com/coder/coder/coderd/util/xio" ) const ( @@ -32,8 +34,9 @@ func dirHasExt(dir string, ext string) (bool, error) { // Tar archives a Terraform directory. func Tar(w io.Writer, directory string, limit int64) error { + // The total bytes written must be under the limit. + w = xio.NewLimitWriter(w, limit) tarWriter := tar.NewWriter(w) - totalSize := int64(0) const tfExt = ".tf" hasTf, err := dirHasExt(directory, tfExt) @@ -54,7 +57,6 @@ func Tar(w io.Writer, directory string, limit int64) error { ) } - fileTooBigError := xerrors.Errorf("Archive too big. Must be <= %d bytes", limit) err = filepath.Walk(directory, func(file string, fileInfo os.FileInfo, err error) error { if err != nil { return err @@ -96,23 +98,20 @@ func Tar(w io.Writer, directory string, limit int64) error { if !fileInfo.Mode().IsRegular() { return nil } - // Before we even open the file, check if it is going to exceed our limit. - if fileInfo.Size()+totalSize > limit { - return fileTooBigError - } + data, err := os.Open(file) if err != nil { return err } defer data.Close() - wrote, err := io.Copy(tarWriter, data) + _, err = io.Copy(tarWriter, data) if err != nil { + if xerrors.Is(err, xio.ErrLimitReached) { + return xerrors.Errorf("Archive too big. Must be <= %d bytes", limit) + } return err } - totalSize += wrote - if limit != 0 && totalSize > limit { - return fileTooBigError - } + return data.Close() }) if err != nil { From 7b2844d55f10d9e39d8a0659762b9352d8e1ebeb Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Jun 2023 12:37:22 -0400 Subject: [PATCH 4/7] Catch and rewrite error --- provisionersdk/archive.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/provisionersdk/archive.go b/provisionersdk/archive.go index b71829b2c23f2..be31a74620ef9 100644 --- a/provisionersdk/archive.go +++ b/provisionersdk/archive.go @@ -115,6 +115,9 @@ func Tar(w io.Writer, directory string, limit int64) error { return data.Close() }) if err != nil { + if xerrors.Is(err, xio.ErrLimitReached) { + return xerrors.Errorf("Archive too big. Must be <= %d bytes", limit) + } return err } err = tarWriter.Flush() From ec5604ee7526b7e57b6f771ec3bc52e1e2ab51ea Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Jun 2023 12:51:29 -0400 Subject: [PATCH 5/7] Add unit test to check edges --- provisionersdk/archive.go | 4 ++-- provisionersdk/archive_test.go | 29 ++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/provisionersdk/archive.go b/provisionersdk/archive.go index be31a74620ef9..4642c82777645 100644 --- a/provisionersdk/archive.go +++ b/provisionersdk/archive.go @@ -34,8 +34,8 @@ func dirHasExt(dir string, ext string) (bool, error) { // Tar archives a Terraform directory. func Tar(w io.Writer, directory string, limit int64) error { - // The total bytes written must be under the limit. - w = xio.NewLimitWriter(w, limit) + // The total bytes written must be under the limit, so use -1 + w = xio.NewLimitWriter(w, limit-1) tarWriter := tar.NewWriter(w) const tfExt = ".tf" diff --git a/provisionersdk/archive_test.go b/provisionersdk/archive_test.go index 66fae25dd9832..947d9c48c30ab 100644 --- a/provisionersdk/archive_test.go +++ b/provisionersdk/archive_test.go @@ -15,6 +15,32 @@ import ( func TestTar(t *testing.T) { t.Parallel() + t.Run("HeaderBreakLimit", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + file, err := os.CreateTemp(dir, "*.tf") + require.NoError(t, err) + _ = file.Close() + // A header is 512 bytes + err = provisionersdk.Tar(io.Discard, dir, 100) + require.Error(t, err) + }) + t.Run("HeaderAndContent", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + file, err := os.CreateTemp(dir, "*.tf") + require.NoError(t, err) + _, _ = file.Write(make([]byte, 100)) + _ = file.Close() + // Pay + header is 1024 bytes (padding) + err = provisionersdk.Tar(io.Discard, dir, 1025) + require.NoError(t, err) + + // Limit is 1 byte too small (n == limit is a failure, must be under) + err = provisionersdk.Tar(io.Discard, dir, 1024) + require.Error(t, err) + }) + t.Run("NoTF", func(t *testing.T) { t.Parallel() dir := t.TempDir() @@ -97,7 +123,8 @@ func TestTar(t *testing.T) { } } archive := new(bytes.Buffer) - err := provisionersdk.Tar(archive, dir, 1024) + // Headers are chonky so raise the limit to something reasonable + err := provisionersdk.Tar(archive, dir, 1024<<2) require.NoError(t, err) dir = t.TempDir() err = provisionersdk.Untar(dir, archive) From 3e368a44efd39c056c39bc24df714d028ae3d5f7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 5 Jun 2023 06:58:27 -0400 Subject: [PATCH 6/7] t.Parallel unit test --- coderd/util/xio/limitwriter_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/coderd/util/xio/limitwriter_test.go b/coderd/util/xio/limitwriter_test.go index 4c6adefa9c0f1..63030e2e20d16 100644 --- a/coderd/util/xio/limitwriter_test.go +++ b/coderd/util/xio/limitwriter_test.go @@ -11,6 +11,8 @@ import ( ) func TestLimitWriter(t *testing.T) { + t.Parallel() + type writeCase struct { N int ExpN int @@ -106,6 +108,8 @@ func TestLimitWriter(t *testing.T) { for _, c := range testCases { t.Run(c.Name, func(t *testing.T) { + t.Parallel() + buf := bytes.NewBuffer([]byte{}) allBuff := bytes.NewBuffer([]byte{}) w := xio.NewLimitWriter(buf, c.L) From cf35412b77756e08f7da343fce9e7d6108edeaaf Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 5 Jun 2023 07:08:42 -0400 Subject: [PATCH 7/7] fixup! t.Parallel unit test --- coderd/util/xio/limitwriter_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/util/xio/limitwriter_test.go b/coderd/util/xio/limitwriter_test.go index 63030e2e20d16..52d6075fbb7f3 100644 --- a/coderd/util/xio/limitwriter_test.go +++ b/coderd/util/xio/limitwriter_test.go @@ -107,9 +107,10 @@ func TestLimitWriter(t *testing.T) { } for _, c := range testCases { + c := c t.Run(c.Name, func(t *testing.T) { t.Parallel() - + buf := bytes.NewBuffer([]byte{}) allBuff := bytes.NewBuffer([]byte{}) w := xio.NewLimitWriter(buf, c.L)