Crash in sass_compile_file from uninitialised Context.importer

[carey]

I've been working on my own integration of LibSass with Java on Windows, but with my last pull of libsass it's started crashing at:

sass.dll!sass_import_get_function(Sass_C_Import_Descriptor * fn) Line 59
sass.dll!Sass::Parser::parse_import() Line 161
sass.dll!Sass::Parser::parse() Line 53
sass.dll!Sass::Context::parse_file() Line 250
sass.dll!Sass::Context::compile_file() Line 294
sass.dll!sass_compile_file(sass_file_context * c_ctx) Line 220

Initialising importer to 0 lets sass_compile_file run successfully. (It crashes later in sass_free_file_context when free() detects a buffer overflow.)

[carey]

The crash in sass_free_file_context is because copy_strings only allocates (8 * num) + 1 bytes instead of 8 * (num + 1). I would use calloc(num + 1, sizeof(char*)) instead.

There's a copy_strings in both sass_context.cpp and sass_interface.cpp, and both seem to have the same problem.

[mgreter]

From your code I guess you are mixing old and new API? Sass_C_Import_Descriptor will only work with the new API and sass_compile_file(sass_file_context * c_ctx) seems to be from the old API.
Maybe we should add something to avoid beeing able to include both API headers at the same time ...

On your remarks on copy_strings you are right, it should use parentheses. It should not make much of a difference for the second allocation (since the size of one char is pretty much always one byte). But for the actual pointer array, it could make a difference (I would guess it should be short of 3 to 7 bytes, either if you have 32 or 64 bit pointers). Added a commit to address this.

IMO calloc is not needed as all the allocated memory is overwritten properly in that function.
Funny that nobody ever had this issue, since the function is pretty much called for every compile!

[carey]

I'm only using the sass_compile_file API, but when it creates a Sass::Context::Data it leaves the importer field uninitialised, rather than setting it to a null pointer. My code crashes when LibSass tries to use the random data here on encountering @import, unless I modify sass_compile_file to set importer to 0.

The Visual C++ 2013 debugging runtime enables a lot of consistency checks in malloc and free, like Linux does when MALLOC_CHECK_ is set to 2. I was running under the debugger to see why @import caused a crash, which is why free_string_array started crashing.

The C standard defines sizeof(char) to always be 1.

[carey]

Here's my test program that crashes in sass_import_get_function:

#include <stdio.h>

#include "libsass/sass_interface.h"

int main()
{
    sass_file_context *ctx = sass_new_file_context();

    ctx->input_path = "test.scss";

    int status = sass_compile_file(ctx);
    if (ctx->error_status == 0) {
        puts(ctx->output_string);
    } else {
        puts(ctx->error_message);
    }

    sass_free_file_context(ctx);

    return 0;
}

[mgreter]

When I compile this on linux I get:

Error: File to read not found or unreadable: test.scss

Is this still an issue with latest master?

[carey]

Sorry, that code looks for the SCSS file I was using for testing. The minimum needed to reproduce the problem is a file test.scss like this in the current directory:

@import "inc";

[mgreter]

Works for me!?

test.scss:

@import "inc";

inc.scss

a { color: red; }

# ./main

a {
  color: red; }

[carey]

I can't reproduce it with GCC on Linux, since the stack is initialised to zeros there, so the pointers in Sass::Context::Data end up being null. Visual Studio fills the stack with 0xcc to detect programming problems, though, so source_c_str_, include_paths_c_str_, include_paths_array_ and importer_ all start out as 0xcccccccccccccccc.

include_paths_str_ and include_paths_array_ get changed to valid pointers in sass_compile_file, and source_c_str_ isn't used, but importer_ is left uninitialised and copied to cpp_ctx, so sass_import_get_function crashes.

I've been able to fix the crash in Windows by either initialising all fields in Sass::Context::Data:

--- a/context.hpp
+++ b/context.hpp
@@ -83,6 +83,8 @@ namespace Sass {
     bool _skip_source_map_update; // status flag to skip source map updates

     KWD_ARG_SET(Data) {
+    public:
+      Data() : source_c_str_(), include_paths_c_str_(), include_paths_array_(), importer_() {}
       KWD_ARG(Data, const char*,     source_c_str);
       KWD_ARG(Data, string,          entry_point);
       KWD_ARG(Data, string,          input_path);

or by explicitly setting importer in sass_compile_file (and sass_compile):

--- a/sass_interface.cpp
+++ b/sass_interface.cpp
@@ -209,6 +209,7 @@ extern "C" {
                        .include_paths_array(0)
                        .include_paths(vector<string>())
                        .precision(c_ctx->options.precision ? c_ctx->options.precision : 5)
+                       .importer(0)
       );
       if (c_ctx->c_functions) {
         struct Sass_C_Function_Descriptor** this_func_data = c_ctx->c_functions;

I prefer the former, since that way both POD and non-POD fields of the Sass::Context::Data class get initialised in the constructor, not just the non-POD fields. It looks like LibSass prefers the latter style, though.

[mgreter]

Thanks for digging into the issue 👍 I will have a look and add your fix.
Anything speaking against adding both fixes (IMO it shouldn't hurt much)!?

[am11]

Perhaps this VS issue is related: sass/node-sass#530 (comment) :)
It throws "Memory Access violation" on accessing the queue.

[carey]

There's no reason not to make both changes. The mythical perfect optimising compiler would generate the same code for either or both.

@am11 The first commit in a94377b should fix the crash in free_string_array, making it possible to debug the real problem in parse_file. That looks like a different problem, where it looks like this has ended up a null pointer.

[am11]

@carey, thanks for looking into it. I have been wandering around the code to find more improvement and I ran Code Analysis on it, it gives some interesting pointers about memory related issues.

Here is the log: http://pastebin.com/qgTgz7nS. I have fixed couple of them, but for example this: a471673#commitcomment-8652376, I couldn't figure out.

The interesting thing is, it draws the entire path to memory corruptions/leaks. There are total of 26 or warnings Code Analysis throws. It might be worth fixing (unlike .NET projects, it takes quite sometime to test all code paths, at least on i7 Lenovo).

[carey]

Thanks for the fix in 375b769. There's very similar code in sass_compile_file that needs fixing in the same way as in sass_compile.

After that fix too, and realising that sass_make_function doesn't copy the signature string, I'm using the old API from Java without crashes.

[am11]

I have found that node-sass importer callback is invoked till 21688e1. As soon as I checkout to next commit e0a5d03, it stops calling the cb.

Another regression is described here 8957ad8#commitcomment-8644934.