Assertion/Exception when using foundation with libsass 3.2.5

[driekus77]

When importing foundation scss:
@import
'foundation/_settings',
'foundation'
;

ast.cpp line 674 exception
Number::Number

WRONG:
if (u[r] == '/') nominator = false;
if (r == string::npos) break;
else l = r + 1;

WORKS:
if (r == string::npos) break;
else if (u[r] == '/') nominator = false;
else l = r + 1;

r must be checked before it is used!

Hope somebody with more domain knowledge can check if my suggested fix is ok.

Greetings,

Henry

[mgreter]

Seems valid; the memory access u[npos] will pretty sure be out of bound!

[xzyfer]

I would a test case for this. I was unable to reproduce it with the latest foundation.

[xzyfer]

@driekus77 can you please confirm which version of foundation you are using. It should be in the bower.json or package.json in the foundation folder.

[xzyfer]

This is blocked on waiting for a test case.

[xzyfer]

I was able to compiled every 5.x release of foundation, include the latest master with LibSass 3.2.5 without this error.

[saper]

@driekus77 did you customize _settings.scss ? Can you post the file?

[xzyfer]

The upstream issue sass/libsass-net#25 suggests they did not.

[driekus77]

Its an assertion so probably you only see/discover it only when running libsass in debug.

I tested it on Windows using Visual Studio 2013 to get libsass in debug

Unfortunately I don't know the foundations version number anymore because I only have the .scss files. I can push the foundation scss files if you want?

Remember that I tested it with libsass-net (C# wrapper arround libsass)

[saper]

Please push whatever you have, we need to reproduce this bug somehow...

[xzyfer]

My guess is we're not going to able to reproduce. I tried everything I could of to hit the code path in question. I believe there should be a memory out of bond bug here as @mgreter points out.

The patch in #1319 seems perfectly valid to me. Either way it's an improvement. Let's just 🚢.

[driekus77]

Ok I pushed my foundation Sass/Scss files to:
https://github.com/driekus77/SassDebugTests/tree/master/foundation

Use default.scss as input for your test

[driekus77]

I overlooked something concerning my suggested fix. A few lines before the fix and right after the u.find_first_of...:

string unit(u.substr(l, r - l)); <== r is also used and can be npos (huge number)

But when I move the npos check up right after the u.find_first_of...
Foundation output is not oke.

Can somebody with domain knowledge please check if r needs to be checked sooner?

Thanks & Greetings,

Henry

[mgreter]

Please check the referenced commits: xzyfer@c45a702 (or on master: 0939984)

+        string unit(u.substr(l, r == string::npos ? r : r - l));

Then if you are worried about passing r (npos) to substr, see:
http://www.cplusplus.com/reference/string/string/substr/

A value of string::npos indicates all characters until the end of the string.

Looks safe enough for me 😉

[driekus77]

Aha sorry for the fuss didn't see youre commit. Only looked at the code in 3.5.2 release.

Thanks for your quick responds.

Greetings, Henry