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

Handle binary for shebangs #9

Closed
wants to merge 1 commit into from
Closed

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Jun 22, 2019

Addresses #8.

The code here is really not fun, but that's because dealing with binary strings in rust is really not fun.
Note that this introduces a dependency on BString so I didn't have to deal with turning a Vec<u8> into OsString which appears to be non-trivial.
Note that this will crash on windows for binary files, not sure if the project plans to run on windows.

The following tests now pass because of this pull:

  • test kernel::execve::shebang::tests::test_extract_shebang_not_script
  • test kernel::execve::shebang::tests::test_expand_shebang_not_script

@jyn514
Copy link
Contributor Author

jyn514 commented Jun 22, 2019

Note: the reason dealing with binary in the first place is important is for files that aren't scripts, e.g. /bin/sleep. If we're willing to disallow non-valid UTF8 once we've seen a #!, it would make this code a lot nicer and handle the vast majority of use cases.

@oxr463
Copy link
Contributor

oxr463 commented Jun 22, 2019

PRoot, the original c implimentation, is Linux-only; it is written against Linux-specific libraries, etc. That being said, this rust version doesn't seem to be a direct translation of the other program. Therefore, I would try to rewrite the code to be as portable as possible.

Can you elaborate more on disallowing non-valid UTF-8? Are there examples for why we wouldn't want this?

@jyn514
Copy link
Contributor Author

jyn514 commented Jun 23, 2019

I'm not completely sure of this myself but I'll give it my best shot. On Linux (and I think most Unix platforms), paths can be arbitrary bytes, if you want an executable called /bin/\x253, you can have it. On Windows, paths are normally UTF-16, but can be arbitrary 16-bit integers (i.e. you can have invalid surrogate pairs in filenames). There's not a way to convert between the two, and you can't use the normal String class because it only allows valid UTF-8.

If we disallow that, then there will be paths we can't exec. However, in the vast majority of cases, this will be just a nuisance to code for because it's not in the standard library - see how getting and splitting the first line is about 10 lines of code where normally it would be BufReader.next_line().split. It's also the reason this commit introduces a new dependency on bstr.

All of this is only an issue for binary file paths, it's not that hard to check if the first two bytes of a file are #! even if the whole thing is a binary ELF file. Additionally, non-binary file paths will currently work on either Linux or Windows.

More info:

@jyn514
Copy link
Contributor Author

jyn514 commented Jun 23, 2019

You mentioned this is a rewrite and should probably be portable - in that case we should get rid of the ASM in src/kernel/execve/loader

@oxr463
Copy link
Contributor

oxr463 commented Jun 23, 2019

You mentioned this is a rewrite and should probably be portable - in that case we should get rid of the ASM in src/kernel/execve/loader

I'm surprised that the loader was brought over from PRoot. I'm not sure how it should be re-implimented here, but I guess that can go into a separate issue.

@oxr463
Copy link
Contributor

oxr463 commented Jul 9, 2019

What about this, libpathrs?

@jyn514
Copy link
Contributor Author

jyn514 commented Jul 9, 2019

libpathrs looks like it addresses safe path handling. That could be useful as part of the main package for proot, but it's not directly related to parsing the path.

@oxr463
Copy link
Contributor

oxr463 commented Mar 25, 2021

Closing in favor of #14

@oxr463 oxr463 closed this Mar 25, 2021
@oxr463 oxr463 added this to the v0.0.1 milestone Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants