Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not suppress 'permission denied' warnings in FileUpload::move() #82

Closed

Conversation

@OndrejSlamecka
Copy link

OndrejSlamecka commented Mar 1, 2016

This commit adds more comprehensible error reporting to the move() method in the following scenarios:

  • Say the program has insufficient permissions to create the directory dirname($dest). With this commit the move() method correctly raises mkdir(): Permission denied warning instead of warning raised by move_uploaded_file: failed to open stream: No such file or directory with previous code.
  • Or the already existing file at $dest may not be deleted with the current permissions. With this commit a correct Permission denied warning is again raised by unlink instead of warning raised by move_uploaded_file with previous code.
This commit adds more comprehensible error reporting to the `move()` method in the following scenarios:

* Say the program has insufficient permissions to create the directory `dirname($dest)`. With this commit the `move()` method correctly raises `mkdir(): Permission denied` warning instead of warning raised by `move_uploaded_file`: `failed to open stream: No such file or directory` with previous code.
* Or the already existing file at `$dest` may not be deleted with the current permissions. With this commit a correct `Permission denied` warning is again raised by `unlink` instead of warning raised by `move_uploaded_file` with previous code.
@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Mar 1, 2016

You solution suffers with race condition problem.

@enumag

This comment has been minimized.

Copy link
Contributor

enumag commented Mar 1, 2016

Then how about keeping the original code and adding if (file_exists($dest)) { throw new ... }?

@OndrejSlamecka

This comment has been minimized.

Copy link
Author

OndrejSlamecka commented Mar 1, 2016

Good point and good suggestion, thank you. The commit above should prevent the problem at least to the level the previous code prevented such problems.

Is InvalidStateException the right one to be thrown?

@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Mar 1, 2016

Are you sure, that only reason why directory has not been created is the permission problem? :)

@OndrejSlamecka

This comment has been minimized.

Copy link
Author

OndrejSlamecka commented Mar 1, 2016

Right, inode limit comes to mind. I made the text more general with the commit above.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Mar 1, 2016

@dg dg force-pushed the nette:master branch from 51568f8 to 2381182 Apr 11, 2016
@dg dg force-pushed the nette:master branch from 19c6fa9 to 28af524 Apr 21, 2016
@dg dg force-pushed the nette:master branch from 6286f91 to d523f35 May 9, 2016
@dg dg force-pushed the nette:master branch from 67a7020 to 3fd485a Jun 17, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented Jun 17, 2016

What about call mkdir only when dir doesn't exist and remove @?

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Jun 17, 2016

That wouldn't be atomic though.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jun 17, 2016

This issue is about suppressing, isn't it?

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Jun 17, 2016

I replied to your previous comment:

call mkdir only when dir doesn't exist

@dg

This comment has been minimized.

Copy link
Member

dg commented Jun 25, 2016

I understand.

@dg dg closed this in 659e277 Jun 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.