Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CreateEventEx parameters are misleading #15

Closed
weltkante opened this issue Aug 29, 2015 · 4 comments
Closed

CreateEventEx parameters are misleading #15

weltkante opened this issue Aug 29, 2015 · 4 comments
Labels

Comments

@weltkante
Copy link

All examples use CreateEventEx(nullptr, FALSE, FALSE, EVENT_ALL_ACCESS) but the types used do not match the types in the method declaration. It still works with these exact parameters because FALSE == NULL == 0 but is misleading to the reader.

It should be either CreateEventEx(nullptr, nulltrp, 0, EVENT_ALL_ACCESS) or CreateEvent(nullptr, FALSE, FALSE, nullptr)

@bobbrow bobbrow added the bug label Aug 31, 2015
bobbrow added a commit that referenced this issue Aug 31, 2015
Incorrect parameter used for 2nd argument of CreateEventEx.
@bobbrow
Copy link
Member

bobbrow commented Aug 31, 2015

Fixed in develop branch. Will merge to master with next release.

@weltkante
Copy link
Author

Not really fixed, but good enough. What follows is some additional thoughts on the matter, not necessarily correct, nor really that important. I wouldn't mind if you leave the samples as it currently is in the dev branch since I understand that event security isn't the focus of the samples.

CreateEventEx(nullptr, nullptr, FALSE, EVENT_ALL_ACCESS) is somewhat better but still not entirely right. The 3rd parameter is a bitflag not a boolean. But at least it'd do something sensible if someone passed TRUE.

Actually I'm not entirely sure why you need to use CreateEventEx instead of CreateEvent in the first place, something to do with the gpu driver needing additional access rights? The MSDN docs of DX12 suggest to use CreateEvent and not CreateEventEx, which works just fine according to my tests. Also the original (wrong) parameters of the samples hint that your code originally used CreateEvent and then someone changed it to CreateEventEx to specify access rights, but didn't realize the signature is totally different.

From what I understand the handle returned by CreateEvent already has full access, as does the handle returned from CreateEventEx, regardless of what you specify, the initial handle always has full access. The access rights you specify, I believe, is intended for other sites accessing the event. Since you don't give the event a name that could only happen by handle inheritance (or possibly handle duplication, not sure). So basically you give everyone full access to that event, as long as he can get a hand on the handle in some way. Not really best practice, but I doubt someone can do much harm by getting control over these event handles. Maybe crash your app/game by messing with the fence waits.

As I said, thats just some thoughts, and I'm no expert on handle security, so some of this might be wrong. It's just a bit of a mystery to me why the samples use the more complex CreateEventEx and have to specify additional security stuff, when it seems to work perfectly fine (for me) going with the simple CreateEvent and default security settings.

@bobbrow
Copy link
Member

bobbrow commented Aug 31, 2015

sigh. And it's only Monday. I agree that there's no added benefit to using CreateEventEx. I switched the code back to using CreateEvent.

@weltkante
Copy link
Author

👍 Thanks, and sorry for bothering with these minor details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants