From bb15cd906717ba054d9c23db2971e94d2e250969 Mon Sep 17 00:00:00 2001
From: Lovell Fuller <github@lovell.info>
Date: Wed, 27 Nov 2019 23:15:56 +0000
Subject: [PATCH] Improve thread safety with copy-on-write for metadata #1986

---
 docs/changelog.md |  5 +++++
 src/common.cc     | 26 ++++++++++++++++----------
 src/common.h      |  6 +++---
 src/pipeline.cc   | 10 +++++-----
 4 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/docs/changelog.md b/docs/changelog.md
index cc98357b..cd5c97d8 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -4,6 +4,11 @@
 
 Requires libvips v8.8.1.
 
+#### v0.23.4 - TBD
+
+* Improve thread safety by using copy-on-write when updating metadata.
+  [#1986](https://github.com/lovell/sharp/issues/1986)
+
 #### v0.23.3 - 17<sup>th</sup> November 2019
 
 * Ensure `trim` operation supports images contained in the alpha channel.
diff --git a/src/common.cc b/src/common.cc
index fa5b4c79..38727e3a 100644
--- a/src/common.cc
+++ b/src/common.cc
@@ -277,7 +277,7 @@ namespace sharp {
             }
             image = VImage::new_from_buffer(descriptor->buffer, descriptor->bufferLength, nullptr, option);
             if (imageType == ImageType::SVG || imageType == ImageType::PDF || imageType == ImageType::MAGICK) {
-              SetDensity(image, descriptor->density);
+              image = SetDensity(image, descriptor->density);
             }
           } catch (vips::VError const &err) {
             throw vips::VError(std::string("Input buffer has corrupt header: ") + err.what());
@@ -323,7 +323,7 @@ namespace sharp {
             }
             image = VImage::new_from_file(descriptor->file.data(), option);
             if (imageType == ImageType::SVG || imageType == ImageType::PDF || imageType == ImageType::MAGICK) {
-              SetDensity(image, descriptor->density);
+              image = SetDensity(image, descriptor->density);
             }
           } catch (vips::VError const &err) {
             throw vips::VError(std::string("Input file has corrupt header: ") + err.what());
@@ -370,15 +370,19 @@ namespace sharp {
   /*
     Set EXIF Orientation of image.
   */
-  void SetExifOrientation(VImage image, int const orientation) {
-    image.set(VIPS_META_ORIENTATION, orientation);
+  VImage SetExifOrientation(VImage image, int const orientation) {
+    VImage copy = image.copy();
+    copy.set(VIPS_META_ORIENTATION, orientation);
+    return copy;
   }
 
   /*
     Remove EXIF Orientation from image.
   */
-  void RemoveExifOrientation(VImage image) {
-    vips_image_remove(image.get_image(), VIPS_META_ORIENTATION);
+  VImage RemoveExifOrientation(VImage image) {
+    VImage copy = image.copy();
+    copy.remove(VIPS_META_ORIENTATION);
+    return copy;
   }
 
   /*
@@ -398,11 +402,13 @@ namespace sharp {
   /*
     Set pixels/mm resolution based on a pixels/inch density.
   */
-  void SetDensity(VImage image, const double density) {
+  VImage SetDensity(VImage image, const double density) {
     const double pixelsPerMm = density / 25.4;
-    image.set("Xres", pixelsPerMm);
-    image.set("Yres", pixelsPerMm);
-    image.set(VIPS_META_RESOLUTION_UNIT, "in");
+    VImage copy = image.copy();
+    copy.set("Xres", pixelsPerMm);
+    copy.set("Yres", pixelsPerMm);
+    copy.set(VIPS_META_RESOLUTION_UNIT, "in");
+    return copy;
   }
 
   /*
diff --git a/src/common.h b/src/common.h
index aec647ab..e4b03d5c 100644
--- a/src/common.h
+++ b/src/common.h
@@ -175,12 +175,12 @@ namespace sharp {
   /*
     Set EXIF Orientation of image.
   */
-  void SetExifOrientation(VImage image, int const orientation);
+  VImage SetExifOrientation(VImage image, int const orientation);
 
   /*
     Remove EXIF Orientation from image.
   */
-  void RemoveExifOrientation(VImage image);
+  VImage RemoveExifOrientation(VImage image);
 
   /*
     Does this image have a non-default density?
@@ -195,7 +195,7 @@ namespace sharp {
   /*
     Set pixels/mm resolution based on a pixels/inch density.
   */
-  void SetDensity(VImage image, const double density);
+  VImage SetDensity(VImage image, const double density);
 
   /*
     Check the proposed format supports the current dimensions.
diff --git a/src/pipeline.cc b/src/pipeline.cc
index 902a6445..83ba354d 100644
--- a/src/pipeline.cc
+++ b/src/pipeline.cc
@@ -104,7 +104,7 @@ class PipelineWorker : public Nan::AsyncWorker {
       if (baton->rotateBeforePreExtract) {
         if (rotation != VIPS_ANGLE_D0) {
           image = image.rot(rotation);
-          sharp::RemoveExifOrientation(image);
+          image = sharp::RemoveExifOrientation(image);
         }
         if (baton->rotationAngle != 0.0) {
           std::vector<double> background;
@@ -404,20 +404,20 @@ class PipelineWorker : public Nan::AsyncWorker {
       // Rotate post-extract 90-angle
       if (!baton->rotateBeforePreExtract &&  rotation != VIPS_ANGLE_D0) {
           image = image.rot(rotation);
-          sharp::RemoveExifOrientation(image);
+          image = sharp::RemoveExifOrientation(image);
       }
 
 
       // Flip (mirror about Y axis)
       if (baton->flip) {
         image = image.flip(VIPS_DIRECTION_VERTICAL);
-        sharp::RemoveExifOrientation(image);
+        image = sharp::RemoveExifOrientation(image);
       }
 
       // Flop (mirror about X axis)
       if (baton->flop) {
         image = image.flip(VIPS_DIRECTION_HORIZONTAL);
-        sharp::RemoveExifOrientation(image);
+        image = sharp::RemoveExifOrientation(image);
       }
 
       // Join additional color channels to the image
@@ -700,7 +700,7 @@ class PipelineWorker : public Nan::AsyncWorker {
 
       // Override EXIF Orientation tag
       if (baton->withMetadata && baton->withMetadataOrientation != -1) {
-        sharp::SetExifOrientation(image, baton->withMetadataOrientation);
+        image = sharp::SetExifOrientation(image, baton->withMetadataOrientation);
       }
 
       // Number of channels used in output image