[PATCH v2 0/5] MR3149: widl: Don't crash if an interface is missing an uuid
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55123 -- v2: widl: Don't crash if an interface is missing an uuid include/roparameterizediid: Make interfaces local include/mpegtype: Make IMpegAudioDecoder local include/amvideo: Make IFullScreenVideo local widl: Don't crash if an interface is empty https://gitlab.winehq.org/wine/wine/-/merge_requests/3149
From: Fabian Maurer <dark.shadow4(a)web.de> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55145 --- tools/widl/client.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/widl/client.c b/tools/widl/client.c index 704be91cb78..a45c189101b 100644 --- a/tools/widl/client.c +++ b/tools/widl/client.c @@ -525,6 +525,9 @@ static void write_client_ifaces(const statement_list_t *stmts, int expr_eval_rou fprintf(client, " */\n"); fprintf(client, "\n"); + if (!type_iface_get_stmts(iface)) + return; + LIST_FOR_EACH_ENTRY(stmt2, type_iface_get_stmts(iface), const statement_t, entry) { if (stmt2->type == STMT_DECLARATION && stmt2->u.var->declspec.stgclass == STG_NONE && -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3149
From: Fabian Maurer <dark.shadow4(a)web.de> --- include/amvideo.idl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/amvideo.idl b/include/amvideo.idl index f3dc45fb29e..47a60581c35 100644 --- a/include/amvideo.idl +++ b/include/amvideo.idl @@ -104,7 +104,8 @@ interface IQualProp : IUnknown [ object, /* uuid(dd1d7110-7836-11cf-bf47-00aa0055595a) conflicts with uuids.h */ - pointer_default(unique) + pointer_default(unique), + local ] interface IFullScreenVideo : IUnknown { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3149
From: Fabian Maurer <dark.shadow4(a)web.de> --- include/mpegtype.idl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/mpegtype.idl b/include/mpegtype.idl index dd0284cfc53..8d85e3a227a 100644 --- a/include/mpegtype.idl +++ b/include/mpegtype.idl @@ -24,7 +24,8 @@ cpp_quote("#include <mmreg.h>") [ object, /* uuid(b45dd570-3c77-11d1-abe1-00a0c905f375) conflicts with uuids.h */ - pointer_default(unique) + pointer_default(unique), + local ] interface IMpegAudioDecoder : IUnknown { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3149
From: Fabian Maurer <dark.shadow4(a)web.de> --- include/roparameterizediid.idl | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/roparameterizediid.idl b/include/roparameterizediid.idl index 85ee57d6f94..d4b6b66cd95 100644 --- a/include/roparameterizediid.idl +++ b/include/roparameterizediid.idl @@ -25,7 +25,8 @@ import "wtypes.idl"; typedef void *ROPARAMIIDHANDLE; [ - object + object, + local ] interface IRoSimpleMetaDataBuilder { @@ -84,7 +85,8 @@ interface IRoSimpleMetaDataBuilder } [ - object + object, + local ] interface IRoMetaDataLocator { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3149
From: Fabian Maurer <dark.shadow4(a)web.de> If an interface contains methods, it must have either the uuid or the local attribute Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55123 --- tools/widl/parser.y | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tools/widl/parser.y b/tools/widl/parser.y index b802f75874d..65ab51a8910 100644 --- a/tools/widl/parser.y +++ b/tools/widl/parser.y @@ -2660,6 +2660,20 @@ static void check_functions(const type_t *iface, int is_inside_library) check_remoting_args(func); } } + if (!is_attr(iface->attrs, ATTR_LOCAL) && !is_attr(iface->attrs, ATTR_UUID) && !is_attr(iface->attrs, ATTR_VERSION)) + { + statement_list_t* methods = type_iface_get_stmts(iface); + if (methods) + { + int method_count = 0; + STATEMENTS_FOR_EACH_FUNC( stmt, methods ) + { + method_count++; + } + if (method_count) + error_at( &iface->where, "Can't omit both local and uuid keyword" ); + } + } } static int async_iface_attrs(attr_list_t *attrs, const attr_t *attr) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3149
On Mon Jun 26 18:17:19 2023 +0000, Fabian Maurer wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/3149/diffs?diff_id=53823&start_sha=ef82549524e738da8e2db0a255c1ccd2357f9073#5f5efa794515cbbbcc0003455fa2884723d05b01_444_443) Thanks for the feedback. Turns out, it's even more complicated than I thought. That logic only applies if the interface contains methods. Some interfaces only contain typedefs or similar, and they don't need those attributes.
Wine also has some interfaces that have no uuid set, because they conflict with uuids.h, and not sure what exactly is going on with roparameterizediid. I made those interfaces local, though not sure if that's the right way to go about that. I don't know the windows reference. For video/audio it seems sensible, not so sure about the WinRT stuff. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_36847
Do we actually need this working? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_36848
On Mon Jun 26 18:28:47 2023 +0000, Nikolay Sivov wrote:
Do we actually need this working? Wouldn't you prefer an error message over a crash?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_36849
On Mon Jun 26 18:21:27 2023 +0000, Fabian Maurer wrote:
Thanks for the feedback. Turns out, it's even more complicated than I thought. That logic only applies if the interface contains methods. Some interfaces only contain typedefs or similar, and they don't need those attributes. Wine also has some interfaces that have no uuid set, because they conflict with uuids.h, and not sure what exactly is going on with roparameterizediid. I made those interfaces local, though not sure if that's the right way to go about that. I don't know the windows reference. For video/audio it seems sensible, not so sure about the WinRT stuff. How should I continue here?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37829
Rémi Bernon (@rbernon) commented about include/roparameterizediid.idl:
typedef void *ROPARAMIIDHANDLE;
[ - object + object, + local ] interface IRoSimpleMetaDataBuilder I think these should be marked `[local]` but not `[object]`. The `[object]` attribute normally also requires deriving from `IUnknown`, and they don't. Currently widl doesn't check that but midl does.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37834
Rémi Bernon (@rbernon) commented about tools/widl/parser.y:
} } + if (!is_attr(iface->attrs, ATTR_LOCAL) && !is_attr(iface->attrs, ATTR_UUID) && !is_attr(iface->attrs, ATTR_VERSION)) + { + statement_list_t* methods = type_iface_get_stmts(iface); + if (methods) + { + int method_count = 0; + STATEMENTS_FOR_EACH_FUNC( stmt, methods ) + { + method_count++; + } + if (method_count) + error_at( &iface->where, "Can't omit both local and uuid keyword" ); + } + }
I believe the check should be: 1) `[object]` interfaces are required to have a `[uuid]`, empty or not. 2) `[local]` interfaces are allowed to not have a `[uuid]`, empty or not. 3) remote (non-`[object]`, non-`[local]`) interfaces must have an `[uuid]`, unless they are empty, but they aren't supposed to be empty and there's a separate warning. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37835
On Tue Jul 4 16:59:02 2023 +0000, Rémi Bernon wrote:
I believe the check should be: 1) `[object]` interfaces are required to have a `[uuid]`, empty or not. 2) `[local]` interfaces are allowed to not have a `[uuid]`, empty or not. 3) remote (non-`[object]`, non-`[local]`) interfaces must have an `[uuid]`, unless they are empty, but they aren't supposed to be empty and there's a separate warning. Fwiw this is in order of priority, `[object,local]` interface have to have an `[uuid]` under midl semantics.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37836
On Tue Jul 4 16:59:02 2023 +0000, Rémi Bernon wrote:
I think these should be marked `[local]` but not `[object]`. The `[object]` attribute normally also requires deriving from `IUnknown`, and they don't. Currently widl doesn't check that but midl does. But omitting the `object` doesn't output the `IRoSimpleMetaDataBuilder` struct, leading to compile errors.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37837
On Tue Jul 4 17:35:20 2023 +0000, Fabian Maurer wrote:
But omitting the `object` doesn't output the `IRoSimpleMetaDataBuilder` struct, leading to compile errors. Right... of course.
Well other possible options: 1) use a custom and random `uuid` there, 2) use a custom widl pragma / attribute to relax the missing `uuid` (and future base interface) checks on `object` 3) Stop using IDL for these interfaces as it is not supposed to. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37838
On Tue Jul 4 17:01:07 2023 +0000, Rémi Bernon wrote: > Fwiw this is in order of priority, `[object,local]` interface have to > have an `[uuid]` under midl semantics. I did a few more tests with midl: - If you have a `local` interface that inherits `IUnknown` it needs to have an uuid. Not sure if that is an allowed case though - A `local, object` interface also needs an uuid. Not sure what you mean with "order of priority" - A `local, object` interface always needs an uuid, even if empty Now I have the problem that some interfaces, like ID3DInclude, are `object, local`, but don't have an uuid. Not sure how I would fix that. Similar to `IDirectDrawVideo` that is `object` but doesn't have a uuid, since it's defined in uuids.h already. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37841
On Tue Jul 4 18:06:45 2023 +0000, Rémi Bernon wrote:
Right... of course. Well other possible options: 1) use a custom and random `uuid` there, 2) use a custom widl pragma / attribute to relax the missing `uuid` (and future base interface) checks on `object` 3) Stop using IDL for these interfaces as it is not supposed to. First off, thank you already. And sorry for opening such a can of worms, I just wanted to prevent a crash and didn't expect this to turn out so complicated.
1. This would work for `IRoSimpleMetaDataBuilder` but since `IDirectDrawVideo` has a similar issue I don't think this is the way forward. `IDirectDrawVideo` can't have its uuid defined since that already comes from uuid.h. While approach 3 would be closer to windows, I do think 2 would be easier/cleaner. A `wine_ignore_guid` attribute would be easily added. What would be your suggestion? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37842
On Tue Jul 4 18:22:00 2023 +0000, Fabian Maurer wrote:
First off, thank you already. And sorry for opening such a can of worms, I just wanted to prevent a crash and didn't expect this to turn out so complicated. 1. This would work for `IRoSimpleMetaDataBuilder` but since `IDirectDrawVideo` has a similar issue I don't think this is the way forward. `IDirectDrawVideo` can't have its uuid defined since that already comes from uuid.h. While approach 3 would be closer to windows, I do think 2 would be easier/cleaner. A `wine_ignore_guid` attribute would be easily added. What would be your suggestion? I'm not completely sure, from a quick look at the list of interface you have found it seems that all the problem that we have are coming from our attempt at using IDL for interfaces which are not defined in IDL in the SDK. Maybe 3) is the simplest solution.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37856
On Tue Jul 4 18:15:26 2023 +0000, Fabian Maurer wrote:
I did a few more tests with midl: - If you have a `local` interface that inherits `IUnknown` it needs to have an uuid. Not sure if that is an allowed case though - A `local, object` interface also needs an uuid. Not sure what you mean with "order of priority" - A `local, object` interface always needs an uuid, even if empty Now I have the problem that some interfaces, like ID3DInclude, are `object, local`, but don't have an uuid. Not sure how I would fix that. Similar to `IDirectDrawVideo` that is `object` but doesn't have a uuid, since it's defined in uuids.h already. Yes, I meant the checks for an interface with `object` always come first, and they are supposed to always have an `uuid` attribute. Only if `object` attribute is missing, then `local` attribute is checked and missing `uuid` is allowed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37857
On Tue Jul 4 19:07:54 2023 +0000, Rémi Bernon wrote:
I'm not completely sure, from a quick look at the list of interface you have found it seems that all the problem that we have are coming from our attempt at using IDL for interfaces which are not defined in IDL in the SDK. Maybe 3) is the simplest solution. Wait, why can't the interface just be [object, local] as Fabian has it?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37859
'm not completely sure, from a quick look at the list of interface you have found it seems that all the problem that we have are coming from our attempt at using IDL for interfaces which are not defined in IDL in the SDK
Pretty much, yes. But then again, using widl saves us some boilerplate code. IMHO it has advantages keeping it as idl, although we would need workarounds to keep it working. Moving the files from .idl to .h is IMHO quite a bit of effort. Compare that to approach 2 where I made a POC here: https://gitlab.winehq.org/DarkShadow44/wine/-/commits/widl_uuid2 Although, for both approaches I'm kinda worried that such changes may break downstream projects like mingw. After all, I don't know if their headers break with that change as well. Is that a consideration? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37860
On Tue Jul 4 19:42:51 2023 +0000, Fabian Maurer wrote:
'm not completely sure, from a quick look at the list of interface you have found it seems that all the problem that we have are coming from our attempt at using IDL for interfaces which are not defined in IDL in the SDK Pretty much, yes. But then again, using widl saves us some boilerplate code. IMHO it has advantages keeping it as idl, although we would need workarounds to keep it working. Moving the files from .idl to .h is IMHO quite a bit of effort. Compare that to approach 2 where I made a POC here: https://gitlab.winehq.org/DarkShadow44/wine/-/commits/widl_uuid2 Although, for both approaches I'm kinda worried that such changes may break downstream projects like mingw. After all, I don't know if their headers break with that change as well. Is that a consideration? @zfigura Because as soon as it's `object` you need an uuid.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37861
On Tue Jul 4 19:43:28 2023 +0000, Fabian Maurer wrote:
@zfigura Because as soon as it's `object` you need an uuid. Interfaces with `object` are also supposed to derive from IUnknown.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37862
On Tue Jul 4 19:48:10 2023 +0000, Rémi Bernon wrote:
Interfaces with `object` are also supposed to derive from IUnknown. Oh, I'm sorry, I misread, I thought you said that interfaces with [object, local] don't need uuids.
Does midl actually forbid [object] that doesn't derive from IUnknown? If not, does it still require a uuid for those [even though it doesn't make any sense for them to have one]? Even if so, maybe we could relax that requirement anyway? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37869
On Tue Jul 4 20:24:10 2023 +0000, Zebediah Figura wrote:
Oh, I'm sorry, I misread, I thought you said that interfaces with [object, local] don't need uuids. Does midl actually forbid [object] that doesn't derive from IUnknown? If not, does it still require a uuid for those [even though it doesn't make any sense for them to have one]? Even if so, maybe we could relax that requirement anyway? It does forbid them.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37871
On Tue Jul 4 20:37:08 2023 +0000, Rémi Bernon wrote:
It does forbid them. Actually, more precisely it's a warning when objects don't derive from IUnknown. But then object also require uuid, and that's not a midl warning but an error.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37872
On Tue Jul 4 20:40:40 2023 +0000, Rémi Bernon wrote:
Actually, more precisely it's a warning when objects don't derive from IUnknown. But then object also require uuid, and that's not a midl warning but an error. So, should I start converting the affected idls to headers? If so, in this MR or maybe each file separately?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3149#note_37926
participants (5)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Nikolay Sivov (＠nsivov)
-
Rémi Bernon
-
Zebediah Figura (＠zfigura)