From 16547f64c244b7d75758735f97a62168d5f94475 Mon Sep 17 00:00:00 2001 From: Abe Jellinek Date: Thu, 27 Jul 2023 11:44:36 -0400 Subject: [PATCH] renameAttachmentFile: Check rename() return value And actually return -1 if it returns false. Before this fix, attempting to rename an attachment file to a name that already exists on disk would never return -1 as the docs say it should. Instead: 1. rename() would fail and return false 2. newName would be set to false 3. renameAttachmentFile() would pass false as the second argument to OS.Path.join() 4. OS.Path.join() would ignore it because it was falsy and return the attachment directory path without any modification 5. relinkAttachmentFile() would be called with path set to the attachment directory 6. relinkAttachmentFile() would notice that path's dirname wasn't the attachment directory - it was the attachment directory's parent - and attempt to copy it and its contents, recursively, into itself, using copyToFollowingLinks() ...which created a directory structure on disk over 100 directories deep - not deeper only because the OS started returning errors due to paths exceeding 32,767 characters (the limit on my filesystem). --- chrome/content/zotero/xpcom/data/item.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index ecbdd5612f..aa7be6e5e0 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -2801,6 +2801,9 @@ Zotero.Item.prototype.renameAttachmentFile = async function (newName, overwrite unique } ); + if (newName === false) { + return -1; + } let destPath = OS.Path.join(OS.Path.dirname(origPath), newName); await this.relinkAttachmentFile(destPath);