rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
524 stars 240 forks source link

infer_current_exe: Avoid picking non-path from maps #563

Closed sthibaul closed 11 months ago

sthibaul commented 11 months ago

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

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

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

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

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

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 https://github.com/rust-lang/backtrace-rs/pull/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 commented 11 months ago

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

It cannot be an absolute path

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

sthibaul commented 11 months ago

superseded by https://github.com/rust-lang/backtrace-rs/pull/567