Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Creds serialization and deserialization #45
Creds serialization and deserialization #45
Changes from all commits
2b21a07
8519cab
eb38ca4
4677881
4514ccb
8b084d1
c1f86bc
0bfb065
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm somewhat weary about using
free
here explicitly. The docs for krb5_unmarshal_credentials state thatI can see the actual implementation essentially calls
krb5_free_creds_content
and then callsfree
. https://github.com/krb5/krb5/blob/354f176ba6d6cc544e1c15712a13f9c006ca605d/src/lib/krb5/krb/kfree.c#L213-L219While this replicates that same logic that's more of an implementation detail and not a public API. We should try to avoid freeing data using
free
if we don't control how it was allocated.Instead I'm wondering if we should add another
cdef
field onCreds
that can be used to control how the struct is deallocated to work with this new scenario. For example (the field it actually defined in the pxd file, the comment is more for documentation)The
krb5_free_creds
would also need to be defined in the file but seems like Heimdal has it so it shouldn't be a problem.The other benefit of this is we don't need to copy the data we can just do something like
Do you see any problem with this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree I stepped onto thin ice here... The point is a pointer-to-pointer used by
krb5_unmarshal_credentials()
, it unconditioanaly allocates the structure using itsk5alloc()
wrapper and writes the allocated pointer tocreds_out
. There is just no way to not allocate some heap memory, so it has to be free'd.https://github.com/krb5/krb5/blob/354f176ba6d6cc544e1c15712a13f9c006ca605d/src/lib/krb5/ccache/ccmarshal.c#L557
There are might be two ways to avoid shortcuts like mine, right after
krb5_unmarshal_credentials()
self.raw
and deallocate everything just returned withkrb5_free_creds()
as original API prescribes. Our existingraw
will contain a deep copy to be free'd withkrb5_free_cred_contents()
in__deallocate__
.self.raw
, save the alloc'ed pointer as_raw_ptr
and indeed callkrb5_free_creds
instead ofkrb5_free_cred_contents
in__deallocate__
. Our existingraw
will remain the shallow copy to keep other things working.The problem with (1) is that deep copy function
krb5_copy_creds()
behaves the same askrb5_unmarshal_credentials()
and allocates the new structure.Are you ok with having additional
_raw_ptr
member inCreds
class?Or any better ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, sorry I didn't see it was a
krb5_creds**
argument. I thought it was justkrb5_creds*
, now the API makes more sense.