Skip to content

Commit

Permalink
Don't error on css variable syntax
Browse files Browse the repository at this point in the history
  • Loading branch information
@xzyfer
xzyfer committed Oct 6, 2014
1 parent b1ce9a8 commit a471673
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 0 deletions.
3 changes: 3 additions & 0 deletions parser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,9 @@ namespace Sass {
else if (lex< sequence< optional< exactly<'*'> >, identifier > >()) {
prop = new (ctx.mem) String_Constant(path, source_position, lexed);
}
else if (lex< custom_property_name >()) {
prop = new (ctx.mem) String_Constant(path, source_position, lexed);
}
else {
error("invalid property name");
}

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 20, 2014

Contributor

@xzyfer, I think this function has a possible bug. After reporting the error, why the execution continues? Should it not return nullptr or alike after error( .. )?

Consider the following path from line 740 till 758.

(in parenthesis, the line number refers to current master)

741 (798):   'prop' is NULL
742 (799):   Skip this branch, (assume 'peek(0)' is false)
745 (802):   Skip this branch, (assume 'lex()' is false)
748 (805):   Skip this branch, (assume 'lex()' is false)
754 (811):   Skip this branch, (assume '!lex()' is false)
755 (812):   Skip this branch, (assume 'peek(0)' is false)
757 (814):   'prop' is dereferenced, but may still be NULL

I am making code analysis warnings fixes, but stuck on this one as I'm not sure, should this function return in case of error? :)

This comment has been minimized.

Sign in to view

Copy link
@xzyfer

xzyfer Nov 21, 2014

Author Contributor

IIRC error throws, so the function's execution should be halted. Are you saying that's not the case?

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 21, 2014

Contributor

Seems like it. This is what VS' code analyzer came up with after crunching all code paths for couple of minutes. Here is the captured list: http://pastebin.com/qgTgz7nS.

It seems like there is an extra switch for it: _assume_macro, which should be set. It could be a false positive. I have found these two discussions related:
Expand Down
5 changes: 5 additions & 0 deletions prelexer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,11 @@ namespace Sass {
backslash_something > > >(src);
}

// Match CSS css variables.
const char* custom_property_name(const char* src) {
return sequence< exactly<'-'>, exactly<'-'>, identifier >(src);
}

// Match interpolant schemas
const char* identifier_schema(const char* src) {
// follows this pattern: (x*ix*)+ ... well, not quite
Expand Down
2 changes: 2 additions & 0 deletions prelexer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,8 @@ namespace Sass {

const char* backslash_something(const char* src);

// Match CSS css variables.
const char* custom_property_name(const char* src);
// Match a CSS identifier.
const char* identifier(const char* src);
// Match selector names.
Expand Down

3 comments on commit a471673

@mgreter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This somehow sounds like a bug I have fixed in 8957ad8 for sass_context.cpp.
@am11 Are you sure you have that fix included?

@am11
Copy link
Contributor

@am11 am11 commented on a471673 Nov 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgreter, yes I have the latest master. All those CA warnings were captured with 0f275a1.

@mgreter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much all these warnings are because we do not yet check if calloc or malloc fails. We probably also should return 0 in this case, but that means that implementers will need to check the return of each sass_make_xxx call ... I have that on my todo list. Not sure what it should have to do with 0f275a1?

Please sign in to comment.