Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upload cache fixes #439

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from
Open

Upload cache fixes #439

wants to merge 18 commits into from

Conversation

pboguslawski
Copy link
Contributor

Proposed change

This mod fixes the following upload cache problems:

(1)
If one uploads many files (dropping files test0.bin-test9.bin in one move) races may occur and file id in DOM won't match file ids in web upload dir on server, i.e.:

$(".AttachmentListContainer tbody tr").filter(function() { console.log("%s, %d", $(this).find('td.Filename').text(), $(this).find('a').data('file-id'))})
test9.bin, 10
test8.bin, 8
test7.bin, 8
test6.bin, 7
test5.bin, 4
test4.bin, 5
test3.bin, 3
test2.bin, 3
test1.bin, 2
test0.bin, 1

This may be fixed with patch (which unblocks updating all file ids on upload not just for one file)...

--- a/var/httpd/htdocs/js/Core.UI.js   2021-11-08 18:53:34.088284466 +0100
+++ b/var/httpd/htdocs/js/Core.UI.js 2021-12-16 18:41:14.090199778 +0100
@@ -697,10 +697,6 @@
 
                                 $TargetObj = $ExistingItemObj.closest('tr');
 
-                                if ($TargetObj.find('a').data('file-id')) {
-                                    return;
-                                }
-
                                 $TargetObj
                                     .find('.Filetype')
                                     .text(Attachment.ContentType)

...but using file ids for identification of uploaded files is not race condition resistant; to provoke race condition apply patch

--- a/Kernel/Modules/AjaxAttachment.pm 2021-06-19 21:59:34.379471088 +0200
+++ b/Kernel/Modules/AjaxAttachment.pm       2021-12-16 18:38:28.943925139 +0100
@@ -56,6 +56,10 @@
             Param => 'Files',
         );
 
+        if ($UploadStuff{Filename} eq 'test01.bin') {
+            sleep(1);
+        }
+
         $UploadCacheObject->FormIDAddFile(
             FormID      => $Self->{FormID},
             Disposition => 'attachment',
@@ -67,6 +71,10 @@
             FormID => $Self->{FormID},
         );
 
+        if ($UploadStuff{Filename} eq 'test02.bin') {
+            sleep(2);
+        }
+
         my @AttachmentData;

and drop files test01.bin and test02.bin (same size) in one move; after upload there will be:

$(".AttachmentListContainer tbody tr").filter(function() { console.log("%s, %d", $(this).find('td.Filename').text(), $(this).find('a').data('file-id'))})
test02.bin, 1
test01.bin, 1

not

test02.bin, 2
test01.bin, 1

This problem was solved by removing unnecessary FileID attribute from upload data struct to avoid any races that may occur between filename list and fileid list. Only filename will be used to identify items added to upload cache.

This bug is dangerous - think about uploading some confidential stuff by mistake, then removing it (disappears from UI) and sending message with different set of files than displayed in UI (i.e. with confidential stuff that user wanted to remove).

(2)
Double clicking trash icon to remove attachment generates two requests to server and one will cause an error.

(3)
Unused upload handling code removed.

Type of change

  • '1 - 🐞 bug 🐞' - Bugfix (non-breaking change which fixes an issue)

Breaking change

Removes FileID attribute from web upload cache objects to avoid races.

Additional information

Replaces: #179
Author-Change-Id: IB#1113065

Checklist

  • The code change is tested and works locally.(❗)
  • There is no commented out code in this PR.(❕)
  • You improved or added new unit tests.(❕)
  • Local ZnunyCodePolicy run passes successfully.(❕)
  • Local unit tests pass.(❕)
  • GitHub workflow ZnunyCodePolicy passes.(❗)
  • GitHub workflow unit tests pass.(❗)

This mod fixes the following upload cache problems:

(1)
If one uploads many files (dropping files test0.bin-test9.bin in one move) races may occur and file id in DOM won't match file ids in web upload dir on server, i.e.:

```
$(".AttachmentListContainer tbody tr").filter(function() { console.log("%s, %d", $(this).find('td.Filename').text(), $(this).find('a').data('file-id'))})
test9.bin, 10
test8.bin, 8
test7.bin, 8
test6.bin, 7
test5.bin, 4
test4.bin, 5
test3.bin, 3
test2.bin, 3
test1.bin, 2
test0.bin, 1
```

This may be fixed with patch (which unblocks updating all file ids on upload not just for one file)...

```
--- a/var/httpd/htdocs/js/Core.UI.js   2021-11-08 18:53:34.088284466 +0100
+++ b/var/httpd/htdocs/js/Core.UI.js 2021-12-16 18:41:14.090199778 +0100
@@ -697,10 +697,6 @@

                                 $TargetObj = $ExistingItemObj.closest('tr');

-                                if ($TargetObj.find('a').data('file-id')) {
-                                    return;
-                                }
-
                                 $TargetObj
                                     .find('.Filetype')
                                     .text(Attachment.ContentType)
```

...but using file ids for identification of uploaded files is not race condition resistant; to provoke race condition apply patch

```
--- a/Kernel/Modules/AjaxAttachment.pm 2021-06-19 21:59:34.379471088 +0200
+++ b/Kernel/Modules/AjaxAttachment.pm       2021-12-16 18:38:28.943925139 +0100
@@ -56,6 +56,10 @@
             Param => 'Files',
         );

+        if ($UploadStuff{Filename} eq 'test01.bin') {
+            sleep(1);
+        }
+
         $UploadCacheObject->FormIDAddFile(
             FormID      => $Self->{FormID},
             Disposition => 'attachment',
@@ -67,6 +71,10 @@
             FormID => $Self->{FormID},
         );

+        if ($UploadStuff{Filename} eq 'test02.bin') {
+            sleep(2);
+        }
+
         my @AttachmentData;
```

and drop files test01.bin and test02.bin (same size) in one move; after upload there will be:

```
$(".AttachmentListContainer tbody tr").filter(function() { console.log("%s, %d", $(this).find('td.Filename').text(), $(this).find('a').data('file-id'))})
test02.bin, 1
test01.bin, 1
```

not

```
test02.bin, 2
test01.bin, 1
```

This problem was solved by removing unnecessarry FileID attribute from upload data struct to avoid any races that may occurr between filename list and fileid list. Only filename will be used to identify items added to upload cache.

(2)
Double clicking trash icon to remove attachment generates two requests to server and one will cause an error.

(3)
Web form is not removed from upload cache dir after saving or updating template.

(4)
Unused upload handling code removed.

Author-Change-Id: IB#1113065
Added missing part of 6e42dbb.

Fixes: 6e42dbb
Author-Change-Id: IB#1113065
Code policy cleanups.

Fixes: 6e42dbb
Author-Change-Id: IB#1113065
Fixes: 6e42dbb
Author-Change-Id: IB#1113065
Will be moved to separate issue.

Fixes: 53a7ba6
Author-Change-Id: IB#1113065
Fixes: 6e42dbb
Author-Change-Id: IB#1113065
Parameter name regression fixed.

Fixes: 2d608b05303537e10c75a453f941026e58c2b9eb
Author-Change-Id: IB#1113065
Author-Change-Id: IB#1113065
@pboguslawski pboguslawski mentioned this pull request May 19, 2023
7 tasks
@dennykorsukewitz dennykorsukewitz added the 1 - 🐞 bug 🐞 An issue with the system. label Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - 🐞 bug 🐞 An issue with the system.
Development

Successfully merging this pull request may close these issues.

None yet

2 participants