Download & Extend
Private Upload » Issues

Private Upload can change file settings back to 'private'

Posted by Jody Lynn on June 22, 2009 at 5:02pm
Project:Private Upload
Version:5.x-1.x-dev
Component:Code
Category:bug report
Priority:critical
Assigned:Unassigned
Status:patch (to be ported)

Issue Summary

function _private_upload_create_url uses variable_set to momentarily change the file system to 'private' and then back to 'public'. In theory this should be harmless, but we've seen our production site get switched to 'private' filesystem twice now, presumably if something goes wrong in that function. This causes all kinds of problems and is not immediately detected.

This patch uses the variable storage global $conf to temporarily change the setting without actually saving it to the database. This approach seems less dangerous and better performance anyway.

AttachmentSize
private_upload_file_downloads.patch1.48 KB
Login or register to post comments

Comments

#1

Posted by Jody Lynn on June 23, 2009 at 2:29pm
Title:Private Upload can change private settings back to 'private'» Private Upload can change file settings back to 'private'

#2

Posted by jweowu on September 24, 2009 at 12:26am

In theory this should be harmless, but we've seen our production site get switched to 'private' filesystem twice now, presumably if something goes wrong in that function.

Using variable_set() is definitely not harmless. It would leave you open to race conditions when multiple requests execute that code simultaneously.

Assuming that the sequence is:
a) store the current setting
b) over-ride with 'private'
c) restore original setting

Then the following could happen:
1. method is 'public'
2. request 1 reads 'public'
3. request 1 writes 'private'
4. request 2 reads 'private'
5. request 2 writes 'private'
6. request 1 restores 'public'
7. request 2 restores 'private'
8. method remains 'private'

Using the global $conf array is correct. It affects the values that variable_get() returns, without writing that value to the database, so it only affects the current request.

#3

Posted by Jody Lynn on October 12, 2009 at 9:07pm
Status:needs review» reviewed & tested by the community

Thanks for the clearer explanation. Setting to 'reviewed'.

#4

Posted by jweowu on October 13, 2009 at 2:12am

You might also want to modify the 'HACK' comment accordingly.

#5

Posted by Jody Lynn on October 13, 2009 at 3:58am
AttachmentSize
private.patch 1.32 KB

#6

Posted by alexharries on October 27, 2009 at 11:18am

Seconded - this has broken our site three times today!! :o)

#7

Posted by Jody Lynn on October 28, 2009 at 10:46pm
Priority:normal» critical

#8

Posted by steev_initsix on November 26, 2009 at 11:43am

And there was me thinking i was going mad. This breaks my site every few days or so. Will test the next release.

#9

Posted by Jody Lynn on November 29, 2009 at 5:33pm
Version:6.x-1.0-rc2» 5.x-1.x-dev
Status:reviewed & tested by the community» patch (to be ported)

Committed to the D6 branch.

Needs to be tested for D5 version before being committed to D5.

#10

Posted by idmacdonald on December 2, 2010 at 5:59pm

This bug makes the 6.x-1.0-rc2 version of the module completely unusable for me. The bug has been fixed in the 6.x-1.x-dev version of the module. I think it would be good to release another stable version of the module soon. I know I prefer to not to run development versions of modules on live sites. And this bug means that the latest 'stable' version of the module is likely to cause major problems for many sites.

Thanks,
-Ian

nobody click here