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

fix ish-app/ish#2349 stop EACCES truncate with open #2352

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kuflierl
Copy link

Relocate the permission check for the generic_openat function to before opening the back-end.
This mitigates the case where open is called on the file back-end even if the user is not allowed to.
Opening the back-end prematurely can cause truncation of the file in question by the error handler.

This seemed like an easy issue to fix so i tried fixing it.
This is my first PR in this Repository, If i made any mistakes let me know!

@tbodt
Copy link
Member

tbodt commented Mar 7, 2024

Looks like this makes tests fail

@kuflierl
Copy link
Author

kuflierl commented Mar 7, 2024

That's indeed worrying, I will look into it when I have time

@kuflierl
Copy link
Author

kuflierl commented Mar 22, 2024

Looks like this makes tests fail

This time it should work @tbodt I ran the tests.

I had to engage the inode lock before the stat

Sorry for the delay tho

@kuflierl
Copy link
Author

kuflierl commented Apr 8, 2024

We may need to increase the timeout delay to reduce false positives

@kuflierl
Copy link
Author

Actually there might be a better way that doesn't cripple the speed. but it might include a slight rework of the fd closing function. @tbodt Would you prefer that?

@tbodt
Copy link
Member

tbodt commented Apr 17, 2024

What happened with the speed?

If more things need to be refactored, more things need to be refactored :D This isn't a bad thing in itself.

@kuflierl
Copy link
Author

What happened with the speed?

If more things need to be refactored, more things need to be refactored :D This isn't a bad thing in itself.

It seems fstat is that much faster than stat. I expected a bit of performance loss due to extra path finding overhead but not this much. Instead of checking before opening the file it might be a better idea to just write a better error handling function to compensate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants