file_save_upload() not notifying user when PHP upload limit is exceeded.

Confirmed. It's still a bug.

Still a bug in the latest 4.7. I was just looking at php.net and saw that...

If the size of post data is greater than post_max_size, the $_POST and $_FILES superglobals are empty. This can be tracked in various ways, e.g. by passing the $_GET variable to the script processing the data, i.e. <form action="edit.php?processed=1">, and then checking if $_GET['processed'] is set.

I think the upload creates an invisible IFRAME or something to that effect. So maybe we should check to see if a $_GET var is set, if not we should return an error saying the file exceeded the PHP configuration limits.

Regardless, what is happening now is users are uploading files that are too large and the upload form is just returning blank. Meaning if you have a file uploaded already, and try to upload another that is too large, it will fail and remove the existing file from the list.

Confirmed.

1. when uploads a file larger than PHP limit it fails without a message
2. when uploads a file larger than PHP limit, it removes the already uploaded files

I can confirm this bug in 5.1

This has been open since 4.6, could somebody please look into it.

Moving to 6.x-dev. Hope I get some time to work on my old issues before the freeze...Anyone have any ideas on this?

Check out related issue: http://drupal.org/node/104220.

it looks like the problem goes back to the 4.7 days and the problem is that we check that is_uploaded_file() is true before checking for errors one of the comments on the patch alludes to the problem.

i've reworked the test so that it only checks that the file is_uploaded_file() when there's no error.

Tested the patch - I'm not seeing any change though, large files still fail silently.
There is a notice though: 'notice: Undefined property: stdClass::$vid in /modules/upload/upload.module on line 536.'

I can confirm this, so i change the status.

here's a slightly improved version. it gives better errors.

i think the problem you guys are running into trying to test this is that it depends on php's max post and file upload sizes. if you upload a file bigger than the post max size you won't see any error because php silently discards the submission. so for example with:

post_max_size = 32M
upload_max_filesize = 4M

uploading a 1mb file would work. uploading an 8mb file would fail with an error. but if you upload a 38mb file you'd loose everything submitted with the form. there's not much we can do about this.

also when you're testing this, use the preview button rather than the attach button. there's some issues with the javascript that are separate from this.

i want to clarify that this patch only fixes one thing: if you submit a file that's larger than php's max file size and less than the max post size you will now get an error. we can't do anything about uploads that are larger than the post size.

Because of the unique error it drops on reaching the POST size limit I thought it is possible to filter out the conditions when this occurs. This is a try. This patch gives correct error messages when on node/add/story, but removes the previous attachments when editing an existing node and encountering the POST size limit. Maybe someone finds this useful.

I've rerolled the patch:
- Fixed an incomplete comment.
- Removed the separate error messages since they weren't actually being used (if the file is larger than the user roles maximum file size, but less than the PHP maximum, the file uploads properly and the error is thrown later)

i tested out snufkin's patch on #12 and it didn't seem to report an error for a file larger than the max post size.

webernet, i think your patch on #13 is a step backwards. you remerged distinct error messages. in one case it's larger than the max upload size in another someone put a constant on the form specifying a maximum upload size. totally different... a misleading error message is worse than none at all. i was trying to separate them out to make it clearer what had happened.

This patch once again splits the messages for the two cases.

i tried to improve the error messages a bit and used webernet's suggestion on IRC of using the filename rather than $source in the error messages.

php.net suggests (as quoted above) that we also pass a GET parameter, which signals that we should have files, and if we have no files, that means we have gone over the POST max size. We can easily use both POST and GET data, so we can detect the case when a file was expected but not uploaded at all.

Gabor, the attached patch would catch a single file that's oversize, the problem is when the whole request is larger than the max post size. in that case then entire request is discarded and you get a blank node upload form... should we be passing a default ?posted=1 to all pages?

@drewish: It sounds like that's what PHP recommends doing, I think that's a good idea.

RobRoy, I agree it's a good idea but I've got no idea where to start hacking the Forms API so that we pass a GET when submitting all the forms with '#type'=>'file' element on them.

Considering that at this point there's two errors that aren't handled correctly, it seems to me that fixing one would be a good start.

I came in late to this so probably don't grasp the whole situation, but just want to jot down some quick ideas that may inspire a real solution.

I was thinking we'd just do it manually somehow. Like in upload_form_alter() we would do a $form['#file_upload'] = TRUE; when adding the attachments fieldset. Then, when FAPI is gonna submit the form...it takes whatever $form['#action'] is set to and adds a file_upload=1 to the query string. Then, the submit handler could check for that.

But that isn't too well thought out, because I guess we only set file_upload=1 when a file upload is attempted, not just when a 'file' field is included in the form. So maybe all file FAPI elements have their own generic '#submit' handler called 'file_validate_form_upload' or something that does some magic based on data submitted.

Ehhh, ya those are some pretty worthless suggestions but might get the balls rolling. :P

