Defect #33752: Uploading a big file fails with NoMemoryError - Redmine

Actions

Copy link

Defect #33752

closed

Uploading a big file fails with NoMemoryError

Added by Karel Pičman about 3 years ago. Updated over 2 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Go MAEDA

Category:

Attachments

Target version:

4.1.4

Start date:

Due date:

% Done:

Estimated time:

Resolution:

Affected version:

Description

Uploading of a file bigger than available RAM fails with no memory error. I've found the reason in request.raw_post which is String and doesn't respond to :read. Consequently, the whole file is read into memory. The attached patch simply replaces raw_post with body which is type of StringIO that provides read method.

Files

Download all files

big_files_upload.diff (594 Bytes) big_files_upload.diff		Karel Pičman, 2020-07-20 08:54
attachments_test.rb.patch (1.22 KB) attachments_test.rb.patch		Pavel Rosický, 2020-07-20 23:57
file_size.diff (1.16 KB) file_size.diff		Karel Pičman, 2020-07-21 09:37
10-patches.rb.patch (621 Bytes) 10-patches.rb.patch		Pavel Rosický, 2020-08-24 12:16
33752.patch (2.36 KB) 33752.patch	patch by Karel Pičman and Pavel Rosický	Go MAEDA, 2021-05-08 09:56

Related issues

Actions

Copy link

Updated by Go MAEDA about 3 years ago

Redmine originally used request.body, but changed to use request.raw_post in r9652 in order to fix an error regarding fastcgi (#10832). The attached big_files_upload.diff reverts r9652.

I think the patch can be merged to the core if the patch works fine with fastcgi.

Actions

Copy link

Updated by Pavel Rosický about 3 years ago

File attachments_test.rb.patch attachments_test.rb.patch added

Hi @Go MAEDA,
in my opinion, the fix in #10832 is wrong. It breaks the concept of streaming file uploads https://github.com/redmine/redmine/blob/master/app/models/attachment.rb#L126

however, the proposed patch breaks FCGI. I've attached a test case.

unlike other request handlers, CGI uses a rewindable input, that doesn't support the #size method. In theory, the only way how to determine the file size is to read the content first. The content should be streamed into a tempfile not into a memory.
https://github.com/rack/rack/blob/master/lib/rack/rewindable_input.rb

the second option is to rely on the CONTENT-LENGTH header, but it might not be accurate or it could be faked.

after some investigation, I found that Rails actually relies on the header
https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/http/request.rb#L327

this means that FCGI is currently broken if an invalid CONTENT-LENGTH is provided. Note that most web-browsers and curl do send this header by default.

I'm wondering why anyone wants to use FCGI these days, they're definitely better options. We should try to fix it if possible, but the original patch #10832 broke other webservers that behave correctly.

Actions

Copy link

Updated by Karel Pičman about 3 years ago

File file_size.diff file_size.diff added

I think that the problem with the size can be solved as suggested by Pavel by getting the size after data are stored into the filesystem. See the patch.

Actions

Copy link

Updated by Go MAEDA about 3 years ago

Target version set to Candidate for next minor release

Actions

Copy link

Updated by Go MAEDA about 3 years ago

A test is broken after applying big_files_upload.diff and file_size.diff.

Failure:
Redmine::ApiTest::AttachmentsTest#test_POST_/uploads.xml_should_return_errors_if_file_is_too_big [test/integration/api_test/attachments_test.rb:207]:
Expected response to be a <422: Unprocessable Entity>, but was a <201: Created>
Response body: <?xml version="1.0" encoding="UTF-8"?><upload><id>24</id><token>24.1d1801f753ccd9fa57966c46f360585caf83337a394a5f238d4e4e7d6005788d</token></upload>.
Expected: 422
  Actual: 201

bin/rails test test/integration/api_test/attachments_test.rb:204

Actions

Copy link

Updated by Pavel Rosický about 3 years ago

File 10-patches.rb.patch 10-patches.rb.patch added

there're validations for size before the file is actually uploaded.

let's choose a simpler approach. These patches should be commited:
big_files_upload.diff
10-patches.rb.patch
attachments_test.rb.patch

it passes all tests.

Actions

Copy link

Updated by Pavel Rosický over 2 years ago

@Go MAEDA may I ask for a review? it's a simple change.

Actions

Copy link

Updated by Go MAEDA over 2 years ago

Target version changed from Candidate for next minor release to 5.0.0

Actions

Copy link

Updated by Jens Krämer over 2 years ago

+1, that may really help installations on VPS or other small-memory machines. The rack patch for FCGI support has been merged already (not released unfortunately, appears it'll be in Rack 3.0.0)

Actions

Copy link

#10

Updated by Go MAEDA over 2 years ago

File 33752.patch 33752.patch added
Subject changed from Uploading big files to Uploading a big file fails with NoMemoryError
Target version changed from 5.0.0 to 4.1.4

Pavel Rosický wrote: