Private Upload can change file settings back to 'private'
Jody Lynn - June 22, 2009 - 17:02
| Project: | Private Upload |
| Version: | 6.x-1.0-rc2 |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | reviewed & tested by the community |
Description
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.
| Attachment | Size |
|---|---|
| private_upload_file_downloads.patch | 1.48 KB |
#1
#2
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
Thanks for the clearer explanation. Setting to 'reviewed'.
#4
You might also want to modify the 'HACK' comment accordingly.
#5
#6
Seconded - this has broken our site three times today!! :o)
#7
#8
And there was me thinking i was going mad. This breaks my site every few days or so. Will test the next release.