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

libbacktrace bundled twice, one copy is missing bugfixes #43449

Closed
infinity0 opened this issue Jul 24, 2017 · 3 comments
Closed

libbacktrace bundled twice, one copy is missing bugfixes #43449

infinity0 opened this issue Jul 24, 2017 · 3 comments
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@infinity0
Copy link
Contributor

From the 1.19 tarball:

$ diff -ruw src/vendor/backtrace-sys/src/libbacktrace/ src/libbacktrace/ | wc -l
553

Includes this:

diff -ru -ruw src/vendor/backtrace-sys/src/libbacktrace/pecoff.c src/libbacktrace/pecoff.c
--- src/vendor/backtrace-sys/src/libbacktrace/pecoff.c	2017-07-24 10:44:52.283085695 +0200
+++ src/libbacktrace/pecoff.c	2017-04-19 22:41:07.723729879 +0200
@@ -602,9 +602,14 @@
   const b_coff_section_header *sects;
   struct backtrace_view str_view;
   int str_view_valid;
-  size_t str_size;
+  // NOTE: upstream this is a `size_t` but this was fixed in Rust commit
+  //       55e2b7e1b, see #33729 for more info. If you see this in a diff
+  //       against the upstream libbacktrace, that's what's going on.
+  uint32_t str_size;
   off_t str_off;
-  struct backtrace_view syms_view;
+  // NOTE: upstream doesn't have `{0}`, this is a fix for Rust issue #39468.
+  //       If syms_view is not initialized, then `free(syms_view.base)` may segfault later.
+  struct backtrace_view syms_view = {0};
   off_t syms_off;
   size_t syms_size;
   int syms_view_valid;

src/libbacktrace is only mentioned explicitly by rustbuild once in src/bootstrap/dist.rs to build a source tarball, so I guess it's not used at all. That means the bugfixes are actually missing from rustc entirely.

More generally, I think it's worth auditing source tarballs to avoid duplicating C code.

@infinity0
Copy link
Contributor Author

Also it would be worth good to push these fixes upstream, from the comment it seems like it's not Rust specific. The upstream code: https://github.com/ianlancetaylor/libbacktrace/blob/master/pecoff.c#L607

@infinity0
Copy link
Contributor Author

From the lock file: https://github.com/rust-lang/rust/blob/master/src/Cargo.lock#L431 It looks like the unfixed version is only used to build cargo and the rust installer (crates-io and error-chain are also part of the dependency tree) so rustc itself is not affected.

The backtrace-rs version, which is unfixed, is actually newer. It contains some ./configure updates as well as additional DWARF2 definitions. There is yet an even newer copy in GCC ((C) 2017) which also doesn't have the Rust fixes: https://github.com/gcc-mirror/gcc/blob/master/libbacktrace/pecoff.c#L607

Since rustc itself is not affected I'll just ignore this second copy for now. However since libstd uses libbacktrace, it would be worth having backtrace-rs use this copy rather than a second one, to avoid this sort of thing in the future.

@Mark-Simulacrum Mark-Simulacrum added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jul 26, 2017
@Mark-Simulacrum
Copy link
Member

We use backtrace from crates.io today I believe so this should mostly be fixed, I hope, though if not bugs would be filed against https://github.com/alexcrichton/backtrace-rs/ anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants