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

Safer handling of string arrays #487

Merged
merged 1 commit into from
Feb 6, 2015
Merged

Conversation

carlosmn
Copy link
Member

@carlosmn carlosmn commented Feb 6, 2015

We need to keep hold of the strings which we create. We must also hold
on to the array of strings which we assing to our git_strarray.

We were not doing the latter, which meant that our strings may have been
freed too early, leaving us with with memory access errors (though often
not leading to a crash due to the custom allocator in python).

As we need to keep hold of two/three pieces of information, this looks
like a good place to introduce a context manager. This allows us to
keep these pointers alive without burdening the call sites with a return
of multiple objects they have no use for.

We need to keep hold of the strings which we create. We must also hold
on to the array of strings which we assing to our git_strarray.

We were not doing the latter, which meant that our strings may have been
freed too early, leaving us with with memory access errors (though often
not leading to a crash due to the custom allocator in python).

As we need to keep hold of two/three pieces of information, this looks
like a good place to introduce a context manager. This allows us to
keep these pointers alive without burdening the call sites with a return
of multiple objects they have no use for.
@carlosmn
Copy link
Member Author

carlosmn commented Feb 6, 2015

@bochecha can you try with this? It should fix #477.

@bochecha
Copy link
Contributor

bochecha commented Feb 6, 2015

It does. 👍

https://koji.fedoraproject.org/koji/taskinfo?taskID=8842754

Thanks a lot.

@jdavid jdavid merged commit 0ba17a5 into libgit2:master Feb 6, 2015
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.

4 participants