GNOME Bugzilla – Bug 642708
Crash when C functions that has a C array parameter sets a GError.
Last modified: 2011-02-24 17:21:27 UTC
When passing in a Python list, it is converted to a GArray. If the array is supposed to be passed in as a C array (GI_ARRAY_TYPE_C), then the GIArgument->v_pointer is set to garray->data, and the outer GArray struct is freed. The once the C function has returned, a GArray is reconstructed using the GIArgument->v_pointer. Then we can continue treating it like a GArray, and don't need separate code paths for GI_ARRAY_TYPE_C. The issues arises because the first part (extracting the data pointer and freeing the struct) is done in _prepare_invocation_state(), whereas the second part (reconstructing the GArray) is done in _process_invocation_state(). If the C function sets a GError, then _invoke_function() sets the exception, and return FALSE, causing the invocation state (and its args) to be freed. _free_invocation_state() assumes that _process_invocation_state() has been called, and the GArray already reconstructed. However the v_pointer is still just the array data. Then _py_argument_release() gets garbage data from garray->len and thinks it has an array with millions of elements. Crash.
Two options come to mind: 1. Pass a value into _free_invocation_state() to let it know how far we got before failure (if any). 2. But in a extra step between _invoke_function() and _process_invocation_state(). Call it _unprepare_invocation_state(). Then make sure you call it before _free_invocation_state() *even* in the case of failure.
Created attachment 181650 [details] [review] [gi] Workaround a bug when freeing C array types * This is a hack and there is really no way around it without ripping out the current array handling code which spans between pygi-invoke.c and pygi-argument.c and completely rewriting it. * The is no time before our stable release * This patch trades a segfault for a leak in the very unusual case where an error occures inside an interface that takes one or more C arrays. Since we wrap C arrays in GArrays internally but have to unwrap them to send them to the introspected C function, there is a period of time where an error can occure with the C array in an unknown state (some being wrapped and others still internal to a GArray) * This patch adds a c_arrays_are_wrapped state to signal that it is safe to free them. However since c_arrays_are_wrapped can only track arrays as a group, not individually, if it is set to FALSE we can not assume that every array is still not wrapped, so instead we will simply leak them to avoid incorrectly freeing one of them and causing a segfault. * This issue is fixed in the invoke rewrite branch as it treats C arrays and G arrays seperately, however that branch is not yet ready to be merged and won't be until the next release.
Created attachment 181651 [details] [review] [gi] Workaround a bug when freeing C array types * This is a hack and there is really no way around it without ripping out the current array handling code which spans between pygi-invoke.c and pygi-argument.c and completely rewriting it. * The is no time before our stable release * This patch trades a segfault for a leak in the very unusual case where an error occures inside an interface that takes one or more C arrays. Since we wrap C arrays in GArrays internally but have to unwrap them to send them to the introspected C function, there is a period of time where an error can occure with the C array in an unknown state (some being true C arrays and others still wrapped in a GArray) * This patch adds a c_arrays_are_wrapped state to signal that it is safe to free them. However since c_arrays_are_wrapped can only track arrays as a group, not individually, if it is set to FALSE we can not assume that every array is a pure C array, so instead we will simply leak them to avoid incorrectly freeing one and causing a segfault. * This issue is fixed in the invoke rewrite branch as it treats C arrays and GArrays separately, however that branch is not yet ready to be merged and won't be until the next release.
Comment on attachment 181651 [details] [review] [gi] Workaround a bug when freeing C array types >From aee47f5507c7f312ed07123f7ed0a450bf6f3367 Mon Sep 17 00:00:00 2001 >From: John (J5) Palmieri <johnp@redhat.com> >Date: Tue, 22 Feb 2011 17:27:34 -0500 >Subject: [PATCH] [gi] Workaround a bug when freeing C array types >@@ -900,15 +933,27 @@ _free_invocation_state (struct invocation_state *state) > backup_args_pos += 1; > } > if (state->args != NULL && state->args[i] != NULL) { >- _pygi_argument_release (state->args[i], state->arg_type_infos[i], >- transfer, direction); >- > type_tag = g_type_info_get_tag (state->arg_type_infos[i]); > if (type_tag == GI_TYPE_TAG_ARRAY > && (direction != GI_DIRECTION_IN && transfer == GI_TRANSFER_NOTHING)) { > /* We created a #GArray and it has not been released above, so free it. */ >- state->args[i]->v_pointer = g_array_free (state->args[i]->v_pointer, FALSE); >+ if (g_type_info_get_array_type (state->arg_type_infos[i]) == GI_ARRAY_TYPE_C && >+ !state->c_arrays_are_wrapped) { >+ /* HACK: Noop - we are in an inconsitant state due to >+ * complex array handler so leak any C arrays >+ * as we don't know if we can free them safely. >+ * This will be removed when we merge the >+ * invoke rewrite branch. >+ */ >+ } else { >+ _pygi_argument_release (state->args[i], state->arg_type_infos[i], >+ transfer, direction); >+ state->args[i]->v_pointer = g_array_free (state->args[i]->v_pointer, FALSE); >+ } > } >+ } else { >+ _pygi_argument_release (state->args[i], state->arg_type_infos[i], >+ transfer, direction); > } > > } >-- >1.7.4.1 This is wrong. You've put an else on the if (state->args != NULL && state->args[i] != NULL) instead of the inner if. It is going to try and release NULL args. But this is not the main problem, probably because _pygi_argument_release() does NULL checks of its own. The main problem is that while this if statement looks like the one where C arrays are rewrapped, it is not: >if (type_tag == GI_TYPE_TAG_ARRAY > && (direction != GI_DIRECTION_IN && transfer == GI_TRANSFER_NOTHING)) { That is statement is there it handle *out* arguments only. It is meant to free the GArray if it was caller allocated. Because there is an && after the (direction != GI_DIRECTION_IN), it will not be entered for in arguments. And *in* C arrays is where the crash is occurring.
Created attachment 181684 [details] [review] [gi] Test case with John's fix for crash with C arrays and a GError is set. I have added a test case, and made a few fixes to John's patch, but the solution is the same his. Workaround a bug when freeing C array types * This is a hack and there is really no way around it without ripping out the current array handling code which spans between pygi-invoke.c and pygi-argument.c and completely rewriting it. * The is no time before our stable release * This patch trades a segfault for a leak in the very unusual case where an error occures inside an interface that takes one or more C arrays. Since we wrap C arrays in GArrays internally but have to unwrap them to send them to the introspected C function, there is a period of time where an error can occure with the C array in an unknown state (some being true C arrays and others still wrapped in a GArray) * This patch adds a c_arrays_are_wrapped state to signal that it is safe to free them. However since c_arrays_are_wrapped can only track arrays as a group, not individually, if it is set to FALSE we can not assume that every array is a pure C array, so instead we will simply leak them to avoid incorrectly freeing one and causing a segfault. * This issue is fixed in the invoke rewrite branch as it treats C arrays and GArrays separately, however that branch is not yet ready to be merged and won't be until the next release.