From 4a6a9cc6712b1c244a16184c5c6339fbc73c8be9 Mon Sep 17 00:00:00 2001 From: Marcin Cieslak Date: Tue, 8 Sep 2015 22:12:23 +0000 Subject: [PATCH 1/4] Introduce SassValueWrapper::fail() This method uses Sass error value as a vehicle to report the error message, but returns NULL to indicate constructor failure. This is used to tell successful creation of the Sass error object from the constructor failure, where no valid Sass object can be created. --- src/sass_types/sass_value_wrapper.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/sass_types/sass_value_wrapper.h b/src/sass_types/sass_value_wrapper.h index aed4e2e34..f6be33c5d 100644 --- a/src/sass_types/sass_value_wrapper.h +++ b/src/sass_types/sass_value_wrapper.h @@ -25,6 +25,7 @@ namespace SassTypes static v8::Local get_constructor(); static v8::Local get_constructor_template(); static NAN_METHOD(New); + static Sass_Value *fail(const char *, Sass_Value **); protected: Sass_Value* value; @@ -119,6 +120,12 @@ namespace SassTypes T* SassValueWrapper::unwrap(v8::Local obj) { return static_cast(Factory::unwrap(obj)); } + + template + Sass_Value *SassValueWrapper::fail(const char *reason, Sass_Value **out) { + *out = sass_make_error(reason); + return NULL; + } } From dd1f3a9d9e84acf65580e2f66496ca74a4bbaa0c Mon Sep 17 00:00:00 2001 From: Marcin Cieslak Date: Tue, 8 Sep 2015 22:51:51 +0000 Subject: [PATCH 2/4] Return NULL when failed to construct a Sass value ::construct() methods may return NULL and indicate an error in the returned error value. --- src/sass_types/color.cpp | 12 ++++++------ src/sass_types/color.h | 2 +- src/sass_types/error.cpp | 6 +++--- src/sass_types/error.h | 2 +- src/sass_types/list.cpp | 8 ++++---- src/sass_types/list.h | 2 +- src/sass_types/map.cpp | 6 +++--- src/sass_types/map.h | 2 +- src/sass_types/number.cpp | 8 ++++---- src/sass_types/number.h | 2 +- src/sass_types/sass_value_wrapper.h | 8 ++++---- src/sass_types/string.cpp | 6 +++--- src/sass_types/string.h | 2 +- 13 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/sass_types/color.cpp b/src/sass_types/color.cpp index 8f8efba87..ed22b13cb 100644 --- a/src/sass_types/color.cpp +++ b/src/sass_types/color.cpp @@ -5,14 +5,14 @@ namespace SassTypes { Color::Color(Sass_Value* v) : SassValueWrapper(v) {} - Sass_Value* Color::construct(const std::vector> raw_val) { + Sass_Value* Color::construct(const std::vector> raw_val, Sass_Value **out) { double a = 1.0, r = 0, g = 0, b = 0; unsigned argb; switch (raw_val.size()) { case 1: if (!raw_val[0]->IsNumber()) { - throw std::invalid_argument("Only argument should be an integer."); + return fail("Only argument should be an integer.", out); } argb = Nan::To(raw_val[0]).FromJust(); @@ -24,7 +24,7 @@ namespace SassTypes case 4: if (!raw_val[3]->IsNumber()) { - throw std::invalid_argument("Constructor arguments should be numbers exclusively."); + return fail("Constructor arguments should be numbers exclusively.", out); } a = Nan::To(raw_val[3]).FromJust(); @@ -32,7 +32,7 @@ namespace SassTypes case 3: if (!raw_val[0]->IsNumber() || !raw_val[1]->IsNumber() || !raw_val[2]->IsNumber()) { - throw std::invalid_argument("Constructor arguments should be numbers exclusively."); + return fail("Constructor arguments should be numbers exclusively.", out); } r = Nan::To(raw_val[0]).FromJust(); @@ -44,10 +44,10 @@ namespace SassTypes break; default: - throw std::invalid_argument("Constructor should be invoked with either 0, 1, 3 or 4 arguments."); + return fail("Constructor should be invoked with either 0, 1, 3 or 4 arguments.", out); } - return sass_make_color(r, g, b, a); + return *out = sass_make_color(r, g, b, a); } void Color::initPrototype(v8::Local proto) { diff --git a/src/sass_types/color.h b/src/sass_types/color.h index 7ef10db19..0be355110 100644 --- a/src/sass_types/color.h +++ b/src/sass_types/color.h @@ -10,7 +10,7 @@ namespace SassTypes public: Color(Sass_Value*); static char const* get_constructor_name() { return "SassColor"; } - static Sass_Value* construct(const std::vector>); + static Sass_Value* construct(const std::vector>, Sass_Value **); static void initPrototype(v8::Local); diff --git a/src/sass_types/error.cpp b/src/sass_types/error.cpp index f02dd12dc..03c6307c4 100644 --- a/src/sass_types/error.cpp +++ b/src/sass_types/error.cpp @@ -6,18 +6,18 @@ namespace SassTypes { Error::Error(Sass_Value* v) : SassValueWrapper(v) {} - Sass_Value* Error::construct(const std::vector> raw_val) { + Sass_Value* Error::construct(const std::vector> raw_val, Sass_Value **out) { char const* value = ""; if (raw_val.size() >= 1) { if (!raw_val[0]->IsString()) { - throw std::invalid_argument("Argument should be a string."); + return fail("Argument should be a string.", out); } value = create_string(raw_val[0]); } - return sass_make_error(value); + return *out = sass_make_error(value); } void Error::initPrototype(v8::Local) {} diff --git a/src/sass_types/error.h b/src/sass_types/error.h index ada3769f3..01786fdaa 100644 --- a/src/sass_types/error.h +++ b/src/sass_types/error.h @@ -10,7 +10,7 @@ namespace SassTypes public: Error(Sass_Value*); static char const* get_constructor_name() { return "SassError"; } - static Sass_Value* construct(const std::vector>); + static Sass_Value* construct(const std::vector>, Sass_Value **); static void initPrototype(v8::Local); }; diff --git a/src/sass_types/list.cpp b/src/sass_types/list.cpp index 4e9515f95..2e019ffd2 100644 --- a/src/sass_types/list.cpp +++ b/src/sass_types/list.cpp @@ -5,27 +5,27 @@ namespace SassTypes { List::List(Sass_Value* v) : SassValueWrapper(v) {} - Sass_Value* List::construct(const std::vector> raw_val) { + Sass_Value* List::construct(const std::vector> raw_val, Sass_Value **out) { size_t length = 0; bool comma = true; if (raw_val.size() >= 1) { if (!raw_val[0]->IsNumber()) { - throw std::invalid_argument("First argument should be an integer."); + return fail("First argument should be an integer.", out); } length = Nan::To(raw_val[0]).FromJust(); if (raw_val.size() >= 2) { if (!raw_val[1]->IsBoolean()) { - throw std::invalid_argument("Second argument should be a boolean."); + return fail("Second argument should be a boolean.", out); } comma = Nan::To(raw_val[1]).FromJust(); } } - return sass_make_list(length, comma ? SASS_COMMA : SASS_SPACE); + return *out = sass_make_list(length, comma ? SASS_COMMA : SASS_SPACE); } void List::initPrototype(v8::Local proto) { diff --git a/src/sass_types/list.h b/src/sass_types/list.h index 5f3a3aeac..c43b75484 100644 --- a/src/sass_types/list.h +++ b/src/sass_types/list.h @@ -10,7 +10,7 @@ namespace SassTypes public: List(Sass_Value*); static char const* get_constructor_name() { return "SassList"; } - static Sass_Value* construct(const std::vector>); + static Sass_Value* construct(const std::vector>, Sass_Value **); static void initPrototype(v8::Local); diff --git a/src/sass_types/map.cpp b/src/sass_types/map.cpp index 2be305b5a..3bd41a9d0 100644 --- a/src/sass_types/map.cpp +++ b/src/sass_types/map.cpp @@ -5,18 +5,18 @@ namespace SassTypes { Map::Map(Sass_Value* v) : SassValueWrapper(v) {} - Sass_Value* Map::construct(const std::vector> raw_val) { + Sass_Value* Map::construct(const std::vector> raw_val, Sass_Value **out) { size_t length = 0; if (raw_val.size() >= 1) { if (!raw_val[0]->IsNumber()) { - throw std::invalid_argument("First argument should be an integer."); + return fail("First argument should be an integer.", out); } length = Nan::To(raw_val[0]).FromJust(); } - return sass_make_map(length); + return *out = sass_make_map(length); } void Map::initPrototype(v8::Local proto) { diff --git a/src/sass_types/map.h b/src/sass_types/map.h index 26fc2286d..832585dd5 100644 --- a/src/sass_types/map.h +++ b/src/sass_types/map.h @@ -10,7 +10,7 @@ namespace SassTypes public: Map(Sass_Value*); static char const* get_constructor_name() { return "SassMap"; } - static Sass_Value* construct(const std::vector>); + static Sass_Value* construct(const std::vector>, Sass_Value **); static void initPrototype(v8::Local); diff --git a/src/sass_types/number.cpp b/src/sass_types/number.cpp index b832e809b..1605516b2 100644 --- a/src/sass_types/number.cpp +++ b/src/sass_types/number.cpp @@ -6,27 +6,27 @@ namespace SassTypes { Number::Number(Sass_Value* v) : SassValueWrapper(v) {} - Sass_Value* Number::construct(const std::vector> raw_val) { + Sass_Value* Number::construct(const std::vector> raw_val, Sass_Value **out) { double value = 0; char const* unit = ""; if (raw_val.size() >= 1) { if (!raw_val[0]->IsNumber()) { - throw std::invalid_argument("First argument should be a number."); + return fail("First argument should be a number.", out); } value = Nan::To(raw_val[0]).FromJust(); if (raw_val.size() >= 2) { if (!raw_val[1]->IsString()) { - throw std::invalid_argument("Second argument should be a string."); + return fail("Second argument should be a string.", out); } unit = create_string(raw_val[1]); } } - return sass_make_number(value, unit); + return *out = sass_make_number(value, unit); } void Number::initPrototype(v8::Local proto) { diff --git a/src/sass_types/number.h b/src/sass_types/number.h index b228d25fa..48a02361f 100644 --- a/src/sass_types/number.h +++ b/src/sass_types/number.h @@ -11,7 +11,7 @@ namespace SassTypes public: Number(Sass_Value*); static char const* get_constructor_name() { return "SassNumber"; } - static Sass_Value* construct(const std::vector>); + static Sass_Value* construct(const std::vector>, Sass_Value **out); static void initPrototype(v8::Local); diff --git a/src/sass_types/sass_value_wrapper.h b/src/sass_types/sass_value_wrapper.h index f6be33c5d..ca2856720 100644 --- a/src/sass_types/sass_value_wrapper.h +++ b/src/sass_types/sass_value_wrapper.h @@ -95,15 +95,15 @@ namespace SassTypes localArgs[i] = info[i]; } if (info.IsConstructCall()) { - try { - Sass_Value* value = T::construct(localArgs); + Sass_Value* value; + if (T::construct(localArgs, &value) != NULL) { T* obj = new T(value); sass_delete_value(value); Nan::SetInternalFieldPointer(info.This(), 0, obj); obj->js_object.Reset(info.This()); - } catch (const std::exception& e) { - return Nan::ThrowError(Nan::New(e.what()).ToLocalChecked()); + } else { + return Nan::ThrowError(Nan::New(sass_error_get_message(value)).ToLocalChecked()); } } else { v8::Local cons = T::get_constructor(); diff --git a/src/sass_types/string.cpp b/src/sass_types/string.cpp index 2c2373ac6..d97095bbc 100644 --- a/src/sass_types/string.cpp +++ b/src/sass_types/string.cpp @@ -6,18 +6,18 @@ namespace SassTypes { String::String(Sass_Value* v) : SassValueWrapper(v) {} - Sass_Value* String::construct(const std::vector> raw_val) { + Sass_Value* String::construct(const std::vector> raw_val, Sass_Value **out) { char const* value = ""; if (raw_val.size() >= 1) { if (!raw_val[0]->IsString()) { - throw std::invalid_argument("Argument should be a string."); + return fail("Argument should be a string.", out); } value = create_string(raw_val[0]); } - return sass_make_string(value); + return *out = sass_make_string(value); } void String::initPrototype(v8::Local proto) { diff --git a/src/sass_types/string.h b/src/sass_types/string.h index c7942a86d..2e72c8251 100644 --- a/src/sass_types/string.h +++ b/src/sass_types/string.h @@ -10,7 +10,7 @@ namespace SassTypes public: String(Sass_Value*); static char const* get_constructor_name() { return "SassString"; } - static Sass_Value* construct(const std::vector>); + static Sass_Value* construct(const std::vector>, Sass_Value **); static void initPrototype(v8::Local); From f6ebeab80eb87155d88c781fecd7163bcd708084 Mon Sep 17 00:00:00 2001 From: Marcin Cieslak Date: Tue, 8 Sep 2015 23:17:50 +0000 Subject: [PATCH 3/4] Throw V8 exception on invalid Sass_Value In the unlikely event libsass gives us the value we don't understand use V8 exception to report failure and exit before calling a user-defined custom function. XXX We do not have currently any way to test this (that would require mocking libsass). --- src/callback_bridge.h | 8 ++++++++ src/sass_types/factory.cpp | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/callback_bridge.h b/src/callback_bridge.h index b95887c34..cf749c1b8 100644 --- a/src/callback_bridge.h +++ b/src/callback_bridge.h @@ -98,7 +98,12 @@ T CallbackBridge::operator()(std::vector argv) { * post_process_args(). */ Nan::HandleScope scope; + Nan::TryCatch try_catch; std::vector> argv_v8 = pre_process_args(argv); + if (try_catch.HasCaught()) { + Nan::FatalException(try_catch); + } + argv_v8.push_back(Nan::New(wrapper)); return this->post_process_return_value( @@ -149,6 +154,9 @@ void CallbackBridge::dispatched_async_uv_callback(uv_async_t *req) { Nan::TryCatch try_catch; std::vector> argv_v8 = bridge->pre_process_args(bridge->argv); + if (try_catch.HasCaught()) { + Nan::FatalException(try_catch); + } argv_v8.push_back(Nan::New(bridge->wrapper)); bridge->callback->Call(argv_v8.size(), &argv_v8[0]); diff --git a/src/sass_types/factory.cpp b/src/sass_types/factory.cpp index 9bcf07005..5e69299b7 100644 --- a/src/sass_types/factory.cpp +++ b/src/sass_types/factory.cpp @@ -39,7 +39,9 @@ namespace SassTypes return new Error(v); default: - throw std::invalid_argument("Unknown type encountered."); + const char *msg = "Unknown type encountered."; + Nan::ThrowTypeError(Nan::New(msg).ToLocalChecked()); + return new Error(sass_make_error(msg)); } } From 228b39d5f05c1c698ee1da3c29d390f3ab2c62f1 Mon Sep 17 00:00:00 2001 From: Marcin Cieslak Date: Wed, 9 Sep 2015 01:03:56 +0000 Subject: [PATCH 4/4] 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. --- binding.gyp | 12 +----- src/binding.cpp | 7 +-- src/custom_function_bridge.cpp | 11 ++--- src/sass_types/boolean.cpp | 5 ++- src/sass_types/factory.cpp | 2 +- src/sass_types/list.cpp | 6 ++- src/sass_types/map.cpp | 12 +++++- src/sass_types/sass_value_wrapper.h | 1 + test/api.js | 66 +++++++++++++++++++++++++++++ 9 files changed, 95 insertions(+), 27 deletions(-) diff --git a/binding.gyp b/binding.gyp index 8a5176924..ec57d30e0 100644 --- a/binding.gyp +++ b/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' ] }] diff --git a/src/binding.cpp b/src/binding.cpp index 19050039f..cb7beafbc 100644 --- a/src/binding.cpp +++ b/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 options, void* cptr, sass_context_wrapper* ctx_w, bool is_file, bool is_sync) { diff --git a/src/custom_function_bridge.cpp b/src/custom_function_bridge.cpp index 9885d0482..f0e49b6c8 100644 --- a/src/custom_function_bridge.cpp +++ b/src/custom_function_bridge.cpp @@ -2,13 +2,14 @@ #include #include "custom_function_bridge.h" #include "sass_types/factory.h" +#include "sass_types/value.h" Sass_Value* CustomFunctionBridge::post_process_return_value(v8::Local 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."); } } diff --git a/src/sass_types/boolean.cpp b/src/sass_types/boolean.cpp index ebc2b2f37..89c7a9659 100644 --- a/src/sass_types/boolean.cpp +++ b/src/sass_types/boolean.cpp @@ -67,6 +67,9 @@ namespace SassTypes } NAN_METHOD(Boolean::GetValue) { - info.GetReturnValue().Set(Nan::New(static_cast(Factory::unwrap(info.This()))->value)); + Boolean *out; + if ((out = static_cast(Factory::unwrap(info.This())))) { + info.GetReturnValue().Set(Nan::New(out->value)); + } } } diff --git a/src/sass_types/factory.cpp b/src/sass_types/factory.cpp index 5e69299b7..1c91f7a2f 100644 --- a/src/sass_types/factory.cpp +++ b/src/sass_types/factory.cpp @@ -63,7 +63,7 @@ namespace SassTypes Value* Factory::unwrap(v8::Local obj) { // Todo: non-SassValue objects could easily fall under that condition, need to be more specific. if (!obj->IsObject() || obj.As()->InternalFieldCount() != 1) { - throw std::invalid_argument("A SassValue object was expected."); + return NULL; } return static_cast(Nan::GetInternalFieldPointer(obj.As(), 0)); diff --git a/src/sass_types/list.cpp b/src/sass_types/list.cpp index 2e019ffd2..a337d638e 100644 --- a/src/sass_types/list.cpp +++ b/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(info[0]).FromJust(), sass_value->get_sass_value()); + if (sass_value) { + sass_list_set_value(unwrap(info.This())->value, Nan::To(info[0]).FromJust(), sass_value->get_sass_value()); + } else { + Nan::ThrowTypeError(Nan::New("A SassValue is expected as the list item").ToLocalChecked()); + } } NAN_METHOD(List::GetSeparator) { diff --git a/src/sass_types/map.cpp b/src/sass_types/map.cpp index 3bd41a9d0..147de42f4 100644 --- a/src/sass_types/map.cpp +++ b/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(info[0]).FromJust(), sass_value->get_sass_value()); + if (sass_value) { + sass_map_set_value(unwrap(info.This())->value, Nan::To(info[0]).FromJust(), sass_value->get_sass_value()); + } else { + Nan::ThrowTypeError(Nan::New("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(info[0]).FromJust(), sass_value->get_sass_value()); + if (sass_value) { + sass_map_set_key(unwrap(info.This())->value, Nan::To(info[0]).FromJust(), sass_value->get_sass_value()); + } else { + Nan::ThrowTypeError(Nan::New("A SassValue is expected as a map key").ToLocalChecked()); + } } NAN_METHOD(Map::GetLength) { diff --git a/src/sass_types/sass_value_wrapper.h b/src/sass_types/sass_value_wrapper.h index ca2856720..54eb16a02 100644 --- a/src/sass_types/sass_value_wrapper.h +++ b/src/sass_types/sass_value_wrapper.h @@ -118,6 +118,7 @@ namespace SassTypes template T* SassValueWrapper::unwrap(v8::Local obj) { + /* This maybe NULL */ return static_cast(Factory::unwrap(obj)); } diff --git a/test/api.js b/test/api.js index 5e733fe48..0796444da 100644 --- a/test/api.js +++ b/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;