WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 3 months ago

#13575 closed defect (bug) (invalid)

wp_max_upload_size() is not taking memory limit into account

Reported by: hakre Owned by: dd32
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Upload Keywords: has-patch needs-refresh
Focuses: Cc:

Description

wp_max_upload_size() is not taking a memory limit into account while checking for the maximum upload size.

according to the official php documentation the memory limit is limiting file uploads as well (and post form operations).

post_max_size - integer

Sets max size of post data allowed. This setting also affects file upload. To upload large files, this value must be larger than upload_max_filesize. If memory limit is enabled by your configure script, memory_limit also affects file uploading. Generally speaking, memory_limit should be larger than post_max_size.

Source

Attachments (6)

13575.patch (734 bytes) - added by hakre 6 years ago.
13575.php (558 bytes) - added by hakre 5 years ago.
ini_set(), ini_get() and ini_restore() php example
13575.2.patch (871 bytes) - added by hakre 5 years ago.
Updated patch reflecting original php.ini memory_limit value and only if memory_limit is active. Containing a filter.
13575.3.patch (839 bytes) - added by hakre 5 years ago.
Updated patch reflecting original php.ini memory_limit value and only if memory_limit is active. W/o a filter.
13575.4.patch (842 bytes) - added by hakre 5 years ago.
Better compare against -1 for no memory limit.
13575.5.diff (833 bytes) - added by antpb 4 months ago.

Download all attachments as: .zip

Change History (20)

@hakre
6 years ago

#2 @hakre
6 years ago

  • Keywords has-patch added
  • Milestone changed from Unassigned to Future Release

It might be better to name the filter upload_memory_limit, so take it as suggestion only - if filtering that is even considered.

#3 @hakre
6 years ago

Related: #13418

#4 follow-up: @dd32
6 years ago

A filter is not needed here, However, the ini_get() may return incorrect information, It may have been upped already by something such as WP_MEMORY_LIMIT.

@hakre
5 years ago

ini_set(), ini_get() and ini_restore() php example

#5 @hakre
5 years ago

Replying to dd32:

I added a demo script which outputs the following (Fedora 13):

initial value: "128M"
new value: "42M"
old value: "128M"
current value: "42M"
original value: "128M"

So the original value should be taken into account as that's the one that comes into play at times of the upload procedure.

@hakre
5 years ago

Updated patch reflecting original php.ini memory_limit value and only if memory_limit is active. Containing a filter.

@hakre
5 years ago

Updated patch reflecting original php.ini memory_limit value and only if memory_limit is active. W/o a filter.

@hakre
5 years ago

Better compare against -1 for no memory limit.

#6 in reply to: ↑ 4 @hakre
5 years ago

Replying to dd32:

A filter is not needed here, However, the ini_get() may return incorrect information, It may have been upped already by something such as WP_MEMORY_LIMIT.

I've uploaded some new patches that reflect both concerns, the filter and the original value. This still needs some tests (e.g. what happens if 64 MB are consumed but setting get's restored to 16 MB original value - would that do a fatal error?) but tries to address accessing the original settings value.

#7 @hakre
5 years ago

Did run some tests now, lowering the memory_limit configuration value while having more memory in use than the set value will not fatal error. Method in patch looks save.

#8 @iseulde
3 years ago

  • Component changed from General to Upload

#9 @chriscct7
5 months ago

  • Keywords needs-refresh added

@antpb
4 months ago

#10 @antpb
4 months ago

  • Keywords dev-feedback added; needs-refresh removed
  • Milestone changed from Future Release to 4.4

Refreshed patch above. Added syntax to the proper file and updated the deprecated function changed in v2.3

wp_convert_bytes_to_hr


is now

size_format

#11 @wonderboymusic
4 months ago

  • Owner set to wonderboymusic
  • Status changed from new to assigned
5.2.1	Compiling with --enable-memory-limit is no longer required for this function to exist.

#12 @wonderboymusic
4 months ago

  • Keywords needs-refresh added; dev-feedback removed

#13 @wonderboymusic
3 months ago

  • Owner changed from wonderboymusic to dd32

#14 @dd32
3 months ago

  • Milestone 4.4 deleted
  • Resolution set to invalid
  • Status changed from assigned to closed

In the real world, the memory limit doesn't appear to limit the upload size.

With my memory limits set to 40M I'm able to upload a 100MB file without issue.

PHP's behaviour may have changed in the last few years, but the documentation now reads:

Sets max size of post data allowed. This setting also affects file upload. To upload large files, this value must be larger than upload_max_filesize. Generally speaking, memory_limit should be larger than post_max_size.

Since there's been no reports of issues surrounding this, i'm marking this as invalid unless someone can track down where/when it affected PHP.

Note: See TracTickets for help on using tickets.