-
Notifications
You must be signed in to change notification settings - Fork 101
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
bitlockerConversionStatus broken by type change #43
Comments
Apologies @ItsMattL , I hadn’t spotted that in review, it was a poor review on my part and we haven’t updated to the tip of master for a while. You may well be a more active user than I am myself so I’d like your opinion: would you class anything in that commit as useful, or would you revert the whole thing? |
Yeah no worries. The constants certainly seem useful. I'll also say I haven't validated the types on all of the values to see if uint is correct for any of them, so ideally someone else could do a verification on the behavior here and make sure it's not something weird in our version of ole or something. The Microsoft docs are only so reliable in my experience, at least when being translated through go-ole. Sometimes the types match exactly, other times you get these weird mismatches. I'm curious if this actually worked for the original contributor, or if it was entirely based on the API docs. |
Ditto on the Microsoft docs not matching what types go-ole seems to return, took a lot of trial an error when first writing this stuff. I’ll be honest and say that at work, I’m too tied up with other projects and will likely only get a few days later in the year to properly do anything related to this, so I can’t fully commit the time to resolving this at the minute, as I think you are right that it’ll need some proper testing & validation if we’re going to change the types. I’ll revert the commit when I’m back in the office so the tip of master is back to something that had been stable for a fairly long time. In the meantime, @safinsingh , are you interested in joining the conversation and possibly fixing your contribution? |
I believe I mentioned in my pull request that the return value should be an unsigned integer according to these Microsoft docs: https://docs.microsoft.com/en-us/windows/win32/secprov/getconversionstatus-win32-encryptablevolume. I would make sure there is no issue with the environment and that |
Just had a bit more time to look into this and I was wrong, we are using a version of I believe we are just using a different function that wasn't touched by this change, I can see in the history, there was one change where I swapped some values to I'm going from memory here, but we don't really touch this code unless something breaks in our internal app & we have to spend time on it, so I believe we started getting weird behaviour in a particular version of Windows 10 (we use Windows 10 x64 exclusively), hence the change to Another Windows 10 update seemed to change things back to needing I never touched the code you are both referencing as I don't believe we are using it in our app, but perhaps it needs the same kind of check to support both? I agree that it would be really helpful to confirm if you are both seeing inconsistent behaviour:
|
It looks like commit f838a77 inadvertently broke the bitlockerConversionStatus by changing the types from int32 to uint32. Our dependent code started returning the "unable to get conversion status while getting BitLocker info; the volume is locked" error. Further investigation suggested this was due to !ok triggering on the type conversion, rather than on the value itself.
It should be possible to confirm this by checking the behavior with reflect.TypeOf:
fmt.Printf("Result type: %s", reflect.TypeOf(statusResultRaw.Value()))
The text was updated successfully, but these errors were encountered: