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

Fix SIGSEGV from bad cast #2322

Closed
wants to merge 1 commit into from
Closed

Fix SIGSEGV from bad cast #2322

wants to merge 1 commit into from

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Feb 7, 2017

  • don't segfault
  • don't error
  • produce correct output

Fixes #2321
Spec sass/sass-spec#1081

@xzyfer xzyfer added this to the 3.5.0.beta.3 milestone Feb 7, 2017
@xzyfer xzyfer self-assigned this Feb 7, 2017
@mgreter
Copy link
Contributor

mgreter commented Feb 7, 2017

I see you already started fixing this. I did the same and came up with this hotfix:

@@ -167,8 +167,15 @@ namespace Sass {
       else if (a->is_rest_argument()) {
         // normal param and rest arg
         List_Obj arglist = Cast<List>(a->value());
+        if (!arglist) {
+          if (Expression_Obj arg = Cast<Expression>(a->value())) {
+            arglist = SASS_MEMORY_NEW(List, a->pstate(), 1);
+            arglist->append(arg);
+          }
+        }
+
         // empty rest arg - treat all args as default values
-        if (!arglist->length()) {
+        if (!arglist || !arglist->length()) {
           break;
         } else {
           if (arglist->length() > LP - ip && !ps->has_rest_parameter()) {

Arglists is one of those classes that need an urgent refactoring.
But there are many like these on the todo list (ie. all vectorized classes)!

@xzyfer
Copy link
Contributor Author

xzyfer commented Feb 7, 2017

@mgreter I'm about to go to bed. If want to take this over whilst I'm sleeping feel free.

I got distracted by making a reduced test case for sass/node-sass#1879

@mgreter
Copy link
Contributor

mgreter commented Feb 7, 2017

OK, will make a PR. Note that I believe the other segfault in Alpine to be a heisenbug. I don't think you will be able to reduce the test case while still exposing the same bug. So far it looks like we are overwriting memory where we should not. Only explanation I could come up so far is that we rely on some undefined behavior somewhere, which only breaks in that certain case. It somewhat looks like stack memory is overwritten, which leads to invalid jmp address.

To be clear, the segfault in my case happens when the executable tries to jump into the "parse_expression" function. But interestingly it is called from "parse_arguments", so this might be related. I know that we do store strange variations of AST nodes in these containers. But I haven't finished my assessments, so this could also be the wrong assumption.

@xzyfer
Copy link
Contributor Author

xzyfer commented Feb 7, 2017

I managed to come up with a relatively small reproducible test case see sass/node-sass#1879 (comment).

@mgreter
Copy link
Contributor

mgreter commented Feb 15, 2017

Fixed in #2323

@mgreter mgreter closed this Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants