Merge pull request #417 from svaarala/not-callable-error-improvement

More verbose error message for "not callable"

Makefile

@@ -196,7 +196,7 @@ CCOPTS_FEATURES += -DDUK_OPT_SELF_TESTS
 #CCOPTS_FEATURES += -DDUK_OPT_NO_TRACEBACKS
 #CCOPTS_FEATURES += -DDUK_OPT_NO_PC2LINE
 #CCOPTS_FEATURES += -DDUK_OPT_NO_VERBOSE_ERRORS
-#CCOPTS_FEATURES += -DDUK_OPT_NO_VERBOSE_PROP_ERRORS
+#CCOPTS_FEATURES += -DDUK_OPT_PARANOID_ERRORS
 #CCOPTS_FEATURES += -DDUK_OPT_NO_AUGMENT_ERRORS
 #CCOPTS_FEATURES += -DDUK_OPT_GC_TORTURE
 #CCOPTS_FEATURES += -DDUK_OPT_SHUFFLE_TORTURE

RELEASES.rst

@@ -1146,8 +1146,8 @@ Planned
   (GH-360)
 
 * Add a human readable summary of object/key for rejected property operations
-  to make error messages more useful for operations like "null.foo = 123;"
-  (GH-210, GH-405)
+  and function calls to make error messages more useful for expressions like
+  "null.foo = 123" and "null()" (GH-210, GH-405, GH-417)
 
 * Add a debugger Throw notify for errors about to be thrown, and an option
   to automatically pause before an uncaught error is thrown (GH-286, GH-347)

config/config-options/DUK_USE_PARANOID_ERRORS.yaml

@@ -0,0 +1,15 @@
+define: DUK_USE_PARANOID_ERRORS
+feature_enables: DUK_OPT_PARANOID_ERRORS
+introduced: 1.4.0
+default: false
+tags:
+  - ecmascript
+  - sandbox
+description: >
+  When enabled, error messages won't involve summarization of keys or values.
+  Summaries may be an issue in some security sensitive environments because
+  error messages will include e.g. property keys.
+
+  The default is to summarize offending base value and key for property access
+  errors such as "null.foo = 123;", invalid calls such as "undefined()", etc.
+  Base values and keys are summarized using duk_push_string_tval_readable().

config/config-options/DUK_USE_VERBOSE_PROP_ERRORS.yaml

@@ -1,14 +0,0 @@
-define: DUK_USE_VERBOSE_PROP_ERRORS
-feature_enables: DUK_OPT_VERBOSE_PROP_ERRORS
-introduced: 1.4.0
-default: true
-tags:
-  - ecmascript
-  - sandbox
-description: >
-  Summarize offending base value and key for property operation errors
-  such as "null.foo = 123;".  Base values and keys are summarized using
-  duk_push_string_tval_readable().
-
-  The key/value summary includes string data which may be an issue for some
-  security critical environments.  Disable this option in such environments.

config/examples/low_memory.yaml

@@ -16,7 +16,7 @@ DUK_USE_TRACEBACKS: false
 DUK_USE_ERRCREATE: false
 DUK_USE_ERRTHROW: false
 DUK_USE_VERBOSE_ERRORS: false
-DUK_USE_VERBOSE_PROP_ERRORS: false
+DUK_USE_PARANOID_ERRORS: true
 DUK_USE_DEBUGGER_SUPPORT: false  # must be disabled if DUK_USE_PC2LINE is disabled
 DUK_USE_PC2LINE: false
 DUK_USE_LEXER_SLIDING_WINDOW: false

config/examples/security_sensitive.yaml

@@ -1,8 +1,8 @@
 # Base configuration for security sensitive environments.
 
-# Disable summary of object/key for rejected property operations.  May be
+# Avoid summary of object/key for rejected property operations.  May be
 # relevant if keys contain potentially sensitive information.
-DUK_USE_VERBOSE_PROP_ERRORS: false
+DUK_USE_PARANOID_ERRORS: true
 
 # Disable tracebacks, minimizes attacker knowledge of call chains.  Access
 # to the internal error _Tracedata property provides access to all functions

doc/sandboxing.rst

@@ -84,9 +84,9 @@ Disable verbose errors
 
 Verbose error messages may cause sandboxing security issues:
 
-* When DUK_USE_VERBOSE_PROP_ERRORS is set, offending object/key is summarized
+* When DUK_USE_PARANOID_ERRORS is not set, offending object/key is summarized
   in an error message of some rejected property operations.  If object keys
-  contain potentially sensitive information, you should disable this option.
+  contain potentially sensitive information, you should enable this option.
 
 * When stack traces are enabled an attacker may gain useful information from
   the stack traces.  Further, access to the internal ``_Tracedata`` property

src/duk_hobject_props.c

@@ -2247,11 +2247,11 @@ DUK_INTERNAL duk_bool_t duk_hobject_getprop(duk_hthread *thr, duk_tval *tv_obj,
 	case DUK_TAG_NULL: {
 		/* Note: unconditional throw */
 		DUK_DDD(DUK_DDDPRINT("base object is undefined or null -> reject"));
-#if defined(DUK_USE_VERBOSE_PROP_ERRORS)
+#if defined(DUK_USE_PARANOID_ERRORS)
+		DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_BASE);
+#else
 		DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, "cannot read property %s of %s",
 		          duk_push_string_tval_readable(ctx, tv_key), duk_push_string_tval_readable(ctx, tv_obj));
-#else
-		DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_BASE);
 #endif
 		return 0;
 	}
@@ -3287,11 +3287,11 @@ DUK_INTERNAL duk_bool_t duk_hobject_putprop(duk_hthread *thr, duk_tval *tv_obj,
 		/* Note: unconditional throw */
 		DUK_DDD(DUK_DDDPRINT("base object is undefined or null -> reject (object=%!iT)",
 		                     (duk_tval *) tv_obj));
-#if defined(DUK_USE_VERBOSE_PROP_ERRORS)
+#if defined(DUK_USE_PARANOID_ERRORS)
+		DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_BASE);
+#else
 		DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, "cannot write property %s of %s",
 		          duk_push_string_tval_readable(ctx, tv_key), duk_push_string_tval_readable(ctx, tv_obj));
-#else
-		DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_BASE);
 #endif
 		return 0;
 	}
@@ -4049,11 +4049,11 @@ DUK_INTERNAL duk_bool_t duk_hobject_putprop(duk_hthread *thr, duk_tval *tv_obj,
  fail_base_primitive:
 	DUK_DDD(DUK_DDDPRINT("result: error, base primitive"));
 	if (throw_flag) {
-#if defined(DUK_USE_VERBOSE_PROP_ERRORS)
+#if defined(DUK_USE_PARANOID_ERRORS)
+		DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_BASE);
+#else
 		DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, "cannot write property %s of %s",
 		          duk_push_string_tval_readable(ctx, tv_key), duk_push_string_tval_readable(ctx, tv_obj));
-#else
-		DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_BASE);
 #endif
 	}
 	duk_pop(ctx);  /* remove key */
@@ -4416,11 +4416,11 @@ DUK_INTERNAL duk_bool_t duk_hobject_delprop(duk_hthread *thr, duk_tval *tv_obj,
  fail_invalid_base_uncond:
 	/* Note: unconditional throw */
 	DUK_ASSERT(duk_get_top(ctx) == entry_top);
-#if defined(DUK_USE_VERBOSE_PROP_ERRORS)
+#if defined(DUK_USE_PARANOID_ERRORS)
+	DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_BASE);
+#else
 	DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, "cannot delete property %s of %s",
 	          duk_push_string_tval_readable(ctx, tv_key), duk_push_string_tval_readable(ctx, tv_obj));
-#else
-	DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_BASE);
 #endif
 	return 0;
 

src/duk_js_call.c

@@ -715,7 +715,12 @@ duk_hobject *duk__nonbound_func_lookup(duk_context *ctx,
 	return func;
 
  not_callable_error:
+	DUK_ASSERT(tv_func != NULL);
+#if defined(DUK_USE_PARANOID_ERRORS)
 	DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_NOT_CALLABLE);
+#else
+	DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, "%s not callable", duk_push_string_tval_readable(ctx, tv_func));
+#endif
 	DUK_UNREACHABLE();
 	return NULL;  /* never executed */
 }

tests/api/test-pcall-prop.c

@@ -24,7 +24,7 @@ rc=1, result='RangeError: getter error'
 rc=1, result='Error: invalid index'
 ==> rc=0, result='undefined'
 *** test_8 (duk_safe_call)
-rc=1, result='TypeError: not callable'
+rc=1, result='TypeError: undefined not callable'
 ==> rc=0, result='undefined'
 *** test_9 (duk_safe_call)
 ==> rc=1, result='Error: invalid call args'

tests/ecmascript/test-bi-duktape-errhandler.js

