From 48c744f9014da38c9c997b12bb7e83f8f679893c Mon Sep 17 00:00:00 2001 From: Ratchanan Srirattanamet Date: Fri, 18 Jun 2021 04:12:39 +0700 Subject: [PATCH 1/3] tests/function_call: add a failing test for transfer-full args --- tests/function_call.js | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/function_call.js b/tests/function_call.js index c2b4704..a476689 100644 --- a/tests/function_call.js +++ b/tests/function_call.js @@ -4,9 +4,11 @@ const gi = require('../lib/') const GLib = gi.require('GLib') +const Gst = gi.require('Gst', '1.0') const Gtk = gi.require('Gtk', '3.0') const { describe, it, mustThrow, expect } = require('./__common__.js') +Gst.init() Gtk.init() describe('function arguments', () => { @@ -24,3 +26,28 @@ describe('function call can throw', const result = GLib.filenameFromUri('http://google.com') console.log('Result:', result) })) + +describe('transfer-full function argument', () => { + it('does not invalidate js wrapper -- boxed', () => { + let gstPromise = new Gst.Promise(); + let structure = Gst.Structure.newEmpty('test'); + + /* + * https://gstreamer.freedesktop.org/documentation/gstreamer/gstpromise.html?gi-language=c#gst_promise_reply + * + * Parameters: + * <...> + * s ( [transfer: full][nullable]) – a GstStructure with the the reply contents + */ + gstPromise.reply(structure); + + // GstPromise will take the structure with it. + gstPromise = null; + // Required, to actually take gstPromise down. + global.gc(); + + // Gst will issue an assertion and return false if structure's + // backing object is destroyed. + expect(structure.isEqual(structure), true); + }); +}) From 77a0fb3c284807d8b394fedf292b7ea247171cba Mon Sep 17 00:00:00 2001 From: Ratchanan Srirattanamet Date: Mon, 19 Jul 2021 01:11:33 +0700 Subject: [PATCH 2/3] fix(value): handle transfer-full, in arguments properly This is done by making V8ToGIArgument() accept the transfer status and react accordingly. I does not make the new argument optional; this forces me into thinking about the appropriate value to put in for various callsites of this function. The make the test added in the previous commit passes. --- src/callback.cc | 7 ++++- src/function.cc | 5 ++-- src/gi.cc | 2 +- src/value.cc | 74 ++++++++++++++++++++++++++++++++++++------------- src/value.h | 4 +-- 5 files changed, 67 insertions(+), 25 deletions(-) diff --git a/src/callback.cc b/src/callback.cc index 7dd55b1..38d4079 100644 --- a/src/callback.cc +++ b/src/callback.cc @@ -192,11 +192,16 @@ void Callback::Execute (GIArgument *result, GIArgument **args, Callback *callbac } } + GITypeInfo ret_info; + g_callable_info_load_return_type(callback->info, &ret_info); + GITransfer transfer = g_arg_info_get_ownership_transfer (&ret_info); + success = V8ToGIArgument( &return_type_info, result, jsRealReturnValue, - g_callable_info_may_return_null (callback->info)); + g_callable_info_may_return_null (callback->info), + transfer); if (!success) { Throw::InvalidReturnValue (&return_type_info, jsReturnValue); diff --git a/src/function.cc b/src/function.cc index 00affdd..e429503 100644 --- a/src/function.cc +++ b/src/function.cc @@ -31,7 +31,8 @@ static void FillArgument(GIArgInfo *arg_info, GIArgument *argument, Local GITypeInfo type_info; bool may_be_null = g_arg_info_may_be_null (arg_info); g_arg_info_load_type (arg_info, &type_info); - V8ToGIArgument(&type_info, argument, value, may_be_null); + GITransfer transfer = g_arg_info_get_ownership_transfer(arg_info); + V8ToGIArgument(&type_info, argument, value, may_be_null, transfer); } static int GetV8ArrayLength (Local value) { @@ -331,7 +332,7 @@ Local FunctionCall ( if (func->is_method) { GIBaseInfo *container = g_base_info_get_container (gi_info); - V8ToGIArgument(container, &total_arg_values[0], info.This()); + V8ToGIArgument(container, &total_arg_values[0], info.This(), GI_TRANSFER_NOTHING); callable_arg_values = &total_arg_values[1]; } else { callable_arg_values = &total_arg_values[0]; diff --git a/src/gi.cc b/src/gi.cc index 4523244..884e139 100644 --- a/src/gi.cc +++ b/src/gi.cc @@ -223,7 +223,7 @@ NAN_METHOD(StructFieldSetter) { GIArgument arg; - if (!GNodeJS::V8ToGIArgument(field_type, &arg, value, true)) { + if (!GNodeJS::V8ToGIArgument(field_type, &arg, value, true, GI_TRANSFER_NOTHING)) { char *message = g_strdup_printf("Couldn't convert value for field '%s'", g_base_info_get_name(field)); Nan::ThrowTypeError (message); diff --git a/src/value.cc b/src/value.cc index 826bb05..a8d6d69 100644 --- a/src/value.cc +++ b/src/value.cc @@ -354,7 +354,7 @@ long GIArgumentToLength(GITypeInfo *type_info, GIArgument *arg, bool is_pointer) } -GArray * V8ToGArray(GITypeInfo *type_info, Local value) { +GArray * V8ToGArray(GITypeInfo *type_info, Local value, GITransfer transfer) { GArray* g_array = NULL; bool zero_terminated = g_type_info_is_zero_terminated(type_info); @@ -382,8 +382,10 @@ GArray * V8ToGArray(GITypeInfo *type_info, Local value) { for (int i = 0; i < length; i++) { auto value = Nan::Get(array, i).ToLocalChecked(); GIArgument arg; + GITransfer inner_transfer = + (transfer == GI_TRANSFER_CONTAINER ? GI_TRANSFER_NOTHING : transfer); - if (V8ToGIArgument(element_info, &arg, value, true)) { + if (V8ToGIArgument(element_info, &arg, value, true, inner_transfer)) { g_array_append_val (g_array, arg); } else { g_warning("V8ToGArray: couldnt convert value: %s", @@ -399,7 +401,7 @@ GArray * V8ToGArray(GITypeInfo *type_info, Local value) { return g_array; } -static void *V8ArrayToCArray(GITypeInfo *type_info, Local value) { +static void *V8ArrayToCArray(GITypeInfo *type_info, Local value, GITransfer transfer) { auto array = Local::Cast (TO_OBJECT (value)); int length = array->Length(); @@ -413,8 +415,10 @@ static void *V8ArrayToCArray(GITypeInfo *type_info, Local value) { auto value = Nan::Get(array, i).ToLocalChecked(); GIArgument arg; + GITransfer inner_transfer = + (transfer == GI_TRANSFER_CONTAINER ? GI_TRANSFER_NOTHING : transfer); - if (V8ToGIArgument(element_info, &arg, value, true)) { + if (V8ToGIArgument(element_info, &arg, value, true, inner_transfer)) { void* pointer = (void*)((ulong)result + i * element_size); memcpy(pointer, &arg, element_size); } else { @@ -453,7 +457,7 @@ static void *V8TypedArrayToCArray(GITypeInfo *type_info, Local value) { return result; } -void *V8ToCArray(GITypeInfo *type_info, Local value) { +void *V8ToCArray(GITypeInfo *type_info, Local value, GITransfer transfer) { if (value->IsString()) { Local string = TO_STRING (value); const char *utf8_data = *Nan::Utf8String(string); @@ -461,7 +465,7 @@ void *V8ToCArray(GITypeInfo *type_info, Local value) { } if (value->IsArray()) { - return V8ArrayToCArray(type_info, value); + return V8ArrayToCArray(type_info, value, transfer); } if (value->IsTypedArray()) { @@ -473,7 +477,7 @@ void *V8ToCArray(GITypeInfo *type_info, Local value) { return NULL; } -gpointer V8ToGList (GITypeInfo *type_info, Local value) { +gpointer V8ToGList (GITypeInfo *type_info, Local value, GITransfer transfer) { // FIXME can @value be null? if (!value->IsArray()) { @@ -504,7 +508,10 @@ gpointer V8ToGList (GITypeInfo *type_info, Local value) { GIArgument arg; Local value = Nan::Get(array, i).ToLocalChecked(); - if (!V8ToGIArgument(element_info, &arg, value, false)) { + GITransfer inner_transfer = + (transfer == GI_TRANSFER_CONTAINER ? GI_TRANSFER_NOTHING : transfer); + + if (!V8ToGIArgument(element_info, &arg, value, false, inner_transfer)) { g_warning("V8ToGList: couldnt convert value #%i to GIArgument", i); continue; } @@ -526,7 +533,7 @@ gpointer V8ToGList (GITypeInfo *type_info, Local value) { return list; } -gpointer V8ToGHash (GITypeInfo *type_info, Local value) { +gpointer V8ToGHash (GITypeInfo *type_info, Local value, GITransfer transfer) { if (!value->IsObject()) { Nan::ThrowTypeError("Expected object"); @@ -587,6 +594,9 @@ gpointer V8ToGHash (GITypeInfo *type_info, Local value) { auto object = TO_OBJECT (value); auto keys = Nan::GetOwnPropertyNames(object).ToLocalChecked(); + GITransfer inner_transfer = + (transfer == GI_TRANSFER_CONTAINER ? GI_TRANSFER_NOTHING : transfer); + for (uint32_t i = 0; i < keys->Length(); i++) { auto key = Nan::Get(keys, i).ToLocalChecked(); auto value = Nan::Get(object, key).ToLocalChecked(); @@ -594,14 +604,14 @@ gpointer V8ToGHash (GITypeInfo *type_info, Local value) { GIArgument key_arg; GIArgument value_arg; - if (!V8ToGIArgument(key_type_info, &key_arg, key, false)) { + if (!V8ToGIArgument(key_type_info, &key_arg, key, false, inner_transfer)) { char* message = g_strdup_printf("Couldn't convert key '%s'", *Nan::Utf8String(key)); Nan::ThrowError(message); free(message); goto item_error; } - if (!V8ToGIArgument(value_type_info, &value_arg, value, false)) { + if (!V8ToGIArgument(value_type_info, &value_arg, value, false, inner_transfer)) { char* message = g_strdup_printf("Couldn't convert value for key '%s'", *Nan::Utf8String(key)); Nan::ThrowError(message); free(message); @@ -613,7 +623,11 @@ gpointer V8ToGHash (GITypeInfo *type_info, Local value) { continue; item_error: - /* Free everything we have converted so far. */ + /* + * Free everything we have converted so far. + * Uses TRANSFER_NOTHING, DIRECTION_IN, as if the function doesn't take + * ownership, instead of passed in `transfer`. + */ FreeGIArgument(type_info, (GIArgument *) &hash_table, GI_TRANSFER_NOTHING, GI_DIRECTION_IN); hash_table = NULL; break; @@ -625,7 +639,7 @@ gpointer V8ToGHash (GITypeInfo *type_info, Local value) { return hash_table; } -bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local value) { +bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local value, GITransfer transfer) { GIInfoType type = g_base_info_get_type (gi_info); switch (type) { @@ -633,6 +647,22 @@ bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local value) { case GI_INFO_TYPE_STRUCT: case GI_INFO_TYPE_UNION: arg->v_pointer = PointerFromWrapper(value); + if (transfer == GI_TRANSFER_EVERYTHING) { + GType gtype = g_registered_type_info_get_g_type (gi_info); + size_t size; + + if (gtype != G_TYPE_NONE) { + arg->v_pointer = g_boxed_copy (gtype, arg->v_pointer); + } else if ((size = Boxed::GetSize(gi_info)) != 0) { + void *boxedCopy = malloc(size); + memcpy(boxedCopy, arg->v_pointer, size); + arg->v_pointer = boxedCopy; + } else { + // XXX what else can we do? Should we just abort? + WARN("Cannot copy a boxed object of type '%s'; memory corruption might occur.", + g_base_info_get_name(gi_info)); + } + } break; case GI_INFO_TYPE_FLAGS: @@ -652,6 +682,8 @@ bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local value) { } case GI_INFO_TYPE_INTERFACE: arg->v_pointer = GObjectFromWrapper(value); + if (transfer == GI_TRANSFER_EVERYTHING) + g_object_ref(arg->v_pointer); break; case GI_INFO_TYPE_CALLBACK: @@ -662,7 +694,7 @@ bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local value) { return true; } -bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local value, bool may_be_null) { +bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local value, bool may_be_null, GITransfer transfer) { GITypeTag type_tag = g_type_info_get_tag (type_info); if (value->IsUndefined () || value->IsNull ()) { @@ -680,6 +712,10 @@ bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local value, case GI_TYPE_TAG_VOID: if (g_type_info_is_pointer(type_info)) { arg->v_pointer = PointerFromWrapper(value); + if (transfer == GI_TRANSFER_EVERYTHING) { + ERROR("Case should not be reached. Please report this to " + "https://github.com/romgrk/node-gtk/issues"); + } break; } arg->v_pointer = NULL; @@ -744,11 +780,11 @@ bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local value, switch (array_type) { case GI_ARRAY_TYPE_C: - arg->v_pointer = V8ToCArray(type_info, value); + arg->v_pointer = V8ToCArray(type_info, value, transfer); break; case GI_ARRAY_TYPE_ARRAY: case GI_ARRAY_TYPE_BYTE_ARRAY: - arg->v_pointer = V8ToGArray(type_info, value); + arg->v_pointer = V8ToGArray(type_info, value, transfer); break; case GI_ARRAY_TYPE_PTR_ARRAY: default: @@ -761,7 +797,7 @@ bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local value, case GI_TYPE_TAG_INTERFACE: { GIBaseInfo *interface_info = g_type_info_get_interface (type_info); - V8ToGIArgument (interface_info, arg, value); + V8ToGIArgument (interface_info, arg, value, transfer); g_base_info_unref(interface_info); } break; @@ -769,13 +805,13 @@ bool V8ToGIArgument(GITypeInfo *type_info, GIArgument *arg, Local value, case GI_TYPE_TAG_GLIST: case GI_TYPE_TAG_GSLIST: { - arg->v_pointer = V8ToGList(type_info, value); + arg->v_pointer = V8ToGList(type_info, value, transfer); } break; case GI_TYPE_TAG_GHASH: { - arg->v_pointer = V8ToGHash(type_info, value); + arg->v_pointer = V8ToGHash(type_info, value, transfer); } break; diff --git a/src/value.h b/src/value.h index a23ea6f..efd1a51 100644 --- a/src/value.h +++ b/src/value.h @@ -19,8 +19,8 @@ Local GErrorToV8 (GITypeInfo *type_info, GError *err); Local GIArgumentToV8 (GITypeInfo *type_info, GIArgument *argument, long length = -1, bool mustCopy = false); long GIArgumentToLength(GITypeInfo *type_info, GIArgument *arg, bool is_pointer); -bool V8ToGIArgument (GITypeInfo *type_info, GIArgument *argument, Local value); -bool V8ToGIArgument (GITypeInfo *type_info, GIArgument *argument, Local value, bool may_be_null); +bool V8ToGIArgument (GITypeInfo *type_info, GIArgument *argument, Local value, GITransfer transfer); +bool V8ToGIArgument (GITypeInfo *type_info, GIArgument *argument, Local value, bool may_be_null, GITransfer transfer); bool V8ToOutGIArgument(GITypeInfo *type_info, GIArgument *arg, Local value, bool may_be_null); void FreeGIArgument (GITypeInfo *type_info, GIArgument *argument, GITransfer transfer = GI_TRANSFER_EVERYTHING, GIDirection direction = GI_DIRECTION_OUT); void FreeGIArgumentArray (GITypeInfo *type_info, GIArgument *arg, GITransfer transfer = GI_TRANSFER_EVERYTHING, GIDirection direction = GI_DIRECTION_OUT, long length = -1); From cdb1bcbfbfe14e0899c0f6bcae998f54cd5822aa Mon Sep 17 00:00:00 2001 From: Ratchanan Srirattanamet Date: Sun, 29 Aug 2021 00:45:34 +0700 Subject: [PATCH 3/3] lint: de-duplicate boxed value copying logic --- src/boxed.cc | 32 ++++++++++++++++++++++---------- src/boxed.h | 1 + src/value.cc | 17 +++-------------- 3 files changed, 26 insertions(+), 24 deletions(-) diff --git a/src/boxed.cc b/src/boxed.cc index e03b288..deb7af8 100644 --- a/src/boxed.cc +++ b/src/boxed.cc @@ -179,17 +179,9 @@ static void BoxedConstructor(const Nan::FunctionCallbackInfo &info) { boxed = External::Cast(*info[0])->Value(); if (mustCopy) { - if (gtype != G_TYPE_NONE) { - boxed = g_boxed_copy (gtype, boxed); - } - else if ((size = Boxed::GetSize(gi_info)) != 0) { - void *boxedCopy = malloc(size); - memcpy(boxedCopy, boxed, size); - boxed = boxedCopy; - } - else { + boxed = CopyBoxed(gi_info, boxed, &size); + if (!boxed) ERROR("Could not copy boxed object."); - } } else { owns_memory = false; @@ -445,4 +437,24 @@ void* PointerFromWrapper(Local value) { return boxed; } +void* CopyBoxed(GIBaseInfo *gi_info, void *boxed, size_t *size_out) +{ + GType gtype = g_registered_type_info_get_g_type (gi_info); + size_t size; + + if (gtype != G_TYPE_NONE) { + return g_boxed_copy (gtype, boxed); + } else if ((size = Boxed::GetSize(gi_info)) != 0) { + void *boxedCopy = malloc(size); + memcpy(boxedCopy, boxed, size); + if (size_out) + *size_out = size; + return boxedCopy; + } else { + WARN("Cannot copy a boxed object of type '%s'.", + g_base_info_get_name(gi_info)); + return NULL; + } +} + }; diff --git a/src/boxed.h b/src/boxed.h index 7fd83a0..c09abbb 100644 --- a/src/boxed.h +++ b/src/boxed.h @@ -32,5 +32,6 @@ Local MakeBoxedClass (GIBaseInfo *info); Local GetBoxedTemplate (GIBaseInfo *info, GType gtype); Local WrapperFromBoxed (GIBaseInfo *info, void *data, bool mustCopy = false); void * PointerFromWrapper (Local); +void * CopyBoxed (GIBaseInfo *gi_info, void *boxed, size_t *size_out = nullptr); }; diff --git a/src/value.cc b/src/value.cc index a8d6d69..3d1881c 100644 --- a/src/value.cc +++ b/src/value.cc @@ -648,20 +648,9 @@ bool V8ToGIArgument(GIBaseInfo *gi_info, GIArgument *arg, Local value, GI case GI_INFO_TYPE_UNION: arg->v_pointer = PointerFromWrapper(value); if (transfer == GI_TRANSFER_EVERYTHING) { - GType gtype = g_registered_type_info_get_g_type (gi_info); - size_t size; - - if (gtype != G_TYPE_NONE) { - arg->v_pointer = g_boxed_copy (gtype, arg->v_pointer); - } else if ((size = Boxed::GetSize(gi_info)) != 0) { - void *boxedCopy = malloc(size); - memcpy(boxedCopy, arg->v_pointer, size); - arg->v_pointer = boxedCopy; - } else { - // XXX what else can we do? Should we just abort? - WARN("Cannot copy a boxed object of type '%s'; memory corruption might occur.", - g_base_info_get_name(gi_info)); - } + arg->v_pointer = CopyBoxed(gi_info, arg->v_pointer); + if (!arg->v_pointer) + return false; } break;