Skip to content

explicitly close source stream on local / encryption storage - #28907

Merged
kesselb merged 2 commits into
masterfrom
bug/noid/close-stream-local
Oct 8, 2021
Merged

explicitly close source stream on local / encryption storage#28907
kesselb merged 2 commits into
masterfrom
bug/noid/close-stream-local

Conversation

@kesselb

@kesselbkesselb commented Sep 20, 2021

Copy link
Copy Markdown
Contributor

e60a829 local

Uncaught Error: Class 'OC\Log\ExceptionSerializer' not found in /var/www/html/lib/private/Log.php:316
Stack trace:
#0 /var/www/html/lib/private/Log/ErrorHandler.php(100): OC\Log->logException(Object(Error), Array)
#1 [internal function]: OC\Log\ErrorHandler::onAll(2, 'fwrite(): suppl...', '/var/www/html/a...', 62, Array)
#2 /var/www/html/apps-extra/files_antivirus/lib/Scanner/ExternalClam.php(62): fwrite(Resource id #1295, '\x00\x00\x00\x00')
#3 /var/www/html/apps-extra/files_antivirus/lib/Scanner/ScannerBase.php(143): OCA\Files_Antivirus\Scanner\ExternalClam->shutdownScanner()
#4 /var/www/html/apps-extra/files_antivirus/lib/AvirWrapper.php(118): OCA\Files_Antivirus\Scanner\ScannerBase->completeAsyncScan()
#5 [internal function]: OCA\Files_Antivirus\AvirWrapper->OCA\Files_Antivirus\{closure}()
#6 /var/www/html/apps/files_external/3rdparty/icewind/streams/src/CallbackWrapper.php(119): call_user_func(Object(Closure))
#7 [internal function]: Icewind\Streams\CallbackWrapper->stream_close()
#8 {main}
thrown at /var/www/html/lib/private/Log.php#316

I run into the above error when scanning a chunked upload after assembly. files_antivirus is using a callback for stream_close to fetch the status (infected yes/no) from clamav. When the stream is not closed explicitly it's happening so late that not even the autoloader is registered anymore.

Fallback implementation for storage already closes the stream on close:

ObjectStoreStorage is also closing the streams explicitly:


be3f4ed encryption

  1. The initial version c6a4811 were closing the input stream.
  2. don't close input stream when writing in encrypted file #13468 removed fclose($stream) to fixCan't upload empty file when encryption app is enabled #13276.
  3. In the meantime Don't fail if copying a file of 0 byte size #22739 update detection for failed writes to the storage. That's a better way to fix point 2.

I'm able to upload empty files with and without encryption (via web and nautilus) to local storage and object store.

@kesselb
kesselbforce-pushed the bug/noid/close-stream-local branch from a513df6 to d0383ddCompareSeptember 20, 2021 19:31
@kesselb

Copy link
Copy Markdown
ContributorAuthor

image

😟

@kesselbkesselb self-assigned this Sep 20, 2021
@blizzz

blizzz commented Sep 21, 2021

Copy link
Copy Markdown
Member

image

worried

It is not closed in

publicfunctionwriteStream(string$path, $stream, int$size = null): int {
// always fall back to fopen
$target = $this->fopen($path, 'w');
[$count, $result] = \OC_Helper::streamCopy($stream, $target);
fclose($target);
return$count;

Comment threadlib/private/Files/Storage/Local.php Outdated
@kesselbkesselb added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 21, 2021
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb
kesselbforce-pushed the bug/noid/close-stream-local branch from d0383dd to be3f4edCompareSeptember 21, 2021 15:42
@kesselbkesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 21, 2021
@kesselbkesselb changed the title explicitly close source stream on local storageexplicitly close source stream on local / encryption storageSep 21, 2021
@blizzzblizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 22, 2021
@kesselb
kesselb merged commit 9187e98 into masterOct 8, 2021
@kesselb
kesselb deleted the bug/noid/close-stream-local branch October 8, 2021 08:02
@kesselb

Copy link
Copy Markdown
ContributorAuthor

/backport to stable22

@kesselb

Copy link
Copy Markdown
ContributorAuthor

/backport to stable21

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't upload empty file when encryption app is enabled

4 participants

@kesselb@blizzz@nickvergessen@solracsf