@@ -40,10 +40,10 @@ error: DoubleError: error in error handling, foo: undefined, bar: undefined
 error: ReferenceError: identifier 'zork' undefined, foo: undefined, bar: undefined
 - non-callable errCreate
 error: DoubleError: error in error handling, foo: undefined, bar: undefined
-error: TypeError: not callable, foo: undefined, bar: undefined
+error: TypeError: 123 not callable, foo: undefined, bar: undefined
 - "undefined" (but set) errCreate
 error: DoubleError: error in error handling, foo: undefined, bar: undefined
-error: TypeError: not callable, foo: undefined, bar: undefined
+error: TypeError: undefined not callable, foo: undefined, bar: undefined
 - delete errCreate property
 error: ReferenceError: identifier 'aiee' undefined, foo: undefined, bar: undefined
 error: URIError: fake uri error, foo: undefined, bar: undefined
@@ -314,10 +314,10 @@ error: DoubleError: error in error handling, foo: undefined, bar: undefined
 error: ReferenceError: identifier 'zork' undefined, foo: undefined, bar: undefined
 - non-callable errThrow
 error: DoubleError: error in error handling, foo: undefined, bar: undefined
-error: TypeError: not callable, foo: undefined, bar: undefined
+error: TypeError: 123 not callable, foo: undefined, bar: undefined
 - "undefined" (but set) errThrow
 error: DoubleError: error in error handling, foo: undefined, bar: undefined
-error: TypeError: not callable, foo: undefined, bar: undefined
+error: TypeError: undefined not callable, foo: undefined, bar: undefined
 - delete errThrow property
 error: ReferenceError: identifier 'aiee' undefined, foo: undefined, bar: undefined
 error: URIError: fake uri error, foo: undefined, bar: undefined

tests/ecmascript/test-dev-call-error-messages.js

@@ -0,0 +1,80 @@
+/*
+ *  Fragile testcase for testing call error messages.
+ */
+
+/*---
+{
+    "custom": true
+}
+---*/
+
+/*===
+TypeError: undefined not callable
+TypeError: 123 not callable
+TypeError: 321 not callable
+TypeError: undefined not callable
+TypeError: 234 not callable
+"TypeError: undefined not callable"
+"TypeError: null not callable"
+"TypeError: true not callable"
+"TypeError: false not callable"
+"TypeError: 123 not callable"
+"TypeError: 'a\u1234string' not callable"
+"TypeError: [object Array] not callable"
+"TypeError: [object Object] not callable"
+===*/
+
+var global = 321;
+
+function test() {
+    var tmp;
+    var dummy = 123;
+    var obj = { bar: 234 };
+
+    // Basic cases
+    try {
+        (undefined)();  // literal
+    } catch (e) {
+        print(e);
+    }
+    try {
+        dummy();  // register-mapped variable
+    } catch (e) {
+        print(e);
+    }
+    try {
+        global();  // global variable, slow path lookup
+    } catch (e) {
+        print(e);
+    }
+    try {
+        obj.foo();  // object property, nonexistent
+    } catch (e) {
+        print(e);
+    }
+    try {
+        obj.bar();  // object property, exists but not callable
+    } catch (e) {
+        print(e);
+    }
+
+    // Summarization of different value types is already covered by
+    // test-dev-prop-error-messages.js, but cover a few values here.
+
+    [
+        undefined, null, true, false, 123, 'a\u1234string', [ 1, 2, 3 ], { foo: 'bar' }
+    ].forEach(function (v) {
+        try {
+            v();
+        } catch (e) {
+            tmp = Duktape.enc('jx', String(e));  // JX encode to get ASCII
+            print(tmp);
+        }
+    });
+}
+
+try {
+    test();
+} catch (e) {
+    print(e.stack || e);
+}

util/matrix_compile.py

@@ -328,7 +328,7 @@ def create_matrix(fn_duk):
 		Select([ '', '-DDUK_OPT_FORCE_ALIGN=4', '-DDUK_OPT_FORCE_ALIGN=8' ]),
 		'-DDUK_OPT_NO_TRACEBACKS',
 		'-DDUK_OPT_NO_VERBOSE_ERRORS',
-		'-DDUK_OPT_NO_VERBOSE_PROP_ERRORS',
+		'-DDUK_OPT_PARANOID_ERRORS',
 		'-DDUK_OPT_NO_MS_RESIZE_STRINGTABLE',
 		'-DDUK_OPT_NO_STRICT_DECL',
 		'-DDUK_OPT_NO_REGEXP_SUPPORT',