Add DP API Cache #14
Add DP API Cache #14
Conversation
msal_extensions/_windows.py
Outdated
| pb_data = self.pbData | ||
| buffer = ctypes.c_buffer(cb_data) | ||
| _memcpy(buffer, pb_data, cb_data) | ||
| _local_free(pb_data) |
There was a problem hiding this comment.
I know this LocalFree() pattern also showed up in that stackoverflow question which we modeled from. Out of curiosity, is such free() here really necessary/desirable? The current way seems making raw() unpredictable when being accidentally called twice?
There was a problem hiding this comment.
So, the _local_free is definitely necessary as we don't want to create a memory leak! However, you're totally right that it should be safe to call this method twice in a way it just isn't right now. I'll fix that up.
There was a problem hiding this comment.
So, I'm going to have to walk this change back. I got to a Windows machine and ran the tests, and curiously pulling this object out into the destructor causes a seg-fault that crashes the host Python process with it.
There was a problem hiding this comment.
If that is the case, can we have one more try, pun not intended, to move that _local_free() out of raw(), and put it into protect() in this manner? On paper, it should work.
def protect(self, ...):
...
try:
return result.raw()
finally:
_local_free(result.pbData)There was a problem hiding this comment.
Love it :) I'll give it another whack in the morning.
There was a problem hiding this comment.
So the trick ended up being that LocalFree MUST NOT be called memory allocated by ctypes.create_string_buffer, but MUST be called on memory allocated by the CryptProtectData and CryptUnprotectData functions. It just so happened that I only ever called raw on the latter, and never called raw for the former. This means that using try & finally allows us to manage the lifetime of only specific structs, where as moving it into a __del__ method was breaking the contract.
Long story short, this is fixed in 161e6cf 2a25b5f. On the otherhand, a4efa65 80e735c can be ignored.
edit: Fixing commit-id's after rebase.
There was a problem hiding this comment.
This reverts commit b9becbc. Once tested on a Windows box (read, actually tested) it demonstrated that this change causes a seg-fault and crash on Windows.
Putting LocalFree in `raw` in the first place was a mistake, and represented a fundamental misunderstanding of DataBlob's memory management story. The memory it points at must be managed independently of the DataBlob's lifecycle.
2 participants
Closes #1