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

infer_current_exe: Avoid picking non-path from maps #563

Closed
wants to merge 1 commit into from

Conversation

sthibaul
Copy link
Contributor

@sthibaul sthibaul commented Sep 3, 2023

Some ports are not showing paths in /proc/self/maps yet, so better make sure that we actually get a path before returning it, and fallback to current_exe() otherwise.

Some ports are not showing paths in /proc/self/maps yet, so better make
sure that we actually get a path before returning it, and fallback to
current_exe() otherwise.
@workingjubilee
Copy link
Member

I think it may be more appropriate to simply introduce the appropriate cfg? I don't feel like "starts with /" is an ironclad way to reason about "this is a path".

@sthibaul
Copy link
Contributor Author

sthibaul commented Sep 4, 2023

I think it may be more appropriate to simply introduce the appropriate cfg? I don't feel like "starts with /" is an ironclad way to reason about "this is a path".

Well, without a heading '/', it cannot be a path...

@sthibaul
Copy link
Contributor Author

sthibaul commented Sep 4, 2023

I think it may be more appropriate to simply introduce the appropriate cfg? I don't feel like "starts with /" is an ironclad way to reason about "this is a path".

Well, without a heading '/', it cannot be a path...

Not saying it's making it a path, but that seems a reasonable check-up.

@sthibaul
Copy link
Contributor Author

sthibaul commented Sep 4, 2023

Also, making it a cfg means that whenever a port happens to implement showing a path, they'll not only have to change the cfg to benefit from it, but also have to take care that the new backtrace source might be running on an old system that doesn't show the path (and thus get the breakage again...)

@workingjubilee
Copy link
Member

Well, without a heading '/', it cannot be a path...

It cannot be an absolute path. I don't like bets like "this code should be portable for different implementers of /proc/self/maps but despite one of the implementers of /proc/self/maps making the decision that they will not show paths, definitely none of them also deviate by allowing non-absolute or otherwise non-standard paths that are nonetheless valid paths on that OS". It feels like trading one assumption for another.

Also, making it a cfg means that whenever a port happens to implement showing a path, they'll not only have to change the cfg to benefit from it, but also have to take care that the new backtrace source might be running on an old system that doesn't show the path (and thus get the breakage again...)

There is a reason that Rust targets have a Linux kernel minimum and libc minimum and BSD OS minimums and other such things for various OS, yes, and there is also a reason that the parse function will have to have its internals entirely changed in #507

And in the future this code will likely be maintained by someone who has internalized the assumptions of some other operating system and they will apply their own reasoning, and they may unintentionally break this because a guard inserted for a target-specific reason was inserted without any indication of why in the source. And then people may or may not decide to inspect the git history. A cfg makes it clear why the difference.

@workingjubilee
Copy link
Member

workingjubilee commented Sep 9, 2023

I realize this may seem like pedantic rambling, I just don't want to get multiple successive PRs from Linux (which I think was the first implementer of /proc/self/maps?), Hurd (which deviates from Linux's implementation insofar as we're concerned here), and Redox (where Everything Is A URL and presumably so would the paths in their possible future impl of... proc:/self/maps I guess?) supporters debating what the "portable" code should look like to best serve these different systems. All of them make the intriguing claim that they are indeed a "Unix system", whatever that means.

Rust has gotten similar requests from people that want to claim their niche variant OS is a Linux target while heavily subsetting the Linux kernel UAPI (and thus libc implementation) we usually demand of Linux, and I gave a similar answer then: the diff may be small, but sometimes a small diff still matters for either performance or maintainability, so I prefer the cfg.

@sthibaul
Copy link
Contributor Author

sthibaul commented Sep 9, 2023

It cannot be an absolute path

If it wasn't an absolute path, it would be useless.

@sthibaul
Copy link
Contributor Author

superseded by #567

@sthibaul sthibaul closed this Sep 21, 2023
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.

2 participants