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 crash in Parser::parse_term() #846

Merged
merged 1 commit into from
Feb 16, 2015
Merged

Fix crash in Parser::parse_term() #846

merged 1 commit into from
Feb 16, 2015

Conversation

mmaxim
Copy link
Contributor

@mmaxim mmaxim commented Jan 19, 2015

Parser::parse_term uses dynamic_cast to attempt to see if the LHS of an expression can be treated as a String_Schema in order to check if string interpolation is in play. Unfortunately, it relies on dynamic_cast throwing an exception if the cast fails, which is not how C++11 behaves. An exception is only thrown from dynamic_cast when references are being cast, not pointers (since a reference cannot be null, but a pointer can):

http://stackoverflow.com/questions/16476613/when-dynamic-cast-will-throw-exception-in-case-used-with-pointer

The change in this pull request removes the try/catch construct, and replaces it with a simple check to see if the pointer is null or not.

…y references (since it can't assign null to a reference). This patch removes the try/catch block and just checks the pointer for null, fixing a crasher here if the dynamic cast fails.
@xzyfer xzyfer added this to the 3.2 milestone Jan 20, 2015
@xzyfer
Copy link
Contributor

xzyfer commented Jan 20, 2015

Nice catch @mmaxim ! Was this code causing you actual issues? I ask because it'd be great to add a test spec that catches this crash.

@mmaxim
Copy link
Contributor Author

mmaxim commented Jan 21, 2015

Yeah this did cause a problem, which is how I stumbled upon it. It could mean there is a deeper bug int he parser to even get to that block of code in the first place, but I didn't look that hard (since this fix just made everything work for me). I'll work on getting a minimal test case.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 21, 2015

Thanks for working on a test case. We really appreciate it.
On 22 Jan 2015 03:39, "Mike Maxim" notifications@github.com wrote:

Yeah this did cause a problem, which is how I stumbled upon it. It could
mean there is a deeper bug int he parser to even get to that block of code
in the first place, but I didn't look that hard (since this fix just made
everything work for me). I'll work on getting a minimal test case.


Reply to this email directly or view it on GitHub
#846 (comment).

@xzyfer
Copy link
Contributor

xzyfer commented Feb 15, 2015

@mgreter does this conflict with your work? If not lets get it shipped.

@mgreter
Copy link
Contributor

mgreter commented Feb 15, 2015

LGTM!

xzyfer added a commit that referenced this pull request Feb 16, 2015
Fix crash in Parser::parse_term()
@xzyfer xzyfer merged commit 8336397 into sass:master Feb 16, 2015
@mgreter mgreter assigned xzyfer and unassigned xzyfer Mar 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants