Bug fix: correctly handle spaces in /proc/pid/maps pathnames

[MasonRemaley]

Prior to this change, pathnames were parsed from /proc/pid/maps using s.split(' '). This is incorrect, because pathnames may contain spaces.

In practice, on the relevant operating systems, this resulted in truncation of pathnames containing spaces. This lead to a failure resolve any symbols or offsets, which in turn causes the stack trace to be printed containing only <unknown>s to the end user.

This PR corrects the parsing code, resolving the issue.

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518896B
Updated binary size: 524944B
Difference: +6048B (1.17%)

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518896B
Updated binary size: 524944B
Difference: +6048B (1.17%)

[MasonRemaley]

Side note--I tried to make the least invasive change possible since I'm not familiar with the codebase. That being said, is parsing /proc/pid/maps actually necessary?

If I'm reading the code correctly it's parsing the whole thing just to get the path to the main program. I would think there are simpler ways to get that info, but it's possible I'm missing something!

[EDIT] Okay yeah--you could open /proc/self/exe directly, and then you wouldn't need to parse /proc/pid/maps anymore. I don't have time to make that change right now though, this PR is still an improvement to the status quo either way!

[bjorn3]

/proc/self/exe doesn't help with dynamically linked libraries.

Edit: Looks like you are right. It seems to only be used for the current executable and not shared libraries.

[philipc]

There's an edge case where explicitly running ld.so gives the wrong path; see #488

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518896B
Updated binary size: 524944B
Difference: +6048B (1.17%)

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518896B
Updated binary size: 524944B
Difference: +6048B (1.17%)

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518896B
Updated binary size: 520312B
Difference: +1416B (0.27%)

[MasonRemaley]

There's an edge case where explicitly running ld.so gives the wrong path; see #488

Wasn't aware of this, thanks for the link!

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518896B
Updated binary size: 520216B
Difference: +1320B (0.25%)

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518904B
Updated binary size: 520208B
Difference: +1304B (0.25%)

[klensy]

Looking at https://github.com/torvalds/linux/blob/18b44bc5a67275641fb26f2c54ba7eef80ac5950/fs/proc/task_mmu.c#L254 i think that multiple spaces isn't possible anywhere except around pathname?

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 519000B
Updated binary size: 519960B
Difference: +960B (0.18%)

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 519000B
Updated binary size: 520024B
Difference: +1024B (0.20%)

[MasonRemaley]

Looking at https://github.com/torvalds/linux/blob/18b44bc5a67275641fb26f2c54ba7eef80ac5950/fs/proc/task_mmu.c#L254 i think that multiple spaces isn't possible anywhere except around pathname?

I'm being intentionally forgiving--

If my game crashes most players will just refund it, but assuming they consent I'll at least get a stack trace so I can fix it for the next person. If some day Linux decides to start aligning other elements by column too, I don't want to end up with a nightmare where I have to drop everything to figure out why my stack traces are failing before missing too much debug info.

[EDIT] Clarifying to make this PR easier to review--backtrace-rs does fail today on Linux on any executable stored at a path containing a space, this PR fixes that. The above discussion is just about how forgiving the parser should be, it's not questioning whether this fix is necessary.

[workingjubilee]

Oh hey, thank you so much!

1. Yes, as discussed, it unfortunately is necessary to parse proc/${pid}/maps due to the "fun" edge-cases if we want this backtrace to be consistently correct. It's acceptable to throw away more data than we currently throw away and adjust strictness, but not to avoid parsing, as avoiding parsing opens up far more problems than it solves. Case in point: the parse wasn't even right.
2. Unless we find some other way around the edge-cases (and I'm not confident ld.so is the only one!). I won't ask you to waste your time on that, however.
3. Yes, I agree that we don't need to require strict consistency in spacing with the existing format. There's no need to be too brittle, and even if the rendering of /proc/*/maps changes, libstd will be expected to have its backtrace work consistently across all kernel versions.

This PR looks great overall! I just see a few silly nits.

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518984B
Updated binary size: 520024B
Difference: +1040B (0.20%)

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518984B
Updated binary size: 520024B
Difference: +1040B (0.20%)

[github-actions]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518984B
Updated binary size: 520024B
Difference: +1040B (0.20%)

[MasonRemaley]

Oh hey, thank you so much!

You're welcome! :)

1. Yes, as discussed, it unfortunately is necessary to parse proc/${pid}/maps due to the "fun" edge-cases if we want this backtrace to be consistently correct. It's acceptable to throw away more data than we currently throw away and adjust strictness, but not to avoid parsing, as avoiding parsing opens up far more problems than it solves. Case in point: the parse wasn't even right.

2. Unless we find some other way around the edge-cases (and I'm not confident ld.so is the only one!). I won't ask you to waste your time on that, however.

That makes sense!

This PR looks great overall! I just see a few silly nits.

Thanks! Merged your suggestions--apologies for the typos!

[MasonRemaley]

Hey! Just checking in to see if you need any other changes for this to be mergeable. :)

[workingjubilee]

Mostly for my brain to get back from outer space!

[MasonRemaley]

Haha no worries, glad to get the fix in!