bump.

Can we agree this is a problem, and that having the problem continue to exist since late 2005 is pretty sad?

How about a fix that deals with some of the above, rather than leave the entire issue outstanding?

bumping to HEAD, hopefully we can backport something once this gets committed.

marked #282605: New Audio content never created if file exceeds max upload (no warning given) as a duplicate of this.

also marked #248185: Wrong error management in file_save_upload as a duplicate. improving the title to make it easier to find

Looks like the patch is not specific to upload.module.

I tried the last patch and it worked for me on drupal 5.
This patch is for drupal 5 (the check was in a function called file_check_upload).

If The file size exceeds the post_max_size what about calling error_get_last() ?

The php generate this warning :
PHP Warning: POST Content-Length of (failed uploaded size) bytes exceeds the limit of (post_max_size) bytes

Marked #289518: No error message for attachment larger than PHP settings limit as a duplicate.

here's a reroll against HEAD.

DrupalTestbedBot is a wrong, looks like it grabbed the previous patch that yassin.mohii posted.

Update: Reported an issue for the test bot #307197: Testing the incorrect patch posting multiple comments.

re-rolled for HEAD. if you want unit tests take a look at #310358: Add a test for file_save_upload and clean up file.test

i'm going to drag the DrupalTestbedBot out back and shoot it: http://drupal.org/node/307197

last patch was rolled from /includes... re-rolled.

I have drupal 5.12 and I could only apply the patch attached to comment #29, the rest fail for my system.

Anyway the patch only solves the problem of submitting a file greater than php upload_max_filesize AND smaller than post_max_size
If post_max_size is exceeded the problem continues.

here's a re-roll that restructures this a bit so that if there's no upload we hurry up and just return FALSE rather than indenting the rest of the code. all file and upload tests pass.

re-rolled.

Excellent! This has bitten me in the past and I'm very happy to see a working patch!

Tested by setting my max_post_size to 32M and max_upload_filesize to 1M and uploading a 3M file. Without the patch, nothing happens, leaving me scratching my head and feeling lost and alone. With the patch, I get a nice error message to tell me what happened.

Code looks good (it's basically just a re-shuffling) and file tests continue to pass. I asked drewish if we could supplement this with unit tests, but unfortunately we cannot; both of these INI values are not settable at run-time.

Committed to HEAD. Marking for porting to 6.x/5.x.

Humm... not quite yet. I just noticed one other test case, submitting the form at admin/build/themes/settings with no files incorrectly gave the following errors:

    * The file logo_upload could not be saved, because the upload did not complete.
    * The file favicon_upload could not be saved, because the upload did not complete.

I'm working on another patch right now but I'll pick this up when I'm finished.

FileSaveUploadTest really needs to add tests for submitting an empty upload to an optional and required fields.

This doesn't address the test yet but now it returns NULL if no upload was attempted.

Marked #357382: Error message shown when no upload file provided as a duplicate of this bug. The patch in #46 is contains unwanted debugging statement so posting a revised patch.

much better looking. i'd like to get tests but it might be worth holding off on for them for now and getting this fixed.

just double checked and this patch fixes the problem in all the file elements that are currently in core (theme logo/favicon, user pictures, upload.modue).

Tested, works, RTBC.

Oh my...trying to write tests for this is just horrible. Can't use drupalPost() with an empty file field.

Looks like dagnabbed artwork... works for me... and if it can't be tested I see no reason to hold it up.

I think I finally figured out a test for this. Before the change to file_save_upload(), this new test will fail. With the change, it passes.

Cleaned out the unrelated whitespace changes and documentation revision for the test.

thanks, #54 looks good and tests pass. RTBC.

Yay! :) Really glad to have this fixed AND tested so we don't break it again. :D

Committed to HEAD.

From #44/#45, it looks like we need this backported.

Here's the backport for #43 and #54 combined. I tested uploading a new user picture on my 6.x patched install and it worked successfully. The only API change is that the function returns FALSE/NULL instead of 0, which all == FALSE.

Revised patch that includes the more helpful error messages in #361514: Error messages in file_save_upload should use the actual file name without changing any actual strings for translation.

Is there any way to backport this to 5.x ?? I just found this error on my site today, and I won't be changing to 6.x for quite awhile. Thanks!

+1 on the backport please! I also just found this issue.

Feel free to review the patch and if it works for you, mark as "reviewed and tested by the community"

Confirmed this patch fixes problems with both Upload module and FileField in Drupal 6. The patch looks large but doesn't change nearly as many lines as it appears to. A huge improvement to prevent upload.module from silently failing without giving the user any indication of an error whatsoever.

