[PATCH 0/1] MR3151: widl: Prevent infinite loop when structure contains array of itself
From: Fabian Maurer <dark.shadow4(a)web.de> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55115 --- tools/widl/typegen.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/tools/widl/typegen.c b/tools/widl/typegen.c index b3373ded11d..2736fd4f6bd 100644 --- a/tools/widl/typegen.c +++ b/tools/widl/typegen.c @@ -747,8 +747,13 @@ 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[100]; + if (visited_structs_pos >= ARRAY_SIZE(visited_structs)) + { + error("Too many structs visited."); + } switch (typegen_detect_type(type, NULL, TDT_IGNORE_STRINGS)) { case TGT_USER_TYPE: @@ -762,14 +767,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 +798,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 +2099,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
Besides the global variable and fixed-size array, both of which seem undesirable and avoidable, I'm bothered by the inconsistency between arrays and pointers. Why do we recurse into the pointer type for arrays, but not for pointers? I don't have midl handy, so I can't easily check, but it seems suspicious that the logic is different there in the first place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_36700
On Fri Jun 23 23:08:53 2023 +0000, Zebediah Figura wrote:
Besides the global variable and fixed-size array, both of which seem undesirable and avoidable, I'm bothered by the inconsistency between arrays and pointers. Why do we recurse into the pointer type for arrays, but not for pointers? I don't have midl handy, so I can't easily check, but it seems suspicious that the logic is different there in the first place. Sure, I'm open to better suggestions, I just couldn't think of anything better for now. Is passing it through and dynamically allocating it better?
As for the difference between arrays and pointers, I'm not too familiar with the details of widl, I just worked with what's there already. I thought the difference between arrays and pointers is, that arrays have a known length (or are local). After all, you can't really marshal a pointer if you don't know how many elements. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_36701
Sure, I'm open to better suggestions, I just couldn't think of anything better for now. Is passing it through and dynamically allocating it better?
Yes, I think so.
As for the difference between arrays and pointers, I'm not too familiar with the details of widl, I just worked with what's there already. I thought the difference between arrays and pointers is, that arrays have a known length (or are local). After all, you can't really marshal a pointer if you don't know how many elements.
Unsized pointers are basically equivalent to an array of size 1. Fundamentally, every type *has* to be marshalable, or you've declared an illegal interface. Anyway, I tested with midl now that I have it handy, and we're definitely supposed to recurse into non-array pointers. It should be simple enough to fix, but if you'd rather not then I'll send a patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_44087
On Fri Sep 1 20:33:24 2023 +0000, Zebediah Figura wrote:
Sure, I'm open to better suggestions, I just couldn't think of anything better for now. Is passing it through and dynamically allocating it better? Yes, I think so. As for the difference between arrays and pointers, I'm not too familiar with the details of widl, I just worked with what's there already. I thought the difference between arrays and pointers is, that arrays have a known length (or are local). After all, you can't really marshal a pointer if you don't know how many elements. Unsized pointers are basically equivalent to an array of size 1. Fundamentally, every type *has* to be marshalable, or you've declared an illegal interface. Anyway, I tested with midl now that I have it handy, and we're definitely supposed to recurse into non-array pointers. It should be simple enough to fix, but if you'd rather not then I'll send a patch. Sure, I will do that. Pushing an update soonish, currently pretty busy.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_44270
On Mon Sep 4 22:08:57 2023 +0000, Fabian Maurer wrote:
Sure, I will do that. Pushing an update soonish, currently pretty busy. 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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_44816
participants (3)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Zebediah Figura (＠zfigura)