Merge branch 'dean/fix-thumbnail-race' into 'master'

fix: copy thumbnail data to avoid buffer race

See merge request whats-this/cdn-origin!13

lib/thumbnailer/cache.go

@@ -25,6 +25,9 @@ func NewThumbnailCache(directory, thumbnailerURL string) *ThumbnailCache {
 // GetThumbnail returns a thumbnail that is cached. If no cached copy exists, a
 // exists, a NoCachedCopy error is returned.
 func (c *ThumbnailCache) GetThumbnail(key string) (io.ReadCloser, error) {
+	if key == "" {
+		return nil, NoKeySpecified
+	}
 	path := filepath.Join(c.Directory, key)
 	data, err := os.Open(path)
 	if os.IsNotExist(err) {
@@ -35,27 +38,30 @@ func (c *ThumbnailCache) GetThumbnail(key string) (io.ReadCloser, error) {
 
 // SetThumbnail stores a thumbnail with the specified key.
 func (c *ThumbnailCache) SetThumbnail(key string, data io.Reader) error {
+	if key == "" {
+		return NoKeySpecified
+	}
 	path := filepath.Join(c.Directory, key)
 	file, err := os.Create(path)
-	defer file.Close()
 	if err != nil {
 		return err
 	}
+	defer file.Close()
 	_, err = io.Copy(file, data)
+	if err != nil {
+		_ = os.Remove(path)
+	}
 	return err
 }
 
 // Transform generates a thumbnail and caches it.
 func (c *ThumbnailCache) Transform(key string, contentType string, data io.Reader) error {
+	if key == "" {
+		return NoKeySpecified
+	}
 	outputImage, err := Transform(c.ThumbnailerURL, contentType, data)
 	if err != nil {
 		return err
 	}
 	return c.SetThumbnail(key, outputImage)
 }
-
-// DeleteThumbnail deletes a thumbnail from the cache.
-func (c *ThumbnailCache) DeleteThumbnail(key string) error {
-	path := filepath.Join(c.Directory, key)
-	return os.Remove(path)
-}

lib/thumbnailer/errors.go

@@ -14,3 +14,6 @@ var NoCachedCopy error = &thumbnailerError{"no cached copy of the thumbnail requ
 
 // InputTooLarge means that the pixel size of the input image is too big to be thumbnailed.
 var InputTooLarge error = &thumbnailerError{"the input size in pixels is too large"}
+
+// NoKeySpecified means that no key was specified.
+var NoKeySpecified error = &thumbnailerError{"no key specified"}

lib/thumbnailer/transform.go

@@ -11,10 +11,10 @@ import (
 
 // Accepted MIME types for thumbnails in map for easy checking
 var thumbnailMIMETypes = map[string]struct{}{
-	"image/gif":  struct{}{},
-	"image/jpeg": struct{}{},
-	"image/png":  struct{}{},
-	"image/webp": struct{}{},
+	"image/gif":  {},
+	"image/jpeg": {},
+	"image/png":  {},
+	"image/webp": {},
 }
 
 // AcceptedMIMEType checks if a MIME type is suitable for thumbnailing.
@@ -27,15 +27,19 @@ func AcceptedMIMEType(mime string) bool {
 // Transform takes an image io.Reader and sends it to the thumbnailer service
 // to be transcoded into a thumbnail.
 func Transform(thumbnailerURL, contentType string, data io.Reader) (*bytes.Buffer, error) {
-	// Set request and response
+	if !AcceptedMIMEType(contentType) {
+		return nil, errors.Errorf("invalid MIME type: %s", contentType)
+	}
+
 	req := fasthttp.AcquireRequest()
 	res := fasthttp.AcquireResponse()
 	defer func() {
 		fasthttp.ReleaseRequest(req)
 		fasthttp.ReleaseResponse(res)
 	}()
-
 	req.Reset()
+	res.Reset()
+
 	req.Header.SetMethod("POST")
 	req.SetRequestURI(thumbnailerURL)
 	req.Header.Set("Content-Type", contentType)
@@ -43,9 +47,7 @@ func Transform(thumbnailerURL, contentType string, data io.Reader) (*bytes.Buffe
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to copy data to request")
 	}
-	res.Reset()
 
-	// Do request
 	err = fasthttp.Do(req, res)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to make request to thumbnailer service")
@@ -54,5 +56,12 @@ func Transform(thumbnailerURL, contentType string, data io.Reader) (*bytes.Buffe
 		return nil, errors.Errorf("thumbnailer service failed to create thumbnail: %s", string(res.Body()))
 	}
 
-	return bytes.NewBuffer(res.Body()), nil
+	// Copy the response body to a buffer so we can return it safely.
+	// ReleaseResponse will return the buffer to the pool.
+	buf := bytes.NewBuffer(nil)
+	err = res.BodyWriteTo(buf)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to copy response body to buffer")
+	}
+	return buf, nil
 }

objects.sql

 -- "objects" table schema
-CREATE TABLE IF NOT EXISTS objects (
-  bucket_key VARCHAR(1088) NOT NULL UNIQUE, -- bucket + key (unique)
-  bucket VARCHAR(20) NOT NULL, -- uint64 bucket ID ("public" for public bucket)
-  "key" VARCHAR(1024) NOT NULL, -- Full bucket path to file (including directory)
-  random_key VARCHAR(1024), -- random key if used
-  dir VARCHAR(1024) NOT NULL, -- Directory of file (with trailing slash)
-  "type" integer NOT NULL DEFAULT 0, -- Object type enumerable (0 = file, 1 = redirect)
-  dest_url VARCHAR(1024) DEFAULT NULL, -- Destination URL for redirect object (only when object.type == 1)
-  content_type VARCHAR(255) DEFAULT 'application/octet-stream', -- Content-Type of file
-  content_length INT DEFAULT NULL, -- Content-Length of file
-  associated_user VARCHAR(36) DEFAULT NULL, -- ID of user who uploaded file
-  created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, -- File creation timestamp
-  deleted_at TIMESTAMP DEFAULT NULL, -- Deletion timestamp
-  delete_reason VARCHAR(256) DEFAULT NULL, -- Deletion reason
-  md5_hash VARCHAR(32) DEFAULT NULL -- MD5 hash of file contents (or destination URL)
+CREATE TABLE objects (
+  bucket_key character varying(1088) NOT NULL, -- ${bucket}/${key} (unique)
+  bucket character varying(20) NOT NULL, -- Bucket ID ("public" for public bucket)
+  key character varying(1024) NOT NULL, -- Full bucket path to file (including directory)
+  dir character varying(1024) NOT NULL, -- Directory of file (with trailing slash)
+  type integer DEFAULT 0 NOT NULL, -- Object type enumerable (0 = file, 1 = redirect)
+  backend_file_id character varying(33) DEFAULT NULL, -- ID of file in backend storage
+  dest_url character varying(4096) DEFAULT NULL, -- Destination URL for redirect object (only when object.type == 1)
+  content_type character varying(255) DEFAULT 'application/octet-stream', -- Content-Type of file
+  content_length integer, -- Content-Length of file
+  created_at timestamp without time zone DEFAULT now() NOT NULL, -- File creation timestamp
+  random_key character varying(1024) DEFAULT NULL, -- Random key if used
+  associated_user character varying(36) DEFAULT NULL, -- ID of user who uploaded file
+  deleted_at timestamp without time zone, -- Deletion timestamp
+  delete_reason character varying(256) DEFAULT NULL::character varying, -- Deletion reason
+  sha256_hash bytea, -- SHA256 hash of file contents (or destination URL)
+  md5_hash bytea -- MD5 hash of file contents (or destination URL)
 );
 
 -- Test file object: /index.md