Tested by:
- Setting post_max_size to 10M and upload_max_filesize to 1M. Restart Apache.
- Uploaded a 2M file in upload module and in FileField. FileField reports unhelpfully "The file in the field was unable to be uploaded.", Upload doesn't report anything at all. After patching, both report "The file IMG_1208.JPG could not be saved, because it exceeds 1 MB, the maximum allowed size for uploads."
- Uploaded a 500K file to ensure files are still accepted.

Both modules still fail pretty spectacularly when an upload exceeds the post_max_size (the form element disappears entirely, preventing any uploads), but that's a different issue. Great work on solving this one.

Pretty big patch indeed, so I'd welcome if more people would test it before RTBC. I for example found a function called which does not even exist in Drupal 6: file_load_multiple(). This function is also used in a passage of code which is not in the Drupal 6 version currently and is not related to fixing this bug either.

Gabor's right this whole block of code in D7 only:

+  // If we are replacing an existing file re-use its database record.
+  if ($replace == FILE_EXISTS_REPLACE) {
+    $existing_files = file_load_multiple(array(), array('filepath' => $file->filepath));
+    if (count($existing_files)) {
+      $existing = reset($existing_files);
+      $file->fid = $existing->fid;
+    }
+  }

I'm have also had issues with this, but checks on anything after the max size of $_POST is exceeded is almost pointless as the entire reference to the form build id is lost. The only solution that I have found so far is to change theme_form to append the action with the build id. This ensures that this is never lost and a failed POST can be detected.

Rather than pushing in a patch, I just post the code to get feedback on the idea.

<?php
// Original
function theme_form($element) {
  // Anonymous div to satisfy XHTML compliance.
  $action = $element['#action'] ? 'action="' . check_url($element['#action']) . '" ' : '';
  return '<form ' . $action . ' accept-charset="UTF-8" method="' . $element['#method'] . '" id="' . $element['#id'] . '"' . drupal_attributes($element['#attributes']) . ">\n<div>" . $element['#children'] . "\n</div></form>\n";
}

// Modified
function theme_form($element) {
  // PHP drops the $_POST array if this exceeds the PHP setting post_max_size.
  $action = '';
  if ($element['#action']) {
    $action = 'action="' . check_url($element['#action']) . (strpos($element['#action'], '?') !== FALSE ? '&' : '?') . 'form-id=' . check_plain($element['#build_id']) . '"';
  }
  // Anonymous div to satisfy XHTML compliance.
  return '<form ' . $action . ' accept-charset="UTF-8" method="' . $element['#method'] . '" id="' . $element['#id'] . '"' . drupal_attributes($element['#attributes']) . ">\n<div>" . $element['#children'] . "\n</div></form>\n";
}

?>

So with the modifications, a var_dump on $_GET and $_POST normally will result in something like:

<?php
var_dump($_GET);
/*
Outputs
array(2) {
  ["q"]=>
  string(31) "admin/settings/site-information"
  ["form-id"]=>
  string(37) "form-f3b2569b7299435726c96c962dab3d89"
}
*/
var_dump($_POST);
/*
array(11) {
  ["site_name"]=>
  string(5) "d7dev"
  ["site_mail"]=>
....
}
*/
?>

But if max_post_size is exceeded:

<?php
var_dump($_GET);
/*
Outputs
array(2) {
  ["q"]=>
  string(31) "admin/settings/site-information"
  ["form-id"]=>
  string(37) "form-f3b2569b7299435726c96c962dab3d89"
}
*/
var_dump($_POST);
/*
array(0) {
}
*/
?>

In both cases, we can still retain the actual form that was originally posted and also detect if the form data has been dropped.

Alan D., I think that's an interesting approach but it should really be in a follow up issue since it seems a bit too big to get backported for D6.

Followup on #66, see Issue: Enabling handling of errors related to max_post_size.

+1 to backport to 5.x

flashvideo module needs this fix (http://drupal.org/node/463126)

Hello,

I was wondering if there has been further development in commiting this to D6?
I came across this issue today and glad to have come across this issue post.

Regards

subscribing

Patch no longer applied, here is a rerolled patch that removes the Drupal 7 - specific code block.

This issue bit me yesterday - very hard to debug since there is no error in the Apache error log.

Looking forward to a fix.

subscribing - hansfn, the error goes to the php log, not apache

Thx for caring dwight, but if you are running PHP as an Apache module, the default behavior is to write PHP error messages to Apache's error log. And yes, all other PHP errors display in the Apache error log.

Redo of the patch file against what I got from CVS for DRUPAL-6.

I'm seeing the better error handling for uploads and filefields.

This patch is removing the second file_munge_filename() call on files that have had ".txt" added because they have dangerous extensions (php, js, etc) - is that a carryover of a different fix from D7? It seems like an OK change to me but I don't know enough of the background to really tell.

5 years later it is still an error ;)

The patch in #78 is missing a closing bracket ")"

drupal_set_message(t('Missing a temporary folder', array(), 'error');

should be

drupal_set_message(t('Missing a temporary folder'), array(), 'error');