Upgrade windows crate #51

rgwood · 2022-05-19T16:45:44Z

This is not quite done but I'm putting it up as a draft in case people want to provide feedback.

Building on #49, I'm able to get everything compiling but the tests crash in the list() function:

running 7 tests
test tests::os_limited::restore_empty ... ok
test tests::os_limited::purge_empty ... ok
error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `C:\Users\reill\source\trash-rs\target\debug\deps\trash-5fc85ca46a4e948f.exe` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

I suspect I'm doing something wrong with the way I create an ITEMIDLIST:
let mut item_uninit = MaybeUninit::<ITEMIDLIST>::uninit();

I'll revisit this in a few days; my brain is fried from working at the intersection of unsafe Rust and Win32.

rgwood · 2022-05-19T16:46:19Z

src/windows.rs

@@ -44,54 +41,10 @@ const FOF_NO_UI: u32 = FOF_SILENT | FOF_NOCONFIRMATION | FOF_NOERRORUI | FOF_NOC
 const FOFX_EARLYFAILURE: u32 = 0x00100000;
 ///////////////////////////////////////////////////////////////////////////

-macro_rules! check_res_and_get_ok {


Think we can get rid of these macros and just do idiomatic ? Rust error handling now.

rgwood · 2022-05-19T16:47:54Z

src/windows.rs

-            });
+
+        // TODO: take care of freeing this?
+        let mut item_uninit = MaybeUninit::<ITEMIDLIST>::uninit();


I changed from MaybeUninit::<*mut ITEMIDLIST> to MaybeUninit::<ITEMIDLIST> to get things to compile and that was probably a bad idea. I need to spend some more time thinking about this.

From what I remember this must be a slice of sorts now, but I wasn't able to produce a pointer to it. It's probably an unsafe-Rust question, but if we should all be stuck I am sure we will get help over at the windows repository.

I'm almost certain that is incorrect. I think the correct way to do it is the following.

Note that item is a pointer, as it should be.
Please also add the call to CoTaskMemFree.

let next_items = &mut [std::ptr::null_mut()]; while let Ok(_) = peidl.Next(next_items, std::ptr::null_mut()) { let item = next_items[0]; defer! {{ CoTaskMemFree(item as *mut c_void); }}

rgwood · 2022-05-19T16:58:06Z

@ArturKovacs FYI this is as far as I was able to get yesterday. It's a bit rough and I need to spend some more time to formulate useful questions for you.

Feel free to take a look now if you're bored or curious, otherwise I'll come back to this in a few days and ping you with some specific questions.

Byron · 2022-05-20T00:16:21Z

my brain is fried from working at the intersection of unsafe Rust and Win32.

That was my experience as well, it's like using a *-sys crate in the typical ecosystem, and one would expect windows to not need any unsafe or pointer fiddling anymore.

rgwood · 2022-05-21T03:17:02Z

Made good progress on this today. This Raymond Chen article was really helpful, I've been porting the C++ from that to Rust. Will update the PR in a day or 2.

ArturKovacs · 2022-05-22T08:13:03Z

src/windows.rs

-            });
+
+        // TODO: take care of freeing this?
+        let mut item_uninit = MaybeUninit::<ITEMIDLIST>::uninit();


I'm almost certain that is incorrect. I think the correct way to do it is the following.

Note that item is a pointer, as it should be.
Please also add the call to CoTaskMemFree.

let next_items = &mut [std::ptr::null_mut()]; while let Ok(_) = peidl.Next(next_items, std::ptr::null_mut()) { let item = next_items[0]; defer! {{ CoTaskMemFree(item as *mut c_void); }}

src/windows.rs

rgwood · 2022-05-22T20:26:46Z

Tests are passing! I'm probably leaking memory though, will take a closer look at that soon (please jump in if you notice any obvious problems).

I managed to simplify some things with Mr. Chen's help:

IShellItem::GetDisplayName is easier to use than IShellFolder::GetDisplayNameOf
IShellItem2::GetFileTime is simpler than what we were doing before with GetDetailsEx and VARIANTs
We can take advantage of the fact that TrashItem IDs are parsing names on Windows, and replace a bunch of code in restore_all and purge_all with a SHCreateItemFromParsingName call
SHGetKnownFolderItem is an easier way to get the recycle bin as an IShellItem

Byron · 2022-05-23T02:18:10Z

I am very impressed by your ability to not only dig in and make it work but to also find simpler ways of doing the same thing in an API as big as the Mount Everest! Thank you!

rgwood · 2022-05-23T20:04:23Z

This might be ready for review. I spent some time trying to understand whether I need to explicitly free memory, and... I'm not always sure 😞.

In some situations, freeing is handled for us. For example, IShellItem is just a struct container for an IUnknown, and IUnknown has a Drop implementation that automatically calls Release(). I'm less sure about this code:

let item2: IShellItem2 = item.cast()?;
let original_location_variant: PROPVARIANT = item2.GetProperty(&SCID_ORIGINAL_LOCATION)?;
let original_location_bstr: BSTR = PropVariantToBSTR(&original_location_variant)?;

Looking into PROPVARIANT it's got a lot of ManuallyDrop uses under the hood, but BSTR has a ::core::ops::Drop implementation that frees its contents so... ¯\_(ツ)_/¯

Takeaway

If someone more experience in this area (@ArturKovacs?) could look over the current PR with an eye toward memory usage, that would be greatly appreciated. If not, I can try to find some time next week to understand this better; maybe there are tools I can use or maybe I should just try running the code repeatedly to see if memory usage goes up.

Byron

Thanks so much for your work and the summary! It's pure insanity to see how incompletely documented an OS API seems to be in terms of ownership and how much of a minefield even simple tasks or even calls seem to be. Maybe I am just spoiled by Rust but I do firmly believe that the windows crates have to get to a point where all that is automatic and safe.

That said, I am approving the PR, but since my bar is so very low (compilation + tests are green) I think @ArturKovacs should be the one driving the merge :D.

rgwood · 2022-05-24T04:28:00Z

Agreed that the windows crates still need some work; I left some feedback. It sounds like the crates are still undergoing frequent breaking changes and they're not quite ready to commit to full documentation yet.

ArturKovacs

Just a tiny remark, but this PR looks good to me

ArturKovacs · 2022-05-24T17:42:19Z

src/windows.rs

-                original_parent: PathBuf::from(orig_loc),
-                time_deleted: date_deleted,
-            });
+        let recycle_bin = recycle_bin.assume_init().expect("not initialized");


The expect function panics if recycle_bin is None. I understand that this entire crate is useless if we fail to get the recycle bin, but in my opinion a lib-type crate should avoid panicking as much as possible.

I think this should instead be something like this:

let recycle_bin = recycle_bin.assume_init().ok_or(Error::Unknown { description: "SHGetKnownFolderItem gave NULL for FOLDERID_RecycleBinFolder".into() })?;

Good call, will make that change.

ArturKovacs · 2022-05-24T17:52:10Z

src/windows.rs

+                Some(item) => {
+                    let id = get_display_name(item, SIGDN_DESKTOPABSOLUTEPARSING)?;
+                    let name = get_display_name(item, SIGDN_PARENTRELATIVE)?;
+                    let item2: IShellItem2 = item.cast()?;


Wow I wouldn't have considered that it's possible to cast these IShellItems into IShellItem2. Well spotted!

ArturKovacs · 2022-05-24T18:45:14Z

I'm less sure about this code:

let item2: IShellItem2 = item.cast()?;
let original_location_variant: PROPVARIANT = item2.GetProperty(&SCID_ORIGINAL_LOCATION)?;
let original_location_bstr: BSTR = PropVariantToBSTR(&original_location_variant)?;

When you call cast on IShellItem, it increments the reference count, so it's correct to drop both of item and item2. (Because cast calls QueryInterface)

The PROPVARIANT seems to be a plain old struct, so there's nothing to drop there. (Because it's living on the stack which is free'd anyways.)

Dropping the BSTR is also correct because the documentation of PropVariantToBSTR says:

The calling application is responsible for using SysFreeString to release the BSTR

rgwood · 2022-05-24T19:02:03Z

@ArturKovacs Thanks!

IShellItem

When you call cast on IShellItem, it increments the reference count, so it's correct to drop both of item and item2. (Because cast calls QueryInterface)

IShellItem and IShellItem2 both wrap an IUnknown, and IUnknown has a Drop implementation that calls Release:

impl Drop for IUnknown {
    fn drop(&mut self) {
        unsafe {
            (self.vtable().Release)(core::mem::transmute_copy(self));
        }
    }
}

So I think that's taken care of for us.

PROPVARIANT

The PROPVARIANT seems to be a plain old struct, so there's nothing to drop there. (Because it's living on the stack which is free'd anyways.)

Under the hood PROPVARIANT has some ManuallyDrops which makes me wonder:

pub struct PROPVARIANT {
    pub Anonymous: PROPVARIANT_0,
}
...
pub union PROPVARIANT_0 {
    pub Anonymous: ::core::mem::ManuallyDrop<PROPVARIANT_0_0>,
    pub decVal: super::super::super::Foundation::DECIMAL,
}

BSTR

Dropping the BSTR is also correct because the documentation of PropVariantToBSTR says:

The calling application is responsible for using SysFreeString to release the BSTR

Thankfully it looks like windows-rs handles this for us:

impl ::core::ops::Drop for BSTR {
    fn drop(&mut self) {
        if !self.0.is_null() {
            unsafe { SysFreeString(self as &Self) }
        }
    }
}

ArturKovacs · 2022-05-24T19:28:30Z

Yeah I also think it's all good as it is.

Byron · 2022-05-25T02:43:36Z

@ArturKovacs Please feel free to merge. GitHub still thinks a change is requested, but in any case when merged I can cut a new release. Thanks.

Byron · 2022-05-25T08:03:20Z

Thanks a lot for merging! Here is the new release 🎉.

rgwood added 2 commits May 18, 2022 11:32

WIP upgrade windows crate

995265e

More Windows upgrade work - compiles now but tests fail

d39b143

rgwood commented May 19, 2022

View reviewed changes

rgwood added 2 commits May 21, 2022 13:21

WIP push forward

f107692

WIP this is so painful

bd3521d

ArturKovacs requested changes May 22, 2022

View reviewed changes

Tests passing! Still need to make sure I'm not leaking memory

3b8370c

rgwood force-pushed the upgrade-windows branch from b92d70a to 3b8370c Compare May 22, 2022 19:56

rgwood added 4 commits May 22, 2022 12:59

test bin

0f58566

delete test bin

9e362fe

clean up dev code, fix clippy

68edbf5

whoops let's be consistent about IShellItem vs IShellItem2

54574d1

rgwood added 5 commits May 22, 2022 13:44

remove unused windows crate features

eb6e8fa

Update TrashItem.id documentation

8ff6aa7

remove anyhow dependency

bbce1bf

clean up imports

ecb2f5e

Upgrade to new latest version of the windows crate

ad5b469

Byron mentioned this pull request May 23, 2022

windows crate upgrade #49

Closed

rgwood added 2 commits May 23, 2022 12:46

Clean up casts, imports, comments

25a1bdb

cargo fmt

788abac

rgwood changed the title ~~Upgrade windows crate - not quite finished~~ Upgrade windows crate May 23, 2022

rgwood marked this pull request as ready for review May 23, 2022 20:04

Byron approved these changes May 24, 2022

View reviewed changes

ArturKovacs reviewed May 24, 2022

View reviewed changes

rgwood added 2 commits May 24, 2022 11:16

replace panic with error

73341b4

Fix clippy

37d15b7

ArturKovacs merged commit d18f9d4 into Byron:master May 25, 2022

rgwood mentioned this pull request Nov 26, 2022

Upgrade windows crate from v0.37 to v0.43 #58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade windows crate #51

Upgrade windows crate #51

rgwood commented May 19, 2022

rgwood May 19, 2022

rgwood May 19, 2022

Byron May 20, 2022

ArturKovacs May 22, 2022

rgwood commented May 19, 2022

Byron commented May 20, 2022

rgwood commented May 21, 2022

ArturKovacs May 22, 2022

rgwood commented May 22, 2022 •

edited

Byron commented May 23, 2022

rgwood commented May 23, 2022

Byron left a comment

rgwood commented May 24, 2022

ArturKovacs left a comment

ArturKovacs May 24, 2022 •

edited

rgwood May 24, 2022

rgwood May 24, 2022

ArturKovacs May 24, 2022

ArturKovacs commented May 24, 2022

rgwood commented May 24, 2022

ArturKovacs commented May 24, 2022

Byron commented May 25, 2022

Byron commented May 25, 2022

Upgrade windows crate #51

Upgrade windows crate #51

Conversation

rgwood commented May 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgwood commented May 19, 2022

Byron commented May 20, 2022

rgwood commented May 21, 2022

Choose a reason for hiding this comment

rgwood commented May 22, 2022 • edited

Byron commented May 23, 2022

rgwood commented May 23, 2022

Takeaway

Byron left a comment

Choose a reason for hiding this comment

rgwood commented May 24, 2022

ArturKovacs left a comment

Choose a reason for hiding this comment

ArturKovacs May 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArturKovacs commented May 24, 2022

rgwood commented May 24, 2022

IShellItem

PROPVARIANT

BSTR

ArturKovacs commented May 24, 2022

Byron commented May 25, 2022

Byron commented May 25, 2022

rgwood commented May 22, 2022 •

edited

ArturKovacs May 24, 2022 •

edited