search

jugmac00 / flask-reuploaded

File uploads for Flask.
https://github.com/jugmac00/flask-reuploaded
MIT License
63 stars 12 forks source link

Possible race condition #139

Open tabebqena opened 2 years ago

tabebqena commented 2 years ago

This is edje case at which, 2 or more app instances that run simultaneously try to upload file with the same name to the same 'UploadSet' .

I checked the Save method and the resolve_conflict method. Both are not protected agianst this scenario.

jugmac00 commented 2 years ago

Thanks for the report!

For the website I maintained I had one folder for each uploader, so I did not encounter this issue myself, nor was it reported before, but this certainly looks like a valid issue.

Given that this package exists for more than a decade, I do not think we need to rush a decision.

I think there are basically two ways to counter a race condition

I am happy to hear your and other users' opinions on this.

tabebqena commented 2 years ago

@jugmac00 As a theory, it is possible to have 2 flask applications that run as separate processes and execute this line or this line at the same time or before one of them can create the new file.

This is a low possibility because it is very rare to upload two files with the same name at the exact same time.

In my opinion, using a lock is a problematic approach. I prefer to add a suffix of random value to each filename. This should be optional, so users that pass a unique filename shouldn't complain.

jugmac00 commented 2 years ago

I am not overly happy with the random suffix, as you usually do not expect a file upload to be renamed.

On the other hand, using a lock would introduce some more complexity.

I'd like to put this issue on hold until we receive real bug reports or at least some more opinions on what to do.

tabebqena commented 2 years ago

I am not overly happy with the random suffix, as you usually do not expect a file upload to be renamed.

In our case the user should expect that any file could be renamed either for security reasons or beacuse it is clashing with another file.

If the user want to keep the original name, He can use the database. I don't remember whether I added example of this scenario or not in the update_Documentation pull request.

On the other hand, using a lock would introduce some more complexity.

:+1:

I'd like to put this issue on hold until we receive real bug reports or at least some more opinions on what to do.

:+1: