Stack overflow / buffer underrun in 3.4.0RC1 in debug mode (MSVC x86_64) #2221
Comments
asottile
changed the title
|
I'm debugging away at this, having a difficult time getting the link flags correct (I seem to be tickling some other unrelated compiler bug which is preventing me from properly linking) |
|
1 in the morning here, I'll look at this tomorrow. Sorry for the incomplete bug report, thought I'd be able to reuse my test harness from last time though I was unable to which has delayed my debugging significantly :( |
|
I've updated with my reproduction, I'm more confused than last time :S |
|
Stupidly this fixes it: $ git diff -w
diff --git a/src/sass_context.cpp b/src/sass_context.cpp
index df92021..dc6f353 100644
--- a/src/sass_context.cpp
+++ b/src/sass_context.cpp
@@ -125,7 +125,8 @@ extern "C" {
json_append_member(json_err, "message", json_mkstring(e.what()));
json_append_member(json_err, "formatted", json_mkstream(msg_stream));
try { c_ctx->error_json = json_stringify(json_err, " "); } catch(...) {}
- c_ctx->error_message = sass_copy_string(msg_stream.str());
+ std::string msg_stream_str = msg_stream.str();
+ c_ctx->error_message = sass_copy_string(msg_stream_str);
c_ctx->error_text = sass_copy_c_string(e.what());
c_ctx->error_status = 1;
c_ctx->error_file = sass_copy_c_string(e.pstate.path);Doesn't feel correct though, are we fighting inlining maybe? |
|
Well, it looked like a compiler bug to me from the begining. You might be right about the inlining though. Somewhat didn't think of it when I created the "sass_copy_string" function. Maybe you can change it somehow to not expose this issue (eg. |
|
I tried poking it some more and couldn't really get anything to work (tried |
|
I'm opting to disable |
|
Unless you guys are interested in supporting |
|
@am11 I'm not familiar with the use case for |
|
With Debug flag, compiler adds additional debug info to the generated program database (PDB) file (like source-map) containing all the symbols and their line/column infos etc. for source to source mapping (mach-code -> C[++] in this case). Some people only generate PDBs in Debug Configuration and disable it for Release Configuration. But mostly it is considered a good practice to have it generated for all configurations. If disabling fixes this issue, we can unset it in vcxproj file ( |
|
Thanks for your input @am11. If it's not actively causing harm I'd prefer to keep it as is. I'll leave this issue open as a reminder to look into the root cause. |
|
If the fix by @asottile works (#2221 (comment)) we should just do that. I (and I'm pretty sure @asottile too) have tried quite some time to understand why this is happening. I fail to see why this even should/could happen (as statet, it looks like a compiler bug to me). Maybe a MSVC guru could enlighten us, but so far none has shown up. @am11 IMHO debug mode does not only output pdb files, but also enables i.e. assertions in STL containers. Therefore I highly recommend to use release mode for actual releases, due to possible massive performance decrease by debug builds. |
|
@mgreter, I am all in for release-only builds. My point was merely this; if possible, we should make steps to repro and report the compiler bug upstream during the process of patching it the hacky whacky way. This way we will have a better/definite closure and some link to track this bug by. Generally speaking, sometimes during the process of separating out the steps to repro we rather find the bug in our code but this seems very unlikely in this case. :) |
|
@am11 I agree and actually like the debug assertion checks for testing, but unfortunately this seems to be quite a heisenbug. It's hard to trigger reliably and even harder to do so in a reduced test case. I myself am more familiar with GCC, so we pretty much lack the expertise here. Not sure if it is a compiler bug, just saying that my gut feeling sofar tells me that :) My educated guesses for this issue are based on that they tend to crop up when some error is catched and I've sofar (AFAIR) only saw it in relation to sass_context, which could indicate some issue with "extern C" and/or dll boundaries (I don't see how, but that at least would somewhat explain the strange behavior). But in the end this doesn't really make sense to me. I would expect a lot of other places where this should have cropped up. Anyway, we did similar fixes in the past. Although I was able to see the logic in the ones we've made before. So we should at least change the pattern in said cpp file to take an explicit stack variable for the string from a stringstream before passing it into a function. |
|
For what it's worth, my patch above worked in one part of the codebase but failed in another so it isn't really a solution. I wonder what the maximum codesize msvc bug reports can be, heh. We've turned off debug builds so this is much lower priority than it was when I created it (I didn't think that setuptools had enough control over distribution-set compile flags, I was wrong!). |
|
I have found the reason why this is happening. The issue is that somewhere in VCBlog or VS2015 release notes (RTM: https://www.visualstudio.com/en-us/news/releasenotes/vs2015-rtm-vs#visualc also see Update 1, 2 and/or update 3 in left-hand nav), there was a detail on breaking changes/improvement in switches they made which causes some non-msbuild build systems pain bit more to figure out, unless we adapt to the new recommendations. For example in your
Note that project system is not emitting I am running VS2015 Update 3. |
|
Forgot to mention, I also added: <ItemGroup>
<ClCompile Include="..\main.cpp" />
</ItemGroup>in |
|
@am11 👏 I have confirmed with 4 compiles (2 with Edit: Running VS2015 Update 3. |
|
Thanks @mgreter! Glad I could contribute. 🎉 Separately, speaking of C++, there is this 🆕 Ranges proposal for STL: https://github.com/ericniebler/range-v3, which might be useful (server as source of inspiration) for this project. One downside concerning LibSass is it doesn't support GCC < v4.8.5. :( |
|
I have probed some more compiler option variations: From the pattern above it seems that it has to do with function inlining ( The conclusion for me is that you should not optimize debug builds too much. This can be usefull if you i.e. want to do performance profiling for actual release builds. But for regular debug builds I normally disable optimization explicitly, as I want to be able to step through every function call and not have certain calls optimized out. |
|
Incredible work y'all |
Appears to be a regression of #2046
I'll try and get better details when I'm at a computer.
I'm at the 3.4.0RC1 tag: sass/libsass-python#166
Reproduction
(This time with proper file paths!):
test.bat
main.cpp
Commands to reproduce
Output
Where it breaks at
Puzzlingly, it appears to crash at the same point as the fix from last time: https://github.com/sass/libsass/blob/3.4.0-RC1/src/sass_context.cpp#L128
The text was updated successfully, but these errors were encountered: