[PATCH v4 0/2] MR3151: widl: Prevent infinite loop when structure contains array of itself
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55115 -- v4: 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 | 62 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 54 insertions(+), 8 deletions(-) diff --git a/tools/widl/typegen.c b/tools/widl/typegen.c index b3373ded11d..332bff87fd4 100644 --- a/tools/widl/typegen.c +++ b/tools/widl/typegen.c @@ -746,8 +746,15 @@ static int type_has_pointers(const type_t *type) return FALSE; } +struct visited_struct_array +{ + const type_t **structs; + int count; + int capacity; +}; + static int type_has_full_pointer(const type_t *type, const attr_list_t *attrs, - int toplevel_param) + int toplevel_param, struct visited_struct_array *visited_structs) { switch (typegen_detect_type(type, NULL, TDT_IGNORE_STRINGS)) { @@ -762,17 +769,55 @@ 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); case TGT_STRUCT: { + int i; + int ret = FALSE; var_list_t *fields = type_struct_get_fields(type); const var_t *field; + + if (!visited_structs->structs) + { + visited_structs->count = 0; + visited_structs->capacity = 100; + visited_structs->structs = xmalloc(sizeof (type_t*) * visited_structs->capacity); + } + + if (visited_structs->count >= visited_structs->capacity) + { + visited_structs->capacity *= 2; + visited_structs->structs = xrealloc(visited_structs->structs, sizeof (type_t*) * visited_structs->capacity); + } + + visited_structs->structs[visited_structs->count] = type; + for (i = 0; i < visited_structs->count; i++) + { + if (visited_structs->structs[i] == type) + { + /* Found struct we visited already, abort to prevent infinite loop. + * Can't be at the first struct we visit, so we can skip cleanup and just return */ + return FALSE; + } + } + + visited_structs->count++; if (fields) LIST_FOR_EACH_ENTRY( field, fields, const var_t, entry ) { - if (type_has_full_pointer(field->declspec.type, field->attrs, FALSE)) - return TRUE; + if (type_has_full_pointer(field->declspec.type, field->attrs, FALSE, visited_structs)) + { + ret = TRUE; + break; + } } - break; + visited_structs->count--; + if (!visited_structs->count) + { + /* End of recursion, clean up */ + free(visited_structs->structs); + visited_structs->structs = 0; + } + return ret; } case TGT_UNION: { @@ -781,7 +826,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)) return TRUE; } break; @@ -2082,12 +2127,13 @@ 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)) + struct visited_struct_array visited_structs = {0}; + if (type_has_full_pointer(type_function_get_rettype(func->declspec.type), func->attrs, TRUE, &visited_structs)) 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, &visited_structs )) 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 332bff87fd4..0c8644a73f4 100644 --- a/tools/widl/typegen.c +++ b/tools/widl/typegen.c @@ -764,7 +764,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); 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
On Thu Sep 14 17:44:20 2023 +0000, Zebediah Figura wrote:
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". Sure, I pushed a rework. IMHO I found a pretty clean solution, although memory management combined with recursion is kinda tricky, so would you mind double checking I didn't miss an edgecase?
I made it so the struct is cleaned before/after use and possibly reused. The array is only allocated once it's needed, and when the last recursion finishes it's freed again. I didn't want to allocate/free the array outside of the functions, I believe it's cleaner this way. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_45538
On Fri Sep 15 16:11:00 2023 +0000, Fabian Maurer wrote:
Sure, I pushed a rework. IMHO I found a pretty clean solution, although memory management combined with recursion is kinda tricky, so would you mind double checking I didn't miss an edgecase? I made it so the struct is cleaned before/after use and possibly reused. The array is only allocated once it's needed, and when the last recursion finishes it's freed again. I didn't want to allocate/free the array outside of the functions, I believe it's cleaner this way. I would just use a separate wrapper function to call the base case, and free the array from there, rather than decrementing count like that. That's what I see more often so I'd call it more idiomatic.
A more minor comment, but those various "int" variables should really be unsigned. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_45564
I would just use a separate wrapper function to call the base case, and free the array from there, rather than decrementing count like that.
Sorry, I'm not sure I understand what you mean... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_45623
On Sat Sep 16 01:02:17 2023 +0000, Fabian Maurer wrote:
I would just use a separate wrapper function to call the base case, and free the array from there, rather than decrementing count like that. Sorry, I'm not sure I understand what you mean... I mean something like:
static bool type_has_full_pointer_recurse(..., struct visited_struct_array *structs) { ... } static bool type_has_full_pointer(...) { struct visited_struct_array structs = {0}; bool ret = type_has_full_pointer_recurse(..., &structs); free(structs.structs); return ret; } -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_45747
On Mon Sep 18 17:19:12 2023 +0000, Zebediah Figura wrote:
I mean something like: static bool type_has_full_pointer_recurse(..., struct visited_struct_array *structs) { ... } static bool type_has_full_pointer(...) { struct visited_struct_array structs = {0}; bool ret = type_has_full_pointer_recurse(..., &structs); free(structs.structs); return ret; } Actually since this is a short-lived program you could even skip freeing the array.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3151#note_45748
participants (3)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Zebediah Figura (＠zfigura)