Race condition on _private_upload_create_url
| Project: | Private Upload |
| Version: | 5.x-1.0-rc2 |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | active |
Jump to:
Hello, first of all excuse me for my poor english.
I've recently implemented private upload in a heavy loaded comunnity site (an average of 200 loged users). Sortly after I detected that user's avatars where not showing and after a little investigation I've found a race condition in this module's code.
Let me explain the conditions.
First of all let's see the afected code:
function _private_upload_create_url($file) {
if (_private_upload_is_file_private($file->filepath)) {
$download_method = variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC);
variable_set('file_downloads', FILE_DOWNLOADS_PRIVATE);
}
// Generate valid URL for both existing attachments and preview of new atta
$href = file_create_url((strpos($file->fid, 'upload') === FALSE ? $file->file->filepath : file_create_filename($file->filename, file_create_path())));
if (_private_upload_is_file_private($file->filepath)) {
variable_set('file_downloads', $download_method);
}
return $href;Imagine 2 nearly simoultanious calls to the function _private_upload_create_url (c1 and c2) with the same $file (a private one) executing in this order.
c1: Enters the function and the first if because the file is private. It sets the variable 'file_downloads' (variable wich stores the site's download method) to private storing the previous value (FILE_DOWNLOADS_PUBLIC by module's requirements) in $download_method.
c2: Enters the function and the first if because the file is private. Since in this moment variable 'file_downloads' has the value FILE_DOWNLOADS_PRIVATE it stores this value in $download_method.
c1: Continues executing and before returning $href sets the variable 'file_downloads' to the right value of FILE_DOWNLOADS_PUBLIC (previously stored)
c2: Continues executing and before returning $href sets the variable 'file_downloads' to the WRONG value of FILE_DOWNLOADS_PRIVATE (previously stored)
This lead to inconsistences in site's configuration on a busy server.
I've workarounded the issue incoprporing the code of file_create_url (the only function called wich depends on the value of 'file_downloads') to the _private_upload_create_url function (quick and dirty fix):
<?php
function _private_upload_create_url($file) {
// BY NITEMAN
//
// based on <a href="http://api.drupal.org/api/function/file_create_url/5
//
//" title="http://api.drupal.org/api/function/file_create_url/5
//
//" rel="nofollow">http://api.drupal.org/api/function/file_create_url/5
//
//</a> if (_private_upload_is_file_private($file->filepath)) {
// $download_method = variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC); // this should be PUBLIC, but don't break misconfigured systems
// variable_set('file_downloads', FILE_DOWNLOADS_PRIVATE);
// }
// // Generate valid URL for both existing attachments and preview of new attachments (these have 'upload' in fid)
// $href = file_create_url((strpos($file->fid, 'upload') === FALSE ? $file->filepath : file_create_filename($file->filename, file_create_path())));
// if (_private_upload_is_file_private($file->filepath)) {
// variable_set('file_downloads', $download_method);
// }
$href = (strpos($file->fid, 'upload') === FALSE ? $file->filepath : file_create_filename($file->filename, file_create_path()));
if (strpos($path, file_directory_path() . '/') === 0) {
$path = trim(substr($path, strlen(file_directory_path())), '\\/');
}
if (_private_upload_is_file_private($file->filepath)) {
return url('system/files/'. $href, NULL, NULL, TRUE);
} else {
return $GLOBALS['base_url'] .'/'. file_directory_path() .'/'. str_replace('\\', '/', $href);
}
// END BY NITEMAN
return $href;
}
?>I've also reviewed 6.x-1.0-rc2 code an it's affected as well by this issue.
Thanks in advance.
#1
I made a litle mistake in the quick & dirty fix, the correct code is:
<?phpfunction _private_upload_create_url($file) {
// BY NITEMAN
//
// based on <a href="http://api.drupal.org/api/function/file_create_url/5
//
$href = (strpos($file->fid, 'upload') === FALSE ? $file->filepath : file_create_filename($file->filename, file_create_path()));
if (strpos($href, file_directory_path() . '/') === 0) {
$href = trim(substr($href, strlen(file_directory_path())), '\\/');
}
if (_private_upload_is_file_private($file->filepath)) {
return url('system/files/'. $href, NULL, NULL, TRUE);
} else {
return $GLOBALS['base_url'] .'/'. file_directory_path() .'/'. str_replace('\\', '/', $href);
}
// END BY NITEMAN
return $href;
}
?>