-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Make backtraces work on Windows GNU targets again. #39234
Make backtraces work on Windows GNU targets again. #39234
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
ba38be8
to
b5dfe50
Compare
@segevfiner
👍 |
|
||
let mut filename = ptr::null(); | ||
let mut file = None; | ||
if let Ok(value) = ::sys::backtrace::get_executable_filename() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Ok(value)
-> Ok((filename, file))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit 2: you can do let (filename, file) = match ::sys::backtrace::get_executable_filename() { .... }
let mut filename = ptr::null(); | ||
let mut file = None; | ||
if let Ok(value) = ::sys::backtrace::get_executable_filename() { | ||
FILENAME_BUF = &value.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static FILENAME_BUF
1) contains a dangling reference to stack-allocated Vec<*i8>
and 2) is not necessary
@@ -135,7 +151,7 @@ pub fn print(w: &mut Write, idx: isize, addr: *mut libc::c_void, | |||
|
|||
// backtrace errors are currently swept under the rug, only I/O | |||
// errors are reported | |||
let state = unsafe { init_state() }; | |||
let (state, _file) = unsafe { init_state() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_file
needs to be unlocked (dropped) only if we got success from backtrace_syminfo
.
Otherwise libbacktrace will try to read it again in next backtrace requests in unlocked state.
On failure the file needs to be either leaked or stored to be unlocked later.
|
||
buf.set_len(size as usize); | ||
|
||
Ok(buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to validate that the reverse conversion with MultiByteToWideChar
gives the original u16
string, otherwise the conversion was lossy and libbacktrace will try to open some other file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the lpUsedDefaultChar
parameter of WideCharToMultiByte
, it can save the need to convert twice assuming that it's not broken in some way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there's still ways in which data loss can occur other than default char replacement. You'll want to pass WC_NO_BEST_FIT_CHARS
.
For strings that require validation, such as file, resource, and user names, the application should always use the WC_NO_BEST_FIT_CHARS flag. This flag prevents the function from mapping characters to characters that appear similar but have very different semantics. In some cases, the semantic change can be extreme. For example, the symbol for "∞" (infinity) maps to 8 (eight) in some code pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally the set of flags should mirror whatever is used inside of (most probably) CreateFileA
which is used inside posix open
in CRT which is used in libbacktrace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function which is actually used to convert strings inside A
functions is RtlAnsiStringToUnicodeString
which in turn calls RtlMultiByteToUnicodeN
, not MultiByteToWideChar
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also another point is that we're going in the opposite direction here, so we can't match the flags anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if we do want to "fix" this, then this will work 😄
let image_name1 = query_full_process_image_name()?;
let ansi_image_name1 = WideCharToMultiByte(image_name1, TRY_TO_GUESS_FLAGS);
let file = CreateFileA(ansi_image_name1)?; // instead of OpenOptions::new().blah().open(&image_name1)
let image_name2 = query_full_process_image_name()?;
if image_name1 != image_name2 {
// Simultaneously make sure that the file is not moved and that encoding conversion was correct
return Err
}
return ansi_image_name1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSDN documents that WC_NO_BEST_FIT_CHARS
should actually make the conversion reversible, which is what you wanted (assuming no surprises). So I added it as suggested.
|
||
fn query_full_process_image_name() -> io::Result<PathBuf> { | ||
super::fill_utf16_buf(|buf, mut sz| unsafe { | ||
c::QueryFullProcessImageNameW(c::GetCurrentProcess(), 0, buf, &mut sz); sz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, GetCurrentProcessId())
instead of c::GetCurrentProcess()
like the #37359 does?
QueryFullProcessImageNameW
didn't work with pseudo-handle -1
returned by GetCurrentProcess
the last time I tried.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there's no need to convert this Vec<u16>
vector into PathBuf
and back, you can just use Vec<u16>
through all the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a bug they fixed in Windows 8/10 or something. Windows... 😛
I converted it to a PathBuf
for the call to OpenOptions::open
below, in a way it's a function that queries for a path so it makes sense that it returns a Path[Buf]
.
Ok((filename, file)) => { | ||
// filename is purposely leaked here since libbacktrace requires | ||
// it to stay allocated permanently | ||
((*Box::into_raw(Box::new(filename))).as_ptr(), Some(file)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let filename_ptr = filename.as_ptr();
mem::forget(filename);
(filename_ptr, Some(file))
is probably the simplest solution, but this is ok too.
Now I think I'm satisfied with this PR. |
c::FALSE, | ||
c::GetCurrentProcessId())); | ||
super::fill_utf16_buf(|buf, mut sz| { | ||
c::QueryFullProcessImageNameW(process_handle.raw(), 0, buf, &mut sz); sz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check for errors here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading fill_utf16_buf
carefully and seeing that QueryFullProcessImageNameW
will leave sz
as is on error. You are right.
pub fn get_executable_filename() -> io::Result<(Vec<i8>, File)> { | ||
let (executable, file) = lock_and_get_executable_filename()?; | ||
let u16_executable = super::to_u16s(executable.into_os_string())?; | ||
Ok((super::wide_char_to_multi_byte(c::CP_ACP, c::WC_NO_BEST_FIT_CHARS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, how much does this buy us over just attempting utf-8 and returning an error if not?
We don't use these standard functions anywhere else in the standard library, so I'm curious if we'd get much benefit from trying to do this here now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because your UTF-8 bytes might be valid in some other encoding, but have different meaning, which would result in accessing the wrong file, which could potentially leave a vector open for attack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well...
UTF-8 will just break when passed to open
/backtrace_create_state
on any non ASCII character while the WideCharToMultiByte(CP_ACP, ...)
will work on any character that is supported in the current ANSI code page.
The ANSI code page is a setting that you can set in Windows which is used by any non Unicode aware (wide char) program, as such it's often set to a code page for the user's local language allowing such programs to work with it. Sadly Windows doesn't use UTF-8 like Linux and keen.
As such using the ANSI code page is very likely to keep this working despite non ASCII characters in the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use these standard functions anywhere else in the standard library
WeThey use them in libbacktrace 🙂
And we have to do the same.
ptr::null_mut()); | ||
STATE | ||
(STATE, file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come file
is returned here? We're going to unconditionally reuse STATE
so long as it's non-null, so it seems like so long as we do that we should keep the file open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need to keep the file open/locked during the first call to a libbacktrace function that operates on the state unless that function fails and as such it will try the initialization again on the next call.
What is currently done is to optionally return the file on the first creation of the state. And if the first call to libbacktrace_syminfo
fails we leak it so it stays open, otherwise it will be dropped on exit from print
.
The options I can think of:
- Refactor the code so it's clearer, I'm open to any suggestions here.
- Leak it always.
Just mem::forget
immediately. - Return it and keep it in a
static mut
(Annoying since it's a drop type so we need to play around withBox
and raw pointers), and drop it explicitly oncebacktrace_syminfo
succeeds the first time. - The same as 3 but place it in a
static mut
immediately ininit_state
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, thanks for the explanation! I think it's probably easiest/best to just leak it here rather than special casing the result later on (depending on internal impl details of libbacktrace). We can probably use .into_raw_handle()
here as well instead of mem::forget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is sys_common
. Which means I can't use .into_raw_handle()
since it only exists on Windows (It's .into_raw_fd()
in Unix/Linux) and cfg!
/#[cfg]
is not allowed in sys_common
(Enforced by tidy). 😕
Looks good to me, thanks @segevfiner! Want to squash the commits and I'll r+? |
This is done by adding a function that can return a filename to pass to backtrace_create_state. The filename is obtained in a safe way by first getting the filename, locking the file so it can't be moved, and then getting the filename again and making sure it's the same. See: rust-lang#37359 (comment) Issue: rust-lang#33985
ef95321
to
4186037
Compare
Squashed! Thanks for looking at this too! Now I guess it's time to cross fingers for CI. 😛 |
@bors: r+ |
📌 Commit 4186037 has been approved by |
⌛ Testing commit 4186037 with merge c588767... |
💔 Test failed - status-travis |
|
@petrochenkov Yeah, I noticed. Working on it. |
I tried to eliminate the dead code lints: (New commit so you can see what I did, instead of immediately squashing):
I tested x86_64-pc-windows-gnu and x86_64-pc-windows-msvc. My computer is slow and lacking disk space to test Linux and some non-GNU Unix platform right now. Hopefully the fix there is trivial enough that I got it right this time 😞 |
@bors: r+ Thanks! |
Added a commit which fixes this. I can squash to one commit if you want. P.S. C also allows for even further evil. It requires |
@bors: r+ |
📌 Commit 1b4a6c8 has been approved by |
⌛ Testing commit 1b4a6c8 with merge d7c78aa... |
@bors: retry
|
@bors: r- I think this may have been the cause of these failures in the rollup |
@alexcrichton It does look like backtraces are screwed up somehow in i686-pc-windows-gnu... 😖 The symbols are wrong or the stack walk isn't working, it might be a bug elsewhere that wasn't detected until now since backtraces were broken for quite some time and no one tried gdb in this target. I will try to compile and check... (So much platform specific fun with this pull request 😢 ) |
And the results are... Build failure: #39357 Obviously I can't try to debug when I can't even build... |
I managed to build using the gcc used in the AppVeyor build and even something as simple as this: fn f() {
panic!("Boom!");
}
fn main() {
f();
} Doesn't display a proper stack trace in this build. The tests show weird frames with only the function drop while this minimal example seems to punt once it tries to climb one frame back. |
@segevfiner |
|
@petrochenkov I should have looked first... #28218 😆 I think I managed to fix this by simply adding But this completely disables this optimization. MSVC disables frame pointer optimizations by default nowadays, "/Oy-" is added by some arcane msbuild stuff. So I wonder if this is not something that is acceptable to disable so that backtraces work? So I guess we need a decision:
|
@segevfiner |
(I did some measurements with enabled/disabled frame pointers on couple of projects with MSVC+PDB toolchain, but it was years ago. There were small performance differences, but I still never disabled frame pointers even in release builds because it resulted in absolutely horrible debugging experience. |
I think we should disable frame pointer optimization on |
@bors r+ |
📌 Commit ab21314 has been approved by |
The follow up commit which disables FPO and restores the test is at segevfiner@befe380 which I will rebase and open a PR for, once the CI will stop finding surprises, so that this can be discussed and tested properly. |
…trochenkov Make backtraces work on Windows GNU targets again. This is done by adding a function that can return a filename to pass to backtrace_create_state. The filename is obtained in a safe way by first getting the filename, locking the file so it can't be moved, and then getting the filename again and making sure it's the same. See: #37359 (comment) Issue: #33985 Note though that this isn't that pretty... I had to implement a `WideCharToMultiByte` wrapper function to convert to the ANSI code page. This will work better than only allowing ASCII provided that the ANSI code page is set to the user's local language, which is often the case. Also, please make sure that I didn't break the Unix build.
☀️ Test successful - status-appveyor, status-travis |
This is done by adding a function that can return a filename
to pass to backtrace_create_state. The filename is obtained in
a safe way by first getting the filename, locking the file so it can't
be moved, and then getting the filename again and making sure it's the same.
See: #37359 (comment)
Issue: #33985
Note though that this isn't that pretty...
I had to implement a
WideCharToMultiByte
wrapper function to convert to the ANSI code page. This will work better than only allowing ASCII provided that the ANSI code page is set to the user's local language, which is often the case.Also, please make sure that I didn't break the Unix build.