File uploads silently failing on Windows when magic_quotes_gpc = On: fix_gpc_magic() strips legitimate backslashes from $_FILES [#91624]

Comment	File	Size	Author
#33	common.inc.4.7.patch	1.19 KB	RobRoy

#28	fix_gpc_files.patch	1.09 KB	chx

#27	fix_gpc_win_1.patch	1.29 KB	RobRoy

#14	fix_gpc_win_0.patch	1.26 KB	RobRoy

#1	fix_gpc_win.patch	1.33 KB	RobRoy

Comment #1

RobRoy CreditAttribution: RobRoy commented 27 October 2006 at 19:44

Title:	File uploads silently failing: fix_gpc_magic() strips legitimate backslashes from $_FILES on IIS when magic_quotes_gpc = On	» File uploads silently failing on Windows when magic_quotes_gpc = On: fix_gpc_magic() strips legitimate backslashes from $_FILES
Status:	Active	» Needs review

File	Size
fix_gpc_win.patch	1.33 KB

Okay, so this applies to all Windows installations, but with Apache you most likely have .htaccess turning off magic_quotes_gpc for you. WIth IIS it is a much bigger problem. It took me hours to finally track this down to fix_gpc_magic().

Bdragon confirmed my suspicions and pointed out a comment at http://us2.php.net/manual/en/features.file-upload.php#42280

So this patch checks if we're on Windows, and will stripslashes on the $_FILES array while avoiding tmp_name. There are quite a few posts on drupal.org with failed uploads for Windows with errors like this:

warning: copy(c:\inetpub\wwwroot\files) [function.copy]: failed to open stream: Permission denied in c:\inetpub\wwwroot\includes\file.inc on line 374.

I'll roll one for HEAD after some reviews.

Log in or register to post comments

Comment #2

tvoorter CreditAttribution: tvoorter commented 10 November 2006 at 02:23

I can confirm that this patch works for me. I had problems uploading images and files with 4.7 on windows 2003 server IIS 6. This did the trick. Thanks!

Log in or register to post comments

Comment #3

MsDetta CreditAttribution: MsDetta commented 10 November 2006 at 15:03

Title:

File uploads silently failing on Windows when magic_quotes_gpc = On: fix_gpc_magic() strips legitimate backslashes from $_FILES

» Where is php.ini file?

but with Apache you most likely have .htaccess turning off magic_quotes_gpc for you

My server is Apache 1.3.37 and I'm experiencing this problem with uploading files. I can't upload anything to the files directory without getting a fatal error message on the server. The files/images shows 775, but when I click the images to change it to 777, it shows as 777. If I try to save the setting, it returns fatal error.

I tried to change the magic_quotes_gpc in the site/default/settings.php and it didn't change the .htaccess file. I tried to change the .htaccess file, and it returned fatal error.

I looked through all the directory folders, and I can't find the php.ini file - where can I find it? Or will the above patch fix my problem on Apache?

Log in or register to post comments

Comment #4

RobRoy CreditAttribution: RobRoy commented 10 November 2006 at 21:57

Title:

Where is php.ini file?

» File uploads silently failing on Windows when magic_quotes_gpc = On: fix_gpc_magic() strips legitimate backslashes from $_FILES

Please don't change the subject.

php.ini is usually in /etc on Linux and C:\PHP or C:\WINDOWS on Windows. This would fix your problem if magic_quotes_gpc = On.

Log in or register to post comments

Comment #5

Chill35 CreditAttribution: Chill35 commented 14 November 2006 at 08:53

I tested the latest release as well as 5.x. The upload module just didn't work.

I had to change the line in php.ini :

magic_quotes_gpc = On

to

magic_quotes_gpc = Off

Otherwise, the upload module WILL NOT WORK.

That modification in the php.ini file might create other problems...

I did NOT install the patch.

I think that should be in the documentation somewhere... don't you?

It seems that the patch given here has not been reviewed ?

So it's not included in 4.7, neither in 5.x

That's a real shame considering many people will have a testing server installed on Windows.

My testing server runs on Windows XP SP 2.

I have Apache 2.0.59 and PHP 5.1.4.

Log in or register to post comments

Comment #6

Heine CreditAttribution: Heine commented 14 November 2006 at 08:58

Version:

4.7.4

» 5.x-dev

Moving to 5-dev to increase the review chance.

Log in or register to post comments

Comment #7

RobRoy CreditAttribution: RobRoy commented 14 November 2006 at 17:17

"I did NOT install the patch."

@Chill35: Can you turn magic_quotes_gpc = On and actually test the patch out. This patch is designed to fix the totally borked uploads when magic_quotes_gpc = On on Windows. You testing the patch and reviewing it will help get this in to Drupal.

Thanks!

Log in or register to post comments

Comment #8

Chill35 CreditAttribution: Chill35 commented 14 November 2006 at 22:41

I tested the patch on a Windows XP SP 2 Apache 2.0.59 php 5.1.4 server.

Before the patch, uploading files didn't work.
After the patch, uploading works.

I modified the php.ini file to the way it was originally, i.e. magic_quotes_gpc = On

I tested both with 4.7.4 and 5.x. Both had the problem.

Do I change the status to : 'patch (ready to be committed)' ? I am a newbie, I don't know how these things work.

Is it possible to include the patch in the current release as well ? Right now it is set to Version 5.x-dev.

I am afraid to make a mistake.

Log in or register to post comments

Comment #9

Chill35 CreditAttribution: Chill35 commented 14 November 2006 at 22:46

Can you turn magic_quotes_gpc = On and actually test the patch out.

Sorry I had not seen that.

Yes, I put the magic_quotes_gpc back to On in php.ini, I restarted my server, tested on both 4.7 and 5 to see if the problem was back. The problem was back. Then I installed the patch on both 4.7 and 5, tested both 4.7 and 7 and BINGO, it works very well now.

Thanks!

Log in or register to post comments

Comment #10

Chill35 CreditAttribution: Chill35 commented 14 November 2006 at 22:48

Do I change the status to : 'patch (ready to be committed)' ?

Log in or register to post comments

Comment #11

Chill35 CreditAttribution: Chill35 commented 14 November 2006 at 23:05

In http://drupal.org/contribute/testing

It says :

If there's a patch, test it. See below for further details.

Testing patches

// TODO

LOL... so I don't know how to review a patch officially. Sorry.

I'll have to go.

Log in or register to post comments

Comment #12

RobRoy CreditAttribution: RobRoy commented 14 November 2006 at 23:24

Hehe, that's pretty funny. Your review was excellent, that's exactly what a review is and helps the big boys see that this actually fixes something that was broken. I'll get somebody to mark this RTBC. Thanks for the review!

Log in or register to post comments

Comment #13

RobRoy CreditAttribution: RobRoy commented 14 November 2006 at 23:27

Priority:

Normal

» Critical

Marking this critical. Although this only applies to Windows users with magic_quotes_gpc On, this totally borks file uploads and doesn't even provide a descriptive message. This patch fixes that behavior.

Log in or register to post comments

Comment #14

RobRoy CreditAttribution: RobRoy commented 14 November 2006 at 23:54

File	Size
fix_gpc_win_0.patch	1.26 KB

dopry let me know that the second key of array_walk() is key, so I've simplified this a bit. Sure did seem like there should have been a PHP function for this. Please retest.

Log in or register to post comments

Comment #15

Chill35 CreditAttribution: Chill35 commented 15 November 2006 at 02:09

I retested. Here is what I did :

For both installations, 4.7.4 and 5.0, I overwrote the includes/common.inc file with its original file. I tested to see if I could experience the problem again. And I did. So that was good.

Then I made sure to use the right patch, the second one :fix_gpc_win_0.patch (and not fix_gpc_win.patch) and patched both includes/common.inc files, in my 4.7.4 and 5.0 installations. With that patch.

Upload is fixed with that new patch (fix_gpc_win_0.patch) in both 4.7 and 5.

Thank you much!

Log in or register to post comments

Comment #16

Chill35 CreditAttribution: Chill35 commented 15 November 2006 at 02:19

I noticed something interesting, RobRoy, that probably is not a bug. More like a feature LOL!

When I upload files that contain in their names funny characters such as an apostrophe, the file IS uploaded successfully, thank God, but the file uploaded has a new name that has, from its original name, the part up to, and including, the apostrophe, amputated.

Actual (real) example : Legros'-team.jpg

in 4.7 > the file uploaded has that name > -team.jpg

in 5 > the same file has the name > -team.jpg

Now that's not only the name as it is shown in the post or story (as attachment), it is also the actual name of the file uploaded (in /files/...).

Log in or register to post comments

Comment #17

Chill35 CreditAttribution: Chill35 commented 15 November 2006 at 02:33

There only seems to be a "problem" with the apostrophe.
Whitespace and accentuated characters are ok.

Other examples :

Original names :

le 3.jpg
l'eau dans l'bain.jpg
l'eau.jpg
beau trésor.jpg

New names (after upload) :

le 3.jpg (OK)
bain.jpg (truncated)
eau.jpg (truncated)
beau trésor.jpg (OK)

All the word up to and including the last apostrophe is truncated.

Log in or register to post comments

Comment #18

RobRoy CreditAttribution: RobRoy commented 15 November 2006 at 02:34

I've noticed that behavior both with and without this patch and think it may be how PHP deals with funny names like that. Thanks for the new review. Someone please mark this RTBC. :)

Log in or register to post comments

Comment #19

Chill35 CreditAttribution: Chill35 commented 15 November 2006 at 02:42

Status:

Needs review

» Reviewed & tested by the community

:) done

Log in or register to post comments

Comment #20

chx CreditAttribution: chx commented 15 November 2006 at 07:17

Priority:	Critical	» Normal
Status:	Reviewed & tested by the community	» Needs work

This smells like a bogus issue. If this would be a real issue, upload would have never worked, and almost surely someone would have complained for a PHP bug, too because we are not alone to use stripslashes to offset magic_quotes_gpc. bugs.php.net search for magic_quotes_gpc upload on Windows comes up almost empty handed.

At least two issues linked are not definitely about GPC. http://drupal.org/node/63536 says "he selected file C:\PHP\uploadtemp\tmp3.tmp could not be copied." so the file is there and http://drupal.org/node/81613#comment-150195 found that a mere change to his temporary directory is enough.

Also the patch is a hack.

Log in or register to post comments

Comment #21

sepeck CreditAttribution: sepeck commented 15 November 2006 at 07:52

Tested on Windows 2003 Server, Apache 2.0
PHP Version 4.3.10

Drupal 5-head (updated on 13nov06)

I turned magic quotes ON. Restarted Apache and was able to upload files and images.

Log in or register to post comments

Comment #22

sepeck CreditAttribution: sepeck commented 15 November 2006 at 07:52

forgot something. I did not apply any patch or change any files for this.

Log in or register to post comments

Comment #23

Heine CreditAttribution: Heine commented 15 November 2006 at 08:04

Note the directives in .htaccess:

# PHP 4, Apache 2
<IfModule sapi_apache2.c>
  php_value magic_quotes_gpc                0
  [snip]
</IfModule>

# PHP 5, Apache 1 and 2
<IfModule mod_php5.c>
  php_value magic_quotes_gpc                0
 [snip]
</IfModule>

Setting magic_quotes_gpc to 1, will cause upload to fail on PHP 5.1.4.

Log in or register to post comments

Comment #24

RobRoy CreditAttribution: RobRoy commented 15 November 2006 at 08:08

Status:

Needs work

» Needs review

This smells like a bogus issue. If this would be a real issue, upload would have never worked

Not correct. Most installations have (a) magic_quotes_gpc turned Off in php.ini or (b) are on Apache so the "php_value magic_quotes_gpc 0" in .htaccess is getting run even if for some reason they have them turned On in php.ini.

To easily test on Apache/Windows, change "php_value magic_quotes_gpc 0" to "php_value magic_quotes_gpc 1" in the default .htaccess and try to upload something.

Log in or register to post comments

Comment #25

RobRoy CreditAttribution: RobRoy commented 15 November 2006 at 08:11

Also the patch is a hack.

Oh, how gracious and constructive of you to say. Thank you! :P Well, I've set it back to code needs review until you tell me what to fix.

Also, I just quickly included those related forum posts and did not fully look into each one. They are just possibly related to this issue, but this issue is definitely real.

Log in or register to post comments

Comment #26

sepeck CreditAttribution: sepeck commented 15 November 2006 at 08:11

Rob made me test with htaccess file set
php_value magic_quotes_gpc 1

files do not upload with this setting in htaccess on Windows 2003 Server with Apache 2

Still need to test patch later.

Log in or register to post comments

Comment #27

RobRoy CreditAttribution: RobRoy commented 15 November 2006 at 08:34

File	Size
fix_gpc_win_1.patch	1.29 KB

chx said to take out the windows specific stuff. "FILES needs a different strip routine because per that comment tmp_name does not have backslashes added. call your routine _fix_gpc_magic_files."

Even though this is a windows specific behavior, while dopry and I discussed earlier that we save one conditional over if we just did a _fix_gpc_magic_files(), it is cleaner to do it this way as chx recommended.

Re-added code comment that got lost from original patch "tmp_name does not have backslashes added see http://us2.php.net/manual/en/features.file-upload.php#42280 ".

Log in or register to post comments

Comment #28

chx CreditAttribution: chx commented 15 November 2006 at 08:38

Status:

Needs review

» Reviewed & tested by the community

File	Size
fix_gpc_files.patch	1.09 KB

I also said that the real problem here is that (per that php.net comment) slashes are not added to tmp_name so then we shall not remove then. If we do this, a very useful byproduct will be that Windows does not break. Other effects could be, do not know, if some other OS changes the function that generates tmp file names to have a backslash, that also won't break. But anyways, if something is not added, we shall not remove.

Rerolled the patch, it was slightly malformed. Otherwise good.