Return NULL if the Object cannot be unwrapped

Fix crash in List::SetValue() etc. when trying
to add a bare JavaScript object.

Check for bare objects in lists and maps in test.

Don't use C++ exceptions in the binding code.

Author: saper

binding.gyp

@@ -60,22 +60,12 @@
               '-std=c++11'
             ],
             'OTHER_LDFLAGS': [],
-            'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',
+            'GCC_ENABLE_CPP_EXCEPTIONS': 'NO',
             'MACOSX_DEPLOYMENT_TARGET': '10.7'
           }
         }],
-        ['OS=="win"', {
-          'msvs_settings': {
-            'VCCLCompilerTool': {
-              'AdditionalOptions': [
-                '/EHsc'
-              ]
-            }
-          }
-        }],
         ['OS!="win"', {
           'cflags_cc+': [
-            '-fexceptions',
             '-std=c++0x'
           ]
         }]

src/binding.cpp

@@ -29,12 +29,7 @@ union Sass_Value* sass_custom_function(const union Sass_Value* s_args, Sass_Func
     argv.push_back((void*)sass_list_get_value(s_args, i));
   }
 
-  try {
-    return bridge(argv);
-  }
-  catch (const std::exception& e) {
-    return sass_make_error(e.what());
-  }
+  return bridge(argv);
 }
 
 int ExtractOptions(v8::Local<v8::Object> options, void* cptr, sass_context_wrapper* ctx_w, bool is_file, bool is_sync) {

src/custom_function_bridge.cpp

@@ -2,13 +2,14 @@
 #include <stdexcept>
 #include "custom_function_bridge.h"
 #include "sass_types/factory.h"
+#include "sass_types/value.h"
 
 Sass_Value* CustomFunctionBridge::post_process_return_value(v8::Local<v8::Value> val) const {
-  try {
-    return SassTypes::Factory::unwrap(val)->get_sass_value();
-  }
-  catch (const std::invalid_argument& e) {
-    return sass_make_error(e.what());
+  SassTypes::Value *v_;
+  if ((v_ = SassTypes::Factory::unwrap(val))) {
+    return v_->get_sass_value();
+  } else {
+    return sass_make_error("A SassValue object was expected.");
   }
 }
 

src/sass_types/boolean.cpp

@@ -67,6 +67,9 @@ namespace SassTypes
   }
 
   NAN_METHOD(Boolean::GetValue) {
-    info.GetReturnValue().Set(Nan::New(static_cast<Boolean*>(Factory::unwrap(info.This()))->value));
+    Boolean *out;
+    if ((out = static_cast<Boolean*>(Factory::unwrap(info.This())))) { 
+      info.GetReturnValue().Set(Nan::New(out->value));
+    }
   }
 }

src/sass_types/factory.cpp

@@ -63,7 +63,7 @@ namespace SassTypes
   Value* Factory::unwrap(v8::Local<v8::Value> obj) {
     // Todo: non-SassValue objects could easily fall under that condition, need to be more specific.
     if (!obj->IsObject() || obj.As<v8::Object>()->InternalFieldCount() != 1) {
-      throw std::invalid_argument("A SassValue object was expected.");
+      return NULL;
     }
 
     return static_cast<Value*>(Nan::GetInternalFieldPointer(obj.As<v8::Object>(), 0));

src/sass_types/list.cpp

@@ -71,7 +71,11 @@ namespace SassTypes
     }
 
     Value* sass_value = Factory::unwrap(info[1]);
-    sass_list_set_value(unwrap(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
+    if (sass_value) {
+      sass_list_set_value(unwrap(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
+    } else {
+      Nan::ThrowTypeError(Nan::New<v8::String>("A SassValue is expected as the list item").ToLocalChecked());
+    }
   }
 
   NAN_METHOD(List::GetSeparator) {

src/sass_types/map.cpp

@@ -62,7 +62,11 @@ namespace SassTypes
     }
 
     Value* sass_value = Factory::unwrap(info[1]);
-    sass_map_set_value(unwrap(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
+    if (sass_value) {
+      sass_map_set_value(unwrap(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
+    } else {
+      Nan::ThrowTypeError(Nan::New<v8::String>("A SassValue is expected as a map value").ToLocalChecked());
+    }
   }
 
   NAN_METHOD(Map::GetKey) {
@@ -100,7 +104,11 @@ namespace SassTypes
     }
 
     Value* sass_value = Factory::unwrap(info[1]);
-    sass_map_set_key(unwrap(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
+    if (sass_value) {
+      sass_map_set_key(unwrap(info.This())->value, Nan::To<uint32_t>(info[0]).FromJust(), sass_value->get_sass_value());
+    } else {
+      Nan::ThrowTypeError(Nan::New<v8::String>("A SassValue is expected as a map key").ToLocalChecked());
+    }
   }
 
   NAN_METHOD(Map::GetLength) {

src/sass_types/sass_value_wrapper.h

@@ -118,6 +118,7 @@ namespace SassTypes
 
   template <class T>
   T* SassValueWrapper<T>::unwrap(v8::Local<v8::Object> obj) {
+    /* This maybe NULL */
     return static_cast<T*>(Factory::unwrap(obj));
   }
 

test/api.js

@@ -859,6 +859,72 @@ describe('api', function() {
       });
     });
 
+    it('should fail when trying to set a bare number as the List item', function(done) {
+      sass.render({
+        data: 'div { color: foo(); }',
+        functions: {
+          'foo()': function() {
+            var out = new sass.types.List(1);
+            out.setValue(0, 2);
+            return out;
+          }
+        }
+      }, function(error) {
+        assert.ok(/Supplied value should be a SassValue object/.test(error.message));
+        done();
+      });
+    });
+
+    it('should fail when trying to set a bare Object as the List item', function(done) {
+      sass.render({
+        data: 'div { color: foo(); }',
+        functions: {
+          'foo()': function() {
+            var out = new sass.types.List(1);
+            out.setValue(0, {});
+            return out;
+          }
+        }
+      }, function(error) {
+        assert.ok(/A SassValue is expected as the list item/.test(error.message));
+        done();
+      });
+    });
+
+    it('should fail when trying to set a bare Object as the Map key', function(done) {
+      sass.render({
+        data: 'div { color: foo(); }',
+        functions: {
+          'foo()': function() {
+            var out = new sass.types.Map(1);
+            out.setKey(0, {});
+            out.setValue(0, new sass.types.String('aaa'));
+            return out;
+          }
+        }
+      }, function(error) {
+        assert.ok(/A SassValue is expected as a map key/.test(error.message));
+        done();
+      });
+    });
+
+    it('should fail when trying to set a bare Object as the Map value', function(done) {
+      sass.render({
+        data: 'div { color: foo(); }',
+        functions: {
+          'foo()': function() {
+            var out = new sass.types.Map(1);
+            out.setKey(0, new sass.types.String('aaa'));
+            out.setValue(0, {});
+            return out;
+          }
+        }
+      }, function(error) {
+        assert.ok(/A SassValue is expected as a map value/.test(error.message));
+        done();
+      });
+    });
+
     it('should always map null, true and false to the same (immutable) object', function(done) {
       var counter = 0;