search

ish-app / ish

Linux shell for iOS
https://ish.app
Other
16.33k stars 855 forks source link

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

Open kuflierl opened 4 months ago

kuflierl commented 4 months ago

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 commented 3 months ago

Looks like this makes tests fail

kuflierl commented 3 months ago

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

kuflierl commented 3 months ago

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 commented 2 months ago

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

kuflierl commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.