Skip to content
This repository has been archived by the owner on Jul 26, 2023. It is now read-only.

Add kernel32!SetHandleInformation and kernel32!GetHandleInformation #440

Merged
merged 6 commits into from
Oct 27, 2019
Merged

Add kernel32!SetHandleInformation and kernel32!GetHandleInformation #440

merged 6 commits into from
Oct 27, 2019

Conversation

mckunda
Copy link
Contributor

@mckunda mckunda commented Oct 24, 2019

  • P/Invokes for kernel32!SetHandleInformation and kernel32!GetHandleInformation
  • HandleFlags enum

@mckunda
Copy link
Contributor Author

mckunda commented Oct 25, 2019

@AArnott if you have any ideas on how to test these, please let me know

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. You've obviously taken care t
Thank you for your contribution! You obviously were careful to conform to our conventions and I appreciate it.

I've called out some changes to be made, then we'll be happy to merge.

/// </remarks>
[DllImport(api_ms_win_core_handle_l1_1_0, SetLastError = true)]
[return: MarshalAs(UnmanagedType.Bool)]
public static extern bool SetHandleInformation([In] SafeHandle hObject, HandleFlags dwMask, HandleFlags dwFlags);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The [In] is not ordinarily necessary. Unless there's some reason you had to add it, please remove.

/// </returns>
[DllImport(api_ms_win_core_handle_l1_1_0, SetLastError = true)]
[return: MarshalAs(UnmanagedType.Bool)]
public static extern unsafe bool GetHandleInformation([In] SafeHandle hObject, [Out] out HandleFlags* lpdwFlags);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove [In] and [Out] here as well. The out modifier as well should be removed since this isn't a pointer to a pointer. It's just a pointer that we must pass in so that Win32 knows where to write the DWORD. I'm pretty sure this would fail as written here.

Also please add [Friendly(FriendlyFlags.Out)] to the HandleFlags* parameter so that an overload is generated that has an out HandleFlags parameter.

@AArnott
Copy link
Collaborator

AArnott commented Oct 26, 2019

Regarding how to test this (which seems like a good idea), you could create a ManualResetEvent in the unit test and then obtain its Handle property, and call the 2 new methods on that handle, just to see if no exception is thrown.

@mckunda
Copy link
Contributor Author

mckunda commented Oct 26, 2019

Thanks for the feedback! Hope I understood you correctly. Also, I'd like to point out two things:

  1. Is it okay that from GetHandleInformation we may get a result that's not defined in HandleFlags, e.g. 0x00000003, which is HANDLE_FLAG_INHERIT | HANDLE_FLAG_PROTECT_FROM_CLOSE? Honestly, I don't quite understand that.
  2. Also I don't know if the methods I added are store-banned, because I have no clue how to check that. Could you please explain?

@mckunda mckunda requested a review from AArnott October 26, 2019 17:06
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Can you touch up the test methods slightly?

src/Kernel32.Tests/Kernel32Facts.cs Outdated Show resolved Hide resolved
src/Kernel32.Tests/Kernel32Facts.cs Outdated Show resolved Hide resolved
@AArnott
Copy link
Collaborator

AArnott commented Oct 26, 2019

You can check for store-banned status from the Microsoft API docs for the method:
image

So that's a good point: we need to move these methods to the storebanned folder so they don't cause store app rejection.

@AArnott
Copy link
Collaborator

AArnott commented Oct 26, 2019

Is it okay that from GetHandleInformation we may get a result that's not defined in HandleFlags

Yes: that's the nature of [Flags] enums. Folks would test for individual flags.

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move to store banned folder.

@mckunda
Copy link
Contributor Author

mckunda commented Oct 26, 2019

Thanks for the help! Hope now it's all OK.

@mckunda mckunda requested a review from AArnott October 26, 2019 18:06
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@AArnott AArnott merged commit 63daa40 into dotnet:master Oct 27, 2019
@mckunda mckunda deleted the feature/add-get-set-handle-info branch October 27, 2019 19:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants