When handing off a picture to a worker thread, copy it first

as otherwise we crash when the picture is freed before the
worker thread (to load from the net or to compute hashes)
is finished

Signed-off-by: Robert C. Helling <helling@atdotde.de>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>

5 files changed, 30 insertions, 6 deletions

diff --git a/subsurface-core/dive.c b/subsurface-core/dive.c
index 47b3e1f06..cad2a85cc 100644
--- a/subsurface-core/dive.c
+++ b/subsurface-core/dive.c
@@ -3374,7 +3374,7 @@ void dive_set_geodata_from_picture(struct dive *dive, struct picture *picture)
 	}
 }
 
-static void picture_free(struct picture *picture)
+void picture_free(struct picture *picture)
 {
 	if (!picture)
 		return;
@@ -3383,6 +3383,18 @@ static void picture_free(struct picture *picture)
 	free(picture);
 }
 
+// When handling pictures in different threads, we need to copy them so we don't
+// run into problems when the main thread frees the picture.
+
+struct picture *clone_picture(struct picture *src)
+{
+	struct picture *dst;
+
+	dst = alloc_picture();
+	copy_pl(src, dst);
+	return dst;
+}
+
 void dive_remove_picture(char *filename)
 {
 	struct picture **picture = &current_dive->picture_list;
diff --git a/subsurface-core/dive.h b/subsurface-core/dive.h
index 74f55ee81..484a67f8a 100644
--- a/subsurface-core/dive.h
+++ b/subsurface-core/dive.h
@@ -376,6 +376,7 @@ struct picture {
 	for (struct picture *picture = (_divestruct).picture_list; picture; picture = picture->next)
 
 extern struct picture *alloc_picture();
+extern struct picture *clone_picture(struct picture *src);
 extern bool dive_check_picture_time(struct dive *d, int shift_time, timestamp_t timestamp);
 extern void dive_create_picture(struct dive *d, char *filename, int shift_time, bool match_all);
 extern void dive_add_picture(struct dive *d, struct picture *newpic);
@@ -385,6 +386,7 @@ extern bool picture_check_valid(char *filename, int shift_time);
 extern void picture_load_exif_data(struct picture *p);
 extern timestamp_t picture_get_timestamp(char *filename);
 extern void dive_set_geodata_from_picture(struct dive *d, struct picture *pic);
+extern void picture_free(struct picture *picture);
 
 extern int explicit_first_cylinder(struct dive *dive, struct divecomputer *dc);
 extern int get_depth_at_time(struct divecomputer *dc, unsigned int time);
diff --git a/subsurface-core/imagedownloader.cpp b/subsurface-core/imagedownloader.cpp
index 9451f8a1b..d28ec9503 100644
--- a/subsurface-core/imagedownloader.cpp
+++ b/subsurface-core/imagedownloader.cpp
@@ -17,6 +17,11 @@ ImageDownloader::ImageDownloader(struct picture *pic)
 	picture = pic;
 }
 
+ImageDownloader::~ImageDownloader()
+{
+	picture_free(picture);
+}
+
 void ImageDownloader::load(bool fromHash){
 	QUrl url;
 	if(fromHash)
@@ -79,21 +84,21 @@ SHashedImage::SHashedImage(struct picture *picture) : QImage()
 		if (filename.isNull()) {
 			// That didn't produce a local filename.
 			// Try the cloud server
-			QtConcurrent::run(loadPicture, picture, true);
+			QtConcurrent::run(loadPicture, clone_picture(picture), true);
 		} else {
 			// Load locally from translated file name
 			load(filename);
 			if (!isNull()) {
 				// Make sure the hash still matches the image file
-				QtConcurrent::run(updateHash, picture);
+				QtConcurrent::run(updateHash, clone_picture(picture));
 			} else {
 				// Interpret filename as URL
-				QtConcurrent::run(loadPicture, picture, false);
+				QtConcurrent::run(loadPicture, clone_picture(picture), false);
 			}
 		}
 	} else {
 		// We loaded successfully. Now, make sure hash is up to date.
-		QtConcurrent::run(hashPicture, picture);
+		QtConcurrent::run(hashPicture, clone_picture(picture));
 	}
 }
 
diff --git a/subsurface-core/imagedownloader.h b/subsurface-core/imagedownloader.h
index cd85c9509..2ff68342c 100644
--- a/subsurface-core/imagedownloader.h
+++ b/subsurface-core/imagedownloader.h
@@ -14,6 +14,7 @@ class ImageDownloader : public QObject {
 	Q_OBJECT;
 public:
 	ImageDownloader(struct picture *picture);
+	~ImageDownloader();
 	void load(bool fromHash);
 
 private:
diff --git a/subsurface-core/qthelper.cpp b/subsurface-core/qthelper.cpp
index 7c67e0993..b3891298a 100644
--- a/subsurface-core/qthelper.cpp
+++ b/subsurface-core/qthelper.cpp
@@ -1108,11 +1108,14 @@ QString fileFromHash(char *hash)
 	return localFilenameOf[QByteArray::fromHex(hash)];
 }
 
+// This needs to operate on a copy of picture as it frees it after finishing!
 void updateHash(struct picture *picture) {
 	QByteArray hash = hashFile(fileFromHash(picture->hash));
 	learnHash(picture, hash);
+	picture_free(picture);
 }
 
+// This needs to operate on a copy of picture as it frees it after finishing!
 void hashPicture(struct picture *picture)
 {
 	char *oldHash = copy_string(picture->hash);
@@ -1120,13 +1123,14 @@ void hashPicture(struct picture *picture)
 	if (!same_string(picture->hash, "") && !same_string(picture->hash, oldHash))
 		mark_divelist_changed((true));
 	free(oldHash);
+	picture_free(picture);
 }
 
 extern "C" void cache_picture(struct picture *picture)
 {
 	QString filename = picture->filename;
 	if (!hashOf.contains(filename))
-		QtConcurrent::run(hashPicture, picture);
+		QtConcurrent::run(hashPicture, clone_picture(picture));
 }
 
 void learnImages(const QDir dir, int max_recursions)