From 0346a9d871799bb4631d64eeecdd1f89c3e86f5e Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Sun, 6 Sep 2015 20:34:18 +0300 Subject: [PATCH 1/4] Change bufferobject push typing, add wrap checks --- src/duk_api_public.h.in | 2 +- src/duk_api_stack.c | 41 ++++++++++++++++++++++++++++++++++------- src/duk_hbufferobject.h | 3 ++- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/duk_api_public.h.in b/src/duk_api_public.h.in index b9c2859d..622da94c 100644 --- a/src/duk_api_public.h.in +++ b/src/duk_api_public.h.in @@ -443,7 +443,7 @@ DUK_EXTERNAL_DECL void *duk_push_buffer_raw(duk_context *ctx, duk_size_t size, d #define DUK_BUFOBJ_FLOAT32ARRAY (11 | DUK_BUFOBJ_CREATE_ARRBUF) #define DUK_BUFOBJ_FLOAT64ARRAY (12 | DUK_BUFOBJ_CREATE_ARRBUF) -DUK_EXTERNAL_DECL void duk_push_buffer_object(duk_context *ctx, duk_idx_t idx_buffer, duk_uint_t byte_offset, duk_uint_t byte_length, duk_uint_t flags); +DUK_EXTERNAL_DECL void duk_push_buffer_object(duk_context *ctx, duk_idx_t idx_buffer, duk_size_t byte_offset, duk_size_t byte_length, duk_uint_t flags); DUK_EXTERNAL_DECL duk_idx_t duk_push_heapptr(duk_context *ctx, void *ptr); diff --git a/src/duk_api_stack.c b/src/duk_api_stack.c index fa19b74b..030ba426 100644 --- a/src/duk_api_stack.c +++ b/src/duk_api_stack.c @@ -3858,7 +3858,7 @@ static const duk_uint32_t duk__bufobj_flags_lookup[] = { }; #undef DUK__PACK_ARGS -DUK_EXTERNAL void duk_push_buffer_object(duk_context *ctx, duk_idx_t idx_buffer, duk_uint_t byte_offset, duk_uint_t byte_length, duk_uint_t flags) { +DUK_EXTERNAL void duk_push_buffer_object(duk_context *ctx, duk_idx_t idx_buffer, duk_size_t byte_offset, duk_size_t byte_length, duk_uint_t flags) { duk_hthread *thr; duk_hbufferobject *h_bufobj; duk_hbuffer *h_val; @@ -3866,16 +3866,33 @@ DUK_EXTERNAL void duk_push_buffer_object(duk_context *ctx, duk_idx_t idx_buffer, duk_uint_t classnum; duk_uint_t protobidx; duk_uint_t lookupidx; + duk_uint_t uint_offset, uint_length, uint_added; DUK_ASSERT_CTX_VALID(ctx); thr = (duk_hthread *) ctx; DUK_UNREF(thr); + /* The underlying types for offset/length in duk_hbufferobject is + * duk_uint_t; make sure argument values fit and that offset + length + * does not wrap. + */ + uint_offset = (duk_uint_t) byte_offset; + uint_length = (duk_uint_t) byte_length; + if (sizeof(duk_size_t) != sizeof(duk_uint_t)) { + if ((duk_size_t) uint_offset != byte_offset || (duk_size_t) uint_length != byte_length) { + goto range_error; + } + } + uint_added = uint_offset + uint_length; + if (uint_added < uint_offset) { + goto range_error; + } + DUK_ASSERT(uint_added >= uint_offset && uint_added >= uint_length); + DUK_ASSERT_DISABLE(flags >= 0); /* flags is unsigned */ lookupidx = flags & 0x0f; /* 4 low bits */ if (lookupidx >= sizeof(duk__bufobj_flags_lookup) / sizeof(duk_uint32_t)) { - DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_CALL_ARGS); - return; /* not reached */ + goto arg_error; } tmp = duk__bufobj_flags_lookup[lookupidx]; classnum = tmp >> 24; @@ -3893,11 +3910,12 @@ DUK_EXTERNAL void duk_push_buffer_object(duk_context *ctx, duk_idx_t idx_buffer, h_bufobj->buf = h_val; DUK_HBUFFER_INCREF(thr, h_val); - h_bufobj->offset = byte_offset; - h_bufobj->length = byte_length; + h_bufobj->offset = uint_offset; + h_bufobj->length = uint_length; h_bufobj->shift = (tmp >> 4) & 0x0f; h_bufobj->elem_type = (tmp >> 8) & 0xff; h_bufobj->is_view = tmp & 0x0f; + DUK_ASSERT_HBUFFEROBJECT_VALID(h_bufobj); /* TypedArray views need an automatic ArrayBuffer which must be * provided as .buffer property of the view. Just create a new @@ -3914,16 +3932,25 @@ DUK_EXTERNAL void duk_push_buffer_object(duk_context *ctx, duk_idx_t idx_buffer, h_bufobj->buf = h_val; DUK_HBUFFER_INCREF(thr, h_val); - h_bufobj->offset = byte_offset; - h_bufobj->length = byte_length; + h_bufobj->offset = uint_offset; + h_bufobj->length = uint_length; DUK_ASSERT(h_bufobj->shift == 0); h_bufobj->elem_type = DUK_HBUFFEROBJECT_ELEM_UINT8; DUK_ASSERT(h_bufobj->is_view == 0); + DUK_ASSERT_HBUFFEROBJECT_VALID(h_bufobj); duk_xdef_prop_stridx(ctx, -2, DUK_STRIDX_LC_BUFFER, DUK_PROPDESC_FLAGS_NONE); duk_compact(ctx, -1); } + return; + + range_error: + DUK_ERROR(thr, DUK_ERR_RANGE_ERROR, DUK_STR_INVALID_CALL_ARGS); + return; /* not reached */ + arg_error: + DUK_ERROR(thr, DUK_ERR_TYPE_ERROR, DUK_STR_INVALID_CALL_ARGS); + return; /* not reached */ } DUK_EXTERNAL duk_idx_t duk_push_error_object_va_raw(duk_context *ctx, duk_errcode_t err_code, const char *filename, duk_int_t line, const char *fmt, va_list ap) { diff --git a/src/duk_hbufferobject.h b/src/duk_hbufferobject.h index ab341576..a2adc67f 100644 --- a/src/duk_hbufferobject.h +++ b/src/duk_hbufferobject.h @@ -38,8 +38,9 @@ } else { \ /* No assertions for offset or length; in particular, \ * it's OK for length to be longer than underlying \ - * buffer. \ + * buffer. Just ensure they don't wrap when added. \ */ \ + DUK_ASSERT((h)->offset + (h)->length >= (h)->offset); \ } \ } while (0) From c4141cd331f5a2f8fefb329ac14dae261289be50 Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Sun, 6 Sep 2015 20:34:24 +0300 Subject: [PATCH 2/4] Bufferobject push wrap API tests --- tests/api/test-push-buffer-object.c | 70 ++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/tests/api/test-push-buffer-object.c b/tests/api/test-push-buffer-object.c index a7d9b48d..1321c64b 100644 --- a/tests/api/test-push-buffer-object.c +++ b/tests/api/test-push-buffer-object.c @@ -398,6 +398,68 @@ static duk_ret_t test_invalid_flags3(duk_context *ctx) { return 0; } +/*=== +*** test_invalid_offlen_wrap1 (duk_safe_call) +==> rc=1, result='RangeError: invalid call args' +*** test_invalid_offlen_wrap2 (duk_safe_call) +==> rc=1, result='RangeError: invalid call args' +===*/ + +/* Byte offset + byte length wrap. */ +static duk_ret_t test_invalid_offlen_wrap1(duk_context *ctx) { + duk_push_fixed_buffer(ctx, 256); + duk_push_buffer_object(ctx, + -1, + (duk_size_t) (duk_uint_t) -100, + 1000, + DUK_BUFOBJ_UINT8ARRAY); + printf("final top: %ld\n", (long) duk_get_top(ctx)); + return 0; +} + +/* Byte offset + byte length wrap, just barely */ +static duk_ret_t test_invalid_offlen_wrap2(duk_context *ctx) { + duk_push_fixed_buffer(ctx, 256); + duk_push_buffer_object(ctx, + -1, + (duk_size_t) (duk_uint_t) -100, + 100, + DUK_BUFOBJ_UINT8ARRAY); + printf("final top: %ld\n", (long) duk_get_top(ctx)); + return 0; +} + +/*=== +*** test_allowed_offlen_nowrap1 (duk_safe_call) +object [object Uint8Array] 99 4294967196 99 1 object +false false false false false true false false false false false false false -> Uint8Array +false false false false false true false false false false false false false -> Uint8Array.prototype +final top: 1 +==> rc=0, result='undefined' +===*/ + +/* Byte offset + byte length are just within limits of duk_uint_t and don't + * wrap. This works and doesn't cause a ~4G allocation because the conceptual + * size (~4G) is unrelated to the underlying buffer size (256 bytes here). + */ +static duk_ret_t test_allowed_offlen_nowrap1(duk_context *ctx) { + duk_push_fixed_buffer(ctx, 256); + duk_push_buffer_object(ctx, + -1, + (duk_size_t) (duk_uint_t) -100, + 99, + DUK_BUFOBJ_UINT8ARRAY); + duk_eval_string(ctx, "dumpBufferInfo"); + duk_dup(ctx, -2); + duk_call(ctx, 1); + duk_pop(ctx); + + duk_pop(ctx); + + printf("final top: %ld\n", (long) duk_get_top(ctx)); + return 0; +} + /* * Main */ @@ -418,5 +480,11 @@ void test(duk_context *ctx) { TEST_SAFE_CALL(test_invalid_flags1); TEST_SAFE_CALL(test_invalid_flags2); TEST_SAFE_CALL(test_invalid_flags3); - /* XXX: could test for more errors */ + TEST_SAFE_CALL(test_invalid_offlen_wrap1); + TEST_SAFE_CALL(test_invalid_offlen_wrap2); + TEST_SAFE_CALL(test_allowed_offlen_nowrap1); + + /* XXX: testing for duk_size_t wrapping a duk_uint_t is only possible + * if duk_size_t is a larger type. + */ } From b7c017852f5f6c0a3a175df937ddacc6c816e110 Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Sun, 6 Sep 2015 20:34:31 +0300 Subject: [PATCH 3/4] Bufferobject push API doc typing fix --- website/api/duk_push_buffer_object.yaml | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/website/api/duk_push_buffer_object.yaml b/website/api/duk_push_buffer_object.yaml index 9f3f83ca..9c3a525b 100644 --- a/website/api/duk_push_buffer_object.yaml +++ b/website/api/duk_push_buffer_object.yaml @@ -1,7 +1,7 @@ name: duk_push_buffer proto: | - void duk_push_buffer_object(duk_context *ctx, duk_idx_t idx_buffer, duk_uint_t byte_offset, duk_uint_t byte_length, duk_uint_t flags); + void duk_push_buffer_object(duk_context *ctx, duk_idx_t idx_buffer, duk_size_t byte_offset, duk_size_t byte_length, duk_uint_t flags); stack: | [ ... buffer! ... ] -> [ ... buffer! ... bufobj! ] @@ -31,13 +31,18 @@ summary: | DUK_BUFOBJ_FLOAT64ARRAYFloat64Array -

The underlying plain buffer should cover the range indicated by the - byte_offset and byte_length arguments. Even - if that is not the case, memory safety is guaranteed; e.g. attempt to - read values outside the underlying buffer will return zero. The underlying - buffer size is intentionally not checked when creating a buffer object: - even if the buffer fully covered the byte range during creation, it might - be resized later.

+

When DataView or any of the typed array views + (Uint8Array, etc) is pushed, an ArrayBuffer + backing the view is automatically created. It is accessible through + the buffer property of the view object.

+ +

The underlying plain buffer should normally cover the range indicated by + the byte_offset and byte_length arguments, but + memory safety is guaranteed even if that is not the case. For example, an + attempt to read values outside the underlying buffer will return zero. The + underlying buffer size is intentionally not checked when creating a buffer + object: even if the buffer fully covered the byte range during creation, it + might be resized later.

example: | /* Map byte range [100,150[ of plain buffer at idx_plain_buf into a From 7823ffaf5b5d0a2a881beee59320abe359f0b148 Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Mon, 7 Sep 2015 00:16:09 +0300 Subject: [PATCH 4/4] Fix warning for packed duk_tval build --- src/duk_hbuffer_alloc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/duk_hbuffer_alloc.c b/src/duk_hbuffer_alloc.c index 2e98704c..3a8eb918 100644 --- a/src/duk_hbuffer_alloc.c +++ b/src/duk_hbuffer_alloc.c @@ -54,7 +54,9 @@ DUK_INTERNAL duk_hbuffer *duk_hbuffer_alloc(duk_heap *heap, duk_size_t size, duk #endif if (flags & DUK_BUF_FLAG_EXTERNAL) { - duk_hbuffer_external *h = (duk_hbuffer_external *) res; + duk_hbuffer_external *h; + h = (duk_hbuffer_external *) res; + DUK_UNREF(h); *out_bufdata = NULL; #if defined(DUK_USE_EXPLICIT_NULL_INIT) #if defined(DUK_USE_HEAPPTR16)