From 37960cbc2f766929895e752972b38cca2825cdb3 Mon Sep 17 00:00:00 2001
From: Harshavardhana <harsha@minio.io>
Date: Sun, 28 Feb 2021 15:33:03 -0800
Subject: [PATCH] fix: avoid writing more content on network with O_DIRECT
 reads (#11659)

There was an io.LimitReader was missing for the 'length'
parameter for ranged requests, that would cause client to
get truncated responses and errors.

fixes #11651
---
 cmd/object-handlers.go     |  9 +++++++++
 cmd/storage-rest-server.go |  8 +++++++-
 cmd/xl-storage.go          | 36 +++++++++++++++++++++++-------------
 3 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go
index 115c92c9b..c01f1ef4b 100644
--- a/cmd/object-handlers.go
+++ b/cmd/object-handlers.go
@@ -54,6 +54,7 @@ import (
 	"github.com/minio/minio/pkg/hash"
 	iampolicy "github.com/minio/minio/pkg/iam/policy"
 	"github.com/minio/minio/pkg/ioutil"
+	xnet "github.com/minio/minio/pkg/net"
 	"github.com/minio/minio/pkg/s3select"
 	"github.com/minio/sio"
 )
@@ -507,6 +508,10 @@ func (api objectAPIHandlers) GetObjectHandler(w http.ResponseWriter, r *http.Req
 		if !httpWriter.HasWritten() && !statusCodeWritten {
 			// write error response only if no data or headers has been written to client yet
 			writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
+			return
+		}
+		if !xnet.IsNetworkOrHostDown(err, true) { // do not need to log disconnected clients
+			logger.LogIf(ctx, fmt.Errorf("Unable to write all the data to client %w", err))
 		}
 		return
 	}
@@ -516,6 +521,10 @@ func (api objectAPIHandlers) GetObjectHandler(w http.ResponseWriter, r *http.Req
 			writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))
 			return
 		}
+		if !xnet.IsNetworkOrHostDown(err, true) { // do not need to log disconnected clients
+			logger.LogIf(ctx, fmt.Errorf("Unable to write all the data to client %w", err))
+		}
+		return
 	}
 
 	// Notify object accessed via a GET request.
diff --git a/cmd/storage-rest-server.go b/cmd/storage-rest-server.go
index e02976c53..c06988a3f 100644
--- a/cmd/storage-rest-server.go
+++ b/cmd/storage-rest-server.go
@@ -40,6 +40,7 @@ import (
 	xhttp "github.com/minio/minio/cmd/http"
 	xjwt "github.com/minio/minio/cmd/jwt"
 	"github.com/minio/minio/cmd/logger"
+	xnet "github.com/minio/minio/pkg/net"
 )
 
 var errDiskStale = errors.New("disk stale")
@@ -527,7 +528,12 @@ func (s *storageRESTServer) ReadFileStreamHandler(w http.ResponseWriter, r *http
 	defer rc.Close()
 
 	w.Header().Set(xhttp.ContentLength, strconv.Itoa(length))
-	io.Copy(w, rc)
+	if _, err = io.Copy(w, rc); err != nil {
+		if !xnet.IsNetworkOrHostDown(err, true) { // do not need to log disconnected clients
+			logger.LogIf(r.Context(), err)
+		}
+		return
+	}
 	w.(http.Flusher).Flush()
 }
 
diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go
index 087510e51..8ad38818a 100644
--- a/cmd/xl-storage.go
+++ b/cmd/xl-storage.go
@@ -1423,9 +1423,6 @@ func (o *odirectReader) Close() error {
 	} else {
 		o.s.poolLarge.Put(o.bufp)
 	}
-	defer func() {
-		atomic.AddInt32(&o.s.activeIOCount, -1)
-	}()
 	return o.f.Close()
 }
 
@@ -1449,10 +1446,10 @@ func (s *xlStorage) ReadFileStream(ctx context.Context, volume, path string, off
 	var file *os.File
 	// O_DIRECT only supported if offset is zero
 	if offset == 0 && globalStorageClass.GetDMA() == storageclass.DMAReadWrite && s.readODirectSupported {
-		file, err = disk.OpenFileDirectIO(filePath, os.O_RDONLY, 0666)
+		file, err = disk.OpenFileDirectIO(filePath, readMode, 0666)
 	} else {
 		// Open the file for reading.
-		file, err = os.Open(filePath)
+		file, err = os.OpenFile(filePath, readMode, 0666)
 	}
 	if err != nil {
 		switch {
@@ -1479,27 +1476,33 @@ func (s *xlStorage) ReadFileStream(ctx context.Context, volume, path string, off
 
 	st, err := file.Stat()
 	if err != nil {
+		file.Close()
 		return nil, err
 	}
 
 	// Verify it is a regular file, otherwise subsequent Seek is
 	// undefined.
 	if !st.Mode().IsRegular() {
+		file.Close()
 		return nil, errIsNotRegular
 	}
 
 	atomic.AddInt32(&s.activeIOCount, 1)
 	if offset == 0 && globalStorageClass.GetDMA() == storageclass.DMAReadWrite && s.readODirectSupported {
+		or := &odirectReader{file, nil, nil, true, false, s, nil}
 		if length <= smallFileThreshold {
-			return &odirectReader{file, nil, nil, true, true, s, nil}, nil
-		}
-		return &odirectReader{file, nil, nil, true, false, s, nil}, nil
-	}
-
-	if offset > 0 {
-		if _, err = file.Seek(offset, io.SeekStart); err != nil {
-			return nil, err
+			or = &odirectReader{file, nil, nil, true, true, s, nil}
 		}
+		r := struct {
+			io.Reader
+			io.Closer
+		}{Reader: io.LimitReader(or, length), Closer: closeWrapper(func() error {
+			defer func() {
+				atomic.AddInt32(&s.activeIOCount, -1)
+			}()
+			return or.Close()
+		})}
+		return r, nil
 	}
 
 	r := struct {
@@ -1512,6 +1515,13 @@ func (s *xlStorage) ReadFileStream(ctx context.Context, volume, path string, off
 		return file.Close()
 	})}
 
+	if offset > 0 {
+		if _, err = file.Seek(offset, io.SeekStart); err != nil {
+			r.Close()
+			return nil, err
+		}
+	}
+
 	// Add readahead to big reads
 	if length >= readAheadSize {
 		rc, err := readahead.NewReadCloserSize(r, readAheadBuffers, readAheadBufSize)