Close file on invalid range

[KN4CK3R]

Found by @lunny
#14726 (review)

[noerw]

Not very familiar with this code, but shouldn't we close in the other error-cases too? (Unable to seek)

[noerw]

Suggested change

	log.Error("Whilst trying to read LFS OID[%s]: Unable to close Error: %v", meta.Oid, err)
	log.Error("Whilst trying to read LFS OID[%s]: Unable to close: %v", meta.Oid, err)

[zeripath]

Not very familiar with this code, but shouldn't we close in the other error-cases too? (Unable to seek)

Yup it's gonna need that too

[zeripath]

The rest of log messages in that function look like this - so if you want to clean this up - you need to clean them all up.

[lunny]

If we close the f here, but we don't return immediately. but we still return f out of the function.

[KN4CK3R]

I don't think all should be canceled if Seek fails. How about remove the Seek part from this method? It is only used by the LFS server to resume downloads. Other calls just pass 0 as fromByte. The Seek should happen in the LFS server code which can handle it better (just present the full file to download).

[lunny]

If don't cancel when seek failed, it may read wrong bytes.

[KN4CK3R]

What do you think? For me it looks cleaner with fewer logging noise.

--- a/modules/lfs/content_store.go
+++ b/modules/lfs/content_store.go
@@ -45,32 +45,13 @@ type ContentStore struct {
 
 // Get takes a Meta object and retrieves the content from the store, returning it as an io.ReadSeekCloser.
-func (s *ContentStore) Get(meta *models.LFSMetaObject, fromByte int64) (io.ReadCloser, error) {
+func (s *ContentStore) Get(meta *models.LFSMetaObject) (io.ReadSeekCloser, error) {
 	f, err := s.Open(meta.RelativePath())
 	if err != nil {
 		log.Error("Whilst trying to read LFS OID[%s]: Unable to open Error: %v", meta.Oid, err)
 		return nil, err
 	}
-	if fromByte > 0 {
-		if fromByte >= meta.Size {
-			err = f.Close()
-			if err != nil {
-				log.Error("Whilst trying to read LFS OID[%s]: Unable to close Error: %v", meta.Oid, err)
-			}
-			return nil, ErrRangeNotSatisfiable{
-				FromByte: fromByte,
-			}
-		}
-		_, err = f.Seek(fromByte, io.SeekStart)
-		if err != nil {
-			log.Error("Whilst trying to read LFS OID[%s]: Unable to seek to %d Error: %v", meta.Oid, fromByte, err)
-			errClose := f.Close()
-			if errClose != nil {
-				log.Error("Whilst trying to read LFS OID[%s]: Unable to close Error: %v", meta.Oid, errClose)
-			}
-		}
-	}
-	return f, err
+	return f, nil
 }
 
--- a/modules/lfs/pointers.go
+++ b/modules/lfs/pointers.go
@@ -67,5 +67,5 @@ func IsPointerFile(buf *[]byte) *models.LFSMetaObject {
 // ReadMetaObject will read a models.LFSMetaObject and return a reader
 func ReadMetaObject(meta *models.LFSMetaObject) (io.ReadCloser, error) {
 	contentStore := &ContentStore{ObjectStorage: storage.LFS}
-	return contentStore.Get(meta, 0)
+	return contentStore.Get(meta)
 }

--- a/modules/lfs/server.go
+++ b/modules/lfs/server.go
@@ -175,6 +175,11 @@ func getContentHandler(ctx *context.Context) {
 			statusCode = 206
 			fromByte, _ = strconv.ParseInt(match[1], 10, 32)
 
+			if fromByte >= meta.Size {
+				writeStatus(ctx, http.StatusRequestedRangeNotSatisfiable)
+				return
+			}
+
 			if match[2] != "" {
				_toByte, _ := strconv.ParseInt(match[2], 10, 32)
				if _toByte >= fromByte && _toByte < toByte {
					toByte = _toByte
				}
			}

			ctx.Resp.Header().Set("Content-Range", fmt.Sprintf("bytes %d-%d/%d", fromByte, toByte, meta.Size-fromByte))
			ctx.Resp.Header().Set("Access-Control-Expose-Headers", "Content-Range")
		}
	}
 
 	contentStore := &ContentStore{ObjectStorage: storage.LFS}
-	content, err := contentStore.Get(meta, fromByte)
+	content, err := contentStore.Get(meta)
 	if err != nil {
-		if IsErrRangeNotSatisfiable(err) {
-			writeStatus(ctx, http.StatusRequestedRangeNotSatisfiable)
-		} else {
-			// Errors are logged in contentStore.Get
-			writeStatus(ctx, 404)
-		}
+		// Errors are logged in contentStore.Get
+		writeStatus(ctx, http.StatusNotFound)
 		return
 	}
 	defer content.Close()
 
+	if fromByte > 0 {
+		_, err = content.Seek(fromByte, io.SeekStart)
+		if err != nil {
+			log.Error("Whilst trying to read LFS OID[%s]: Unable to seek to %d Error: %v", meta.Oid, fromByte, err)
+
+			writeStatus(ctx, http.StatusInternalServerError)
+			return
+		}
+	}
+
 	contentLength := toByte + 1 - fromByte
 	ctx.Resp.Header().Set("Content-Length", strconv.FormatInt(contentLength, 10))
 	ctx.Resp.Header().Set("Content-Type", "application/octet-stream")

[lunny]

@KN4CK3R Further more, we can remove Get method, because we already have Open and Get is the same as Open after removed the frombyte parameter.

[KN4CK3R]

I thought about that too but kept the Get method to hide the meta.RelativePath() internal detail. One step further would be a private inner storage to remove the public Open method.

type ContentStore struct {
	storage storage.ObjectStorage
}

[6543]

I guess it's a bugfix...?

backport to v1.14?

[lunny]

I guess it's a bugfix...?

backport to v1.14?

please

[6543]

🚀

[6543]

@KN4CK3R please backport 🚀

[KN4CK3R]

While creating the backport I noticed I forgot to push the proposed changes from #15166 (comment) The current code is not really good because of the potential double Close. I will create a second PR with the changes.