[PATCH 5/8] server: Notify client about freed object so that it may free associated kernel object.
Long term, we may consider making interface between server and device manager more generic so that it could be used for messages other than IRPs. Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 14 +++++++++++++- server/device.c | 15 +++++++++++++++ server/protocol.def | 6 ++++++ 3 files changed, 34 insertions(+), 1 deletion(-)
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=49453 Your paranoid android. === debian9b (32 bit WoW report) === Report errors: ntoskrnl.exe:ntoskrnl contains a misplaced todo message for driver === debian9b (64 bit WoW report) === Report errors: ntoskrnl.exe:ntoskrnl contains a misplaced todo message for driver === debian9b (build log) === Task errors: Unable to set the VM system time: the "nc -q0 '10.42.42.143' '4243'" command returned 1 (settime/connect). Maybe the TestAgentd process is missing the required privileges.
On 03/15/2019 09:58 AM, Jacek Caban wrote:
Long term, we may consider making interface between server and device manager more generic so that it could be used for messages other than IRPs.
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 14 +++++++++++++- server/device.c | 15 +++++++++++++++ server/protocol.def | 6 ++++++ 3 files changed, 34 insertions(+), 1 deletion(-)
Sorry if this is a dumb question, but why do we need this (and then, by extension, the whole infrastructure involving server changes)?
I am also curious about something, what was wrong w/ the old infrastructure where the object structure held the list to kernel_objects? On Fri, Mar 15, 2019 at 12:51 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
On 03/15/2019 09:58 AM, Jacek Caban wrote:
Long term, we may consider making interface between server and device manager more generic so that it could be used for messages other than IRPs.
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 14 +++++++++++++- server/device.c | 15 +++++++++++++++ server/protocol.def | 6 ++++++ 3 files changed, 34 insertions(+), 1 deletion(-)
Sorry if this is a dumb question, but why do we need this (and then, by extension, the whole infrastructure involving server changes)?
Hi Derek, On 3/15/19 6:15 PM, Derek Lesho wrote:
I am also curious about something, what was wrong w/ the old infrastructure where the object structure held the list to kernel_objects?
It's mostly about safety. Note that server does not trust clients to be sane, even if the client is a kernel in Windows sense. The patchset allowed storing a reference to an arbitrary handle-accessible object. Although from object's perspective it mostly behaves like yet another handle, it's not exactly the same (otherwise it wouldn't make sense to introduce it). There are some corner cases where kernel process could harm wineserver if it's not treated with care. For example it could try store a reference to device manager (something Windows application would not do since there is no such thing as server device manager handle on Windows), which would cause circular dependency and a leak. In previous patchset I tried to catch such cases. Avoiding the above needed a special case, which wasn't really nice. Allowing references on only explicitly whitelisted object types solves the problem. Jacek
Ah, I see. Wouldn't it be cleaner to put a whitelist inside of device.c and see if the incoming object type is on that whitelist? This seems like it would be a more self-contained solution to the problem. On Fri, Mar 15, 2019 at 2:32 PM Jacek Caban <jacek(a)codeweavers.com> wrote:
Hi Derek,
On 3/15/19 6:15 PM, Derek Lesho wrote:
I am also curious about something, what was wrong w/ the old infrastructure where the object structure held the list to kernel_objects?
It's mostly about safety. Note that server does not trust clients to be sane, even if the client is a kernel in Windows sense. The patchset allowed storing a reference to an arbitrary handle-accessible object. Although from object's perspective it mostly behaves like yet another handle, it's not exactly the same (otherwise it wouldn't make sense to introduce it). There are some corner cases where kernel process could harm wineserver if it's not treated with care. For example it could try store a reference to device manager (something Windows application would not do since there is no such thing as server device manager handle on Windows), which would cause circular dependency and a leak. In previous patchset I tried to catch such cases. Avoiding the above needed a special case, which wasn't really nice. Allowing references on only explicitly whitelisted object types solves the problem.
Jacek
On 3/15/19 8:43 PM, Derek Lesho wrote:
Ah, I see. Wouldn't it be cleaner to put a whitelist inside of device.c and see if the incoming object type is on that whitelist? This seems like it would be a more self-contained solution to the problem.
I don't see a clean way to do that. Besides, if we consider being able to be associated with kernel object be a feature of object type, having it inside object type seems right. We could consider moving the list back to object header, but keep the new op and limit its functionality to allow or not the association. This would allow sharing slightly more code at expense of making all object use more memory (even those not supporting kernel objects). It's probably not a big deal through. Jacek
On 3/15/19 5:50 PM, Zebediah Figura wrote:
On 03/15/2019 09:58 AM, Jacek Caban wrote:
Long term, we may consider making interface between server and device manager more generic so that it could be used for messages other than IRPs.
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 14 +++++++++++++- server/device.c | 15 +++++++++++++++ server/protocol.def | 6 ++++++ 3 files changed, 34 insertions(+), 1 deletion(-)
Sorry if this is a dumb question, but why do we need this (and then, by extension, the whole infrastructure involving server changes)?
Generally speaking, it's meant to allow kernel objects implementation. The main part in my consideration was ObReferenceObjectByHandle. Although we could have more hacks around it without server support, there are limitations to what's possible to hack. If we want to return exactly the same object for given actual object (implemented usually in server in Wine case), we need server involved in handle to object mapping. Once we have that, there is another problem with object life time. Driver may release its reference to the object, but an other process may still have open handles. On Windows, those handles would keep object reference count of the object, but on Wine that count is in server, not ntoskrnl.exe. It means that only server knows when the object is actually destroyed. If we don't want to leak in ntoskrnl.exe, we need some mechanism to be notified about object being freed. Also such kernel object reflecting a server object may be used as an argument for other functions, like KeSetEvent. Unlike events initialized with KeInitializeEvent, we don't have much control over such event. MRAC does that: it creates an event, passes it by handle in ioctl(), driver uses ObReferenceObjectByHandle and then KeSetEvent on this even. So we need a way to implement such functions on top of kernel object pointers. I tried storing handles on client side, but it had more problems as I noted in [1]. Instead, we may just get a handle whenever we need them and that's all we need to implement KeSetEvent on top of NtSetEvent. In case of MRAC, it also needs FILE_OBJECT and PETHREAD. For FILE_OBJECT, it checks some of its fields. I didn't debug details yet, but found that some fake values satisfied its checks. PETHREAD follows the pattern of bug 46205, where ObReferenceObjectByHandle needs to return an object that we will be able to wait on later. On top of this series, all we need is something similar to [2]. And once we have that, KeGetCurrentThread (which you were interested in), would be a matter of calling ObReferenceObjectByHandle with current thread handle (and probably caching the result in TEB; also server would know thread client pointer, so it could just pass it directly in get_next_device_request when needed). So there are multiple problems to solve. Some may be worked around without server support, others not. I tried to make it generic enough to fit all those requirements and allow using it for all objects. Note that server already has limited mechanism for mapping device files and devices to user pointers, which could also use the generic mechanism instead. Jacek [1] https://source.winehq.org/patches/data/159405 [2] https://source.winehq.org/patches/data/161002
On 03/15/2019 01:19 PM, Jacek Caban wrote:
On 3/15/19 5:50 PM, Zebediah Figura wrote:
On 03/15/2019 09:58 AM, Jacek Caban wrote:
Long term, we may consider making interface between server and device manager more generic so that it could be used for messages other than IRPs.
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 14 +++++++++++++- server/device.c | 15 +++++++++++++++ server/protocol.def | 6 ++++++ 3 files changed, 34 insertions(+), 1 deletion(-)
Sorry if this is a dumb question, but why do we need this (and then, by extension, the whole infrastructure involving server changes)?
Generally speaking, it's meant to allow kernel objects implementation. The main part in my consideration was ObReferenceObjectByHandle. Although we could have more hacks around it without server support, there are limitations to what's possible to hack. If we want to return exactly the same object for given actual object (implemented usually in server in Wine case), we need server involved in handle to object mapping.
Once we have that, there is another problem with object life time. Driver may release its reference to the object, but an other process may still have open handles. On Windows, those handles would keep object reference count of the object, but on Wine that count is in server, not ntoskrnl.exe. It means that only server knows when the object is actually destroyed. If we don't want to leak in ntoskrnl.exe, we need some mechanism to be notified about object being freed.
Also such kernel object reflecting a server object may be used as an argument for other functions, like KeSetEvent. Unlike events initialized with KeInitializeEvent, we don't have much control over such event. MRAC does that: it creates an event, passes it by handle in ioctl(), driver uses ObReferenceObjectByHandle and then KeSetEvent on this even. So we need a way to implement such functions on top of kernel object pointers. I tried storing handles on client side, but it had more problems as I noted in [1]. Instead, we may just get a handle whenever we need them and that's all we need to implement KeSetEvent on top of NtSetEvent.
Thanks, it was IRP_MJ_CLOSE that I had not considered. I guess there's indeed no way to hold a reference without handle, without introducing server changes.
participants (4)
-
Derek Lesho -
Jacek Caban -
Marvin -
Zebediah Figura