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

backtrace-sys creates invalid symbol name pointer #267

Closed
MeFisto94 opened this issue Dec 10, 2019 · 11 comments
Closed

backtrace-sys creates invalid symbol name pointer #267

MeFisto94 opened this issue Dec 10, 2019 · 11 comments

Comments

@MeFisto94
Copy link

MeFisto94 commented Dec 10, 2019

Apologizes for the imprecise issue title, but I'm not 100% sure what happens and where that happens.
This issue has been arised for a few people when using Servo on Linux, see servo/servo#24984

I've done quite some investigation and the conclusion is:
The caller of that frame (second last) is faulty:
libbacktrace/elf.c:elf_syminfo is invoking the callback in symbolize/libbacktrace.rs's syminfo_cb.
Program Counter (PC): 0x000055555b12f07c
_symval: 0x000055555b12ec80
_symsize: 0x568
symname: <read memory from 0x7ffecbd3cbb2 failed (0 of 1 bytes read)>
So here symname is a pointer onto an invalid memory region.

The symbol table is generated from within backtrace_syminfo which calls fileline_initialize, thus this might happen from within upstream or this crate somehow accidentially messes up the pointer (e.g. freeing it).

This is the point where my knowledge ends and maybe you guys can shed some light on it.
I've upgraded servo's backtrace to 0.3.40 as well, but the result stayed the same.

Thanks in Advance for any help :)

Edit:

  1. I had to downgrade to 0.3.38 again, because after that, the debug symbols got lost for lldb (or better put: the source code view/link to ~/.cargo/registry/src/github.com-hash/backtrace-sys/src/libbacktrace

  2. https://github.com/rust-lang-nursery/libbacktrace/blob/f4d02bbdbf8a2c5a31f0801dfef597a86caad9e3/elf.c#L684 this is the "problem", as far as I can tell: here the entries of the symbol table are copied, which here means: the struct https://github.com/rust-lang-nursery/libbacktrace/blob/f4d02bbdbf8a2c5a31f0801dfef597a86caad9e3/elf.c#L405 and thus https://github.com/rust-lang-nursery/libbacktrace/blob/f4d02bbdbf8a2c5a31f0801dfef597a86caad9e3/elf.c#L393

  3. Judging by https://github.com/rust-lang-nursery/libbacktrace/blob/f4d02bbdbf8a2c5a31f0801dfef597a86caad9e3/elf.c#L2965, sdata is meant to stay, so 2 is intentionally only a shallow copy

  4. Unfortunately https://github.com/rust-lang-nursery/libbacktrace/blob/f4d02bbdbf8a2c5a31f0801dfef597a86caad9e3/elf.c#L3071 fails, so goto fail, which releases the strtab table (https://github.com/rust-lang-nursery/libbacktrace/blob/f4d02bbdbf8a2c5a31f0801dfef597a86caad9e3/elf.c#L3199), which was never intended to be released under regular conditions.

  5. Problem? https://github.com/rust-lang-nursery/libbacktrace/blob/f4d02bbdbf8a2c5a31f0801dfef597a86caad9e3/elf.c#L644 is also only a shallow copy.
    To me this looks like a clear bug upstream in libbacktrace, where either this failure (4) should be critical, in which case state shouldn't contain any syminfo_data at all, or the failure could happen, where the strings should be copied / symtab shouldn't be released ["We hold on to the string table permanently"].

Furthermore the caller https://github.com/rust-lang-nursery/libbacktrace/blob/f4d02bbdbf8a2c5a31f0801dfef597a86caad9e3/elf.c#L3234 doesn't back-propagate the failure (missing else branch), which would cause dl_iterate_phdr to fail and this should make backtrace_initialize fail, so this rust bindings can also react to the fault.

Since all of this isn't happening, I assume this is considered a small error and thus everything contains to read the next headers, tables, the next shared libraries, whatever, continue operating as normal. Then it's just about never releaseing symtab, not even in the case of fault, because still something is returned and thus it's considered "working".

@MeFisto94
Copy link
Author

Okay, I did further research, as you can see, and I wanted to point out ianlancetaylor/libbacktrace#24 (comment)

I'll going to report my findings upstream to gcc, depending on the situation you might want to get a more up to date source of libbacktrace, now that it lives in gcc.

@alexcrichton
Copy link
Member

Thanks for the detailed report and investigation! Definitely happy to include updates to the library as necessary.

One thing you may also want to try out is to enable the gimli-symbolize feature which may fix the issues for you as well?

@MeFisto94
Copy link
Author

I've actually forked this library for a quick and dirty solution until I hear back from GNU.

Switching to gimli-symbolize will probably fix these issues as well, but is quite an intrusive change, which would require more review and approval than these two lines of comments.

For reference: https://github.com/MeFisto94/backtrace-rs/tree/fix-strtab-freeing-crash or rather https://github.com/MeFisto94/libbacktrace/tree/fix-strtab-freeing-crash

@MeFisto94
Copy link
Author

FYI: in ianlancetaylor/libbacktrace#29 there is a patch, so keep an eye out on feature upgrades to libbacktrace. Even if #289 would hide the error, an upgrade is necessary on systems that do not support memory mapped IO.

@MeFisto94
Copy link
Author

So there are two new commits and issue fixes upstream, which could be included, at least after #289 is merged.

That would be a good candidate for a new version

@MeFisto94
Copy link
Author

MeFisto94 commented Mar 4, 2020

@alexcrichton Since #289 is now merged and ianlancetaylor/libbacktrace#29 is also fixed, how about bumping the crate's version number?

Edit: And of course updating backtrace-sys to the latest upstream version

@alexcrichton
Copy link
Member

Sure thing, done!

@MeFisto94
Copy link
Author

Actually the git submodule of backtrace-sys still points to https://github.com/rust-lang-nursery/libbacktrace/tree/f4d02bbdbf8a2c5a31f0801dfef597a86caad9e3, so I guess that repository needs to pull the upstream changes first and then the submodule needs to be updated as well.

Sorry for the trouble!

@alexcrichton
Copy link
Member

Yes the submodule update is being attempted in #299, but there are still issues to work through.

@MeFisto94
Copy link
Author

Oh! I didn't see that, I thought it's just a simple git pull and that it'd have been forgotten, sorry!

@MeFisto94
Copy link
Author

so now I guess this is closed in #299

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

2 participants