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

Faulty Path/String assumptions #342

Closed
Ofenhed opened this issue Sep 28, 2021 · 3 comments
Closed

Faulty Path/String assumptions #342

Ofenhed opened this issue Sep 28, 2021 · 3 comments
Assignees

Comments

@Ofenhed
Copy link

Ofenhed commented Sep 28, 2021

I've noticed multiple bugs regarding data types in utils.rs. Since they are of the same type, I made it a single ticket.

First bug appends two paths with the format! macro instead of using Path::join.

Second bug is similar with a function which seems to try to perform Path::strip_prefix("/").

Third bug converts the executed program and its arguments to String, and gives the Path a different behavior than the arguments (where the Path drops invalid characters and the arguments are simply dropped if they contain invalid characters).

I could also find the format! issue from the first bug in rootfs.rs.

All of the examples above operate on Paths with String as an intermediate data type. String is always UTF-8 compatible, but Path is not. This means that assert_eq!(Path::new(format!("{}{}", a.display(), b.display())), a.join(b)); will fail for some paths. I have not had time to find where these functions are used, so I don't know how big of an issue this is, but it should be fixed before it becomes a serious issue. It should also be verified that CStrings/OsStrings never use Strings as an intermediate data type.

@Furisto
Copy link
Collaborator

Furisto commented Sep 28, 2021

Thanks for the report. Would you be interested in fixing it?

@Ofenhed
Copy link
Author

Ofenhed commented Sep 28, 2021

In the near future I'm afraid I wouldn't have time for that.

@Furisto Furisto self-assigned this Sep 28, 2021
@yihuaf
Copy link
Collaborator

yihuaf commented Sep 28, 2021

I can help fix this. I've noticed this before and have a feeling we have to revisit some of this code.

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