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

SuFile.isDirectory() can throw NoShellException (and shouldn't) #131

Closed
Fox2Code opened this issue Sep 17, 2022 · 4 comments
Closed

SuFile.isDirectory() can throw NoShellException (and shouldn't) #131

Fox2Code opened this issue Sep 17, 2022 · 4 comments

Comments

@Fox2Code
Copy link
Contributor

Due to SuFile.isDirectory() call SuFile.cmdBool() call SuFile.getShell() call Shell.getShell() call MainShell.get() call BuilderImpl.build() that can throw NoShellException and can causes crashes as it is a common assumption that File.isDirectory() does not throw any errors.

This is very obscure effect was discovered by someone using my app that use libsu.

@vvb2060
Copy link
Contributor

vvb2060 commented Sep 18, 2022

I think this is expected behavior and it is the developer's responsibility to check that the shell is valid before using it.

@Fox2Code
Copy link
Contributor Author

I personally disagree that isDirectory() should throw NoShellException because SuFile extends File, when this issue crashed my app on someone device, I had no idea and it was a existing behavior for me, so this was very unexpected for me.

I already did my best do avoid calling SuFile when root does not exists or is not available, and this "behavior" was really unexpected, so I think it's definitely a flaw of the library because the goal of a library is to help developers make programs.

But this is mainly a mater of opinion, so I understand the disagreement.

@topjohnwu
Copy link
Owner

Well, SuFile is backed by a shell; if you have no shell available, how does it operate? If you are really concerned about this, you should move over to use the nio module.

@Fox2Code
Copy link
Contributor Author

Well, I reported this because SuFile crashed despite my app checking for root access.

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

No branches or pull requests

3 participants