-
Notifications
You must be signed in to change notification settings - Fork 463
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
Sometimes the sass_context_get_error_src
method returns an invalid pointer
#3019
Comments
PR #3020 is a fix for this error. |
Do you know how to try to reproduce that problem? |
Take the This error occurred at once at two users of the LibSass Host for .NET library (see the “Unhandled exceptions” issue). |
AFAICT Side note: in my refactoring branch I changed all char star pointers to be smart pointers, so they can easier outlive the main context if they are still used in some error object ... |
I haven't tried.
Such a situation is excluded. |
So was able to reproduce this with LibSassHost and I've attached the file I used (rename the extension as github didn't allow scss directly). Now the good or bad news is that I tried this file also with sassc with address sanitizer enabled (clang 8.0.1 with |
What is interesting we copy Lines 2896 to 2897 in 4da7c4b
How long will parser state live in the exception handling? |
As long as you don't call I played around a bit more, but lack the understanding how LibSassHost is structured. To reproduce I simply hijacked |
One dangerous thing: Line 2900 in 4da7c4b
|
That's why we copy |
Does it help if |
We release Line 2900 in 4da7c4b
libsass/src/error_handling.hpp Line 56 in 4da7c4b
|
I guess saper is right, a few simple prints seem to confirm this. |
@glebm do you remember why you added For a quick fix we might get away by stealing the owned_src from the error object: c_ctx->error_src = e.pstate.src;
+ e.owned_src = 0; But I wonder why we make a copy in the first place ... |
@saper @mgreter Before that happens, we clear owned src in the copy constructor (ownership transfer): libsass/src/error_handling.hpp Lines 44 to 47 in f7567ea
This should really be a move-only type, but VS2013 doesn't fully support that (not an issue here though).
IIRC it's because |
OK, stepped through all life-cycles and the issue we have at hand is the following. |
@saper Basically I think what you're likely seeing is a similar issue but not for |
Whatever you do please test with https://github.com/sass/libsass/files/1965545/case.zip and ASAN to avoid a regression. |
Sure, please check it too once I have the PR ready |
@mgreter Here is an example where we parse a temporary string not attached to the context: Lines 132 to 134 in 23cabbe
If this throws and we don't copy, the There are probably other places where this happens:
|
Thanks, these should probably go into |
After some more thoughts I'll leave the fix by @Taritsyn as is (#3022), just added some more missing bits. Since I already addressed some of the underlying issues in my upcoming branch I will leave it this way. Also since there is only some minor overhead and only in error cases with this solution. In the future I want to use smart pointers for content references and I also created a class that will allow to mix evaluated parts into the real/parent style-sheet in order to report dynamic cases even better. |
Assemblies are built using two scripts:
It is better to install assembly in the form of NuGet package. In this case, NuGet package will copy the assembly by using MSBuild (see the |
I have added some crazy debugging https://gist.github.com/48ff78d712a17d8936635dd6636ffcde and the result is:
Seems like there is just one instance we are passing around... |
But you are right about (running version patched with https://gist.github.com/48ff78d712a17d8936635dd6636ffcde so line numbers are slightly off)
So |
Here's more: We could not reproduce this problem because diff --git a/sassc.c b/sassc.c
index dcb3fff..29a2279 100644
--- a/sassc.c
+++ b/sassc.c
@@ -53,10 +53,11 @@ int get_argv_utf8(int* argc_ptr, char*** argv_ptr) {
#include <sysexits.h>
#endif
-int output(int error_status, const char* error_message, const char* output_string, const char* outfile) {
+int output(int error_status, const char* error_message, const char *error_src, const char* output_string, const char* outfile) {
if (error_status) {
if (error_message) {
fprintf(stderr, "%s", error_message);
+ fprintf(stderr, "error_src: %s", error_src);
} else {
fprintf(stderr, "An error occurred; no error message available.\n");
}
@@ -139,6 +140,7 @@ int compile_stdin(struct Sass_Options* options, char* outfile) {
ret = output(
sass_context_get_error_status(ctx_out),
sass_context_get_error_message(ctx_out),
+ sass_context_get_error_src(ctx_out),
sass_context_get_output_string(ctx_out),
outfile
);
@@ -160,6 +162,7 @@ int compile_file(struct Sass_Options* options, char* input_path, char* outfile)
ret = output(
sass_context_get_error_status(ctx_out),
sass_context_get_error_message(ctx_out),
+ sass_context_get_error_src(ctx_out),
sass_context_get_output_string(ctx_out),
outfile
);
@@ -168,6 +171,7 @@ int compile_file(struct Sass_Options* options, char* input_path, char* outfile)
ret = output(
sass_context_get_error_status(ctx_out),
sass_context_get_error_message(ctx_out),
+ sass_context_get_error_src(ctx_out),
sass_context_get_source_map_string(ctx_out),
srcmap_file
); After this, it crashes nicely - here is the output of the testcase from @glebm #3019 (comment) This is pure 3.6.2 configured with
(sassc was built in a similar way, I have used
Maybe this is also a root cause for sass/node-sass#2171 |
@mgreter Thanks for fixes! |
I am using the LibSass version 3.6.2. When compiling a large file (for example, modified
_theming.scss
file from the Angular Material package version 8.2.3), that contains a syntax error near the end of file,sass_context_get_error_src
method may return an invalid pointer instead of the source code of this file.The text was updated successfully, but these errors were encountered: