[PATCH v3 0/2] MR3151: widl: Prevent infinite loop when structure contains array of itself
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55115 -- v3: widl: In type_has_full_pointer recurse for pointer types as well widl: Prevent infinite loop when structure contains array of itself https://gitlab.winehq.org/wine/wine/-/merge_requests/3151
From: Fabian Maurer <dark.shadow4(a)web.de> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55115 --- tools/widl/typegen.c | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/tools/widl/typegen.c b/tools/widl/typegen.c index b3373ded11d..9aadfd06c2a 100644 --- a/tools/widl/typegen.c +++ b/tools/widl/typegen.c @@ -747,8 +747,22 @@ static int type_has_pointers(const type_t *type) } static int type_has_full_pointer(const type_t *type, const attr_list_t *attrs, - int toplevel_param) + int toplevel_param, int visited_structs_pos) { + static const type_t **visited_structs; + static int visited_structs_count; + + if (!visited_structs) + { + visited_structs_count = 100; + visited_structs = xmalloc(sizeof (type_t*) * visited_structs_count); + } + + if (visited_structs_pos >= visited_structs_count) + { + visited_structs_count *= 2; + visited_structs = xrealloc(visited_structs, sizeof (type_t*) * visited_structs_count); + } switch (typegen_detect_type(type, NULL, TDT_IGNORE_STRINGS)) { case TGT_USER_TYPE: @@ -762,14 +776,26 @@ static int type_has_full_pointer(const type_t *type, const attr_list_t *attrs, if (get_pointer_fc(type, attrs, toplevel_param) == FC_FP) return TRUE; else - return type_has_full_pointer(type_array_get_element_type(type), NULL, FALSE); + return type_has_full_pointer(type_array_get_element_type(type), NULL, FALSE, visited_structs_pos); case TGT_STRUCT: { + int i; var_list_t *fields = type_struct_get_fields(type); const var_t *field; + + visited_structs[visited_structs_pos] = type; + for (i = 0; i < visited_structs_pos; i++) + { + if (visited_structs[i] == type) + { + /* Found struct we visted already, abort to prevent infinite loop */ + return FALSE; + } + } + if (fields) LIST_FOR_EACH_ENTRY( field, fields, const var_t, entry ) { - if (type_has_full_pointer(field->declspec.type, field->attrs, FALSE)) + if (type_has_full_pointer(field->declspec.type, field->attrs, FALSE, visited_structs_pos + 1)) return TRUE; } break; @@ -781,7 +807,7 @@ static int type_has_full_pointer(const type_t *type, const attr_list_t *attrs, fields = type_union_get_cases(type); if (fields) LIST_FOR_EACH_ENTRY( field, fields, const var_t, entry ) { - if (field->declspec.type && type_has_full_pointer(field->declspec.type, field->attrs, FALSE)) + if (field->declspec.type && type_has_full_pointer(field->declspec.type, field->attrs, FALSE, visited_structs_pos)) return TRUE; } break; @@ -2082,12 +2108,12 @@ static unsigned int type_buffer_alignment(const type_t *t) int is_full_pointer_function(const var_t *func) { const var_t *var; - if (type_has_full_pointer(type_function_get_rettype(func->declspec.type), func->attrs, TRUE)) + if (type_has_full_pointer(type_function_get_rettype(func->declspec.type), func->attrs, TRUE, 0)) return TRUE; if (!type_function_get_args(func->declspec.type)) return FALSE; LIST_FOR_EACH_ENTRY( var, type_function_get_args(func->declspec.type), const var_t, entry ) - if (type_has_full_pointer( var->declspec.type, var->attrs, TRUE )) + if (type_has_full_pointer( var->declspec.type, var->attrs, TRUE, 0 )) return TRUE; return FALSE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3151
From: Fabian Maurer <dark.shadow4(a)web.de> --- tools/widl/typegen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/widl/typegen.c b/tools/widl/typegen.c index 9aadfd06c2a..d2baa3d9c00 100644 --- a/tools/widl/typegen.c +++ b/tools/widl/typegen.c @@ -771,7 +771,7 @@ static int type_has_full_pointer(const type_t *type, const attr_list_t *attrs, if (get_pointer_fc(type, attrs, toplevel_param) == FC_FP) return TRUE; else - return FALSE; + return type_has_full_pointer(type_pointer_get_ref_type(type), NULL, FALSE, visited_structs_pos); case TGT_ARRAY: if (get_pointer_fc(type, attrs, toplevel_param) == FC_FP) return TRUE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3151
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=137209 Your paranoid android. === debian11b (64 bit WoW report) === uiautomationcore: uiautomation.c:14688: Test failed: Unexpected hr 0x800706b5. uiautomation.c:14689: Test failed: expected prov_callback_base_hwnd uiautomation.c:14690: Test failed: expected prov_callback_nonclient uiautomation.c:14704: Test failed: Wait for event_handle failed. uiautomation.c:14705: Test failed: expected uia_event_callback uiautomation.c:15296: Test failed: 5 failures in child process
On Sat Sep 9 12:55:32 2023 +0000, Fabian Maurer wrote:
When recursing into array pointer, we get a stackoverflow due to infinite recursion as well. Therefore, unless I missed something, we need first the infinite-recursion fix, then the array recursion change. The file that causes the stackoverflow with array recursion is actxprxy_shobjidl.idl. However, even with the current master branch it crashes when running widl manually like ``` ./tools/widl/widl -o test.c -m32 --nostdinc -Ldlls/\* -Idlls/actxprxy \ -I../wine-git/dlls/actxprxy -Iinclude -I../wine-git/include -I../wine-git/include/msvcrt -D_UCRT \ -D__WINESRC__ ../wine-git/dlls/actxprxy/actxprxy_shobjidl.idl ``` It works when compiling wine though... Something is very weird here, but it means I can't really check which type(s) exactly cause the infinite array-recursion. I also updated to use dynamic allocation, but kept the static variable. IMHO, since it's single threaded and only short lived, it's easier to just leave the memory than constantly allocate and free the memory. Can be changed though, if you think that's preferable. The static variable feels brittle to me; I guess there's no reason it doesn't work but if it were up to me I'd still prefer to avoid it. If it were me I'd just embed all the array parameters in a struct and pass that down.
I think in general what you're calling "pos" and "count", I'm used to calling "count" and "capacity". -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_45366
participants (4)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Marvin -
Zebediah Figura (＠zfigura)