Skip to content

Signature cleanups#4122

Merged
ethomson merged 3 commits intolibgit2:masterfrom
pks-t:pks/signature-dbl-free
Feb 13, 2017
Merged

Signature cleanups#4122
ethomson merged 3 commits intolibgit2:masterfrom
pks-t:pks/signature-dbl-free

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Feb 13, 2017

Some findings related to #4118

When `git_buf_sanitize` gets called, it converts a buffer with NULL
content to be correctly initialized. This is done by pointing it to
`git_buf__initbuf`. While the method's documentation states this
clearly, it may also lead to the conclusion that it will do the same to
buffers which do _not_ have NULL contents.

Clarify behavior when passing a buffer with non-NULL contents, where
`git_buf_sanitize` will ensure that the contents are `\0`-terminated.
*
* @param out the buffer to fill
* @param out the buffer to fill; existing content will be
* overwritten
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ugh. Will fix

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed

The functions `git_commit_header_field` and
`git_commit_extract_signature` both receive buffers used to hand back
the results to the user. While these functions called `git_buf_sanitize`
on these buffers, this is not the right thing to do, as it will simply
initialize or zero-terminate passed buffers. As we want to overwrite
contents, we instead have to call `git_buf_clear` to completely reset
them.
When extracting a commit's signature, we first free the object and only
afterwards put its signature contents into the result buffer. This works
in most cases - the free'd object will normally be cached anyway, so we
only end up decrementing its reference count without actually freeing
its contents. But in some more exotic setups, where caching is disabled,
this can definitly be a problem, as we might be the only instance
currently holding a reference to this object.

Fix this issue by first extracting the contents and freeing the object
afterwards only.
@pks-t pks-t force-pushed the pks/signature-dbl-free branch from fe9d122 to ade0d9c Compare February 13, 2017 12:50
@ethomson ethomson merged commit a59545d into libgit2:master Feb 13, 2017
@pks-t pks-t deleted the pks/signature-dbl-free branch February 14, 2017 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants