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

filter: ensure GIL during pygit2_filter_cleanup #1259

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

0x2b3bfa0
Copy link
Contributor

@0x2b3bfa0 0x2b3bfa0 commented Dec 20, 2023

All the calls to pygit2_filter_payload_free need the GIL to call Py_DECREF on the pygit2_filter_payload objects, but the filter.cleanup callback was missing PyGILState_Ensure and causing a crash.

filter->filter.cleanup = pygit2_filter_cleanup;

pygit2/src/filter.c

Lines 541 to 546 in 8da921b

void pygit2_filter_cleanup(git_filter *self, void *payload)
{
struct pygit2_filter_payload *pl = (struct pygit2_filter_payload *)payload;
pygit2_filter_payload_free(pl);
}

pygit2/src/filter.c

Lines 222 to 234 in 8da921b

static void pygit2_filter_payload_free(
struct pygit2_filter_payload *payload)
{
if (payload == NULL)
return;
if (payload->py_filter != NULL)
Py_DECREF(payload->py_filter);
if (payload->src != NULL)
Py_DECREF(payload->src);
if (payload->stream != NULL)
free(payload->stream);
free(payload);
}

Minimal reproducible example

$ git init && touch file && git add file && git commit file --message=example && touch file # dirty repository
Initialized empty Git repository in /.../.git/
[main (root-commit) ...] example
 Author: ...
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file
$ python <<< 'import pygit2; pygit2.filter_register("filter", pygit2.Filter); pygit2.Repository(".").diff()'
Fatal Python error: PyThreadState_Get: the function must be called with the GIL held, but the GIL is released (the current Python thread state is NULL)
Python runtime state: initialized

Current thread 0x00007f08ed342740 (most recent call first):
  File "/workspaces/studio/pygit2/pygit2/index.py", line 250 in diff_to_workdir
  File "/workspaces/studio/pygit2/pygit2/repository.py", line 488 in diff
  File "<stdin>", line 1 in <module>

Extension modules: pygit2._pygit2, _cffi_backend (total: 2)
Aborted (core dumped)

Stack trace

(gdb) py-bt
Traceback (most recent call first):
  <built-in method git_diff_index_to_workdir of _cffi_backend.Lib object at remote 0x7ffff7609710>
  File "/.../pygit2/index.py", line 250, in diff_to_workdir
    err = C.git_diff_index_to_workdir(cdiff, repo._repo, self._index,
  File "/.../pygit2/repository.py", line 488, in diff
    return self.index.diff_to_workdir(*opt_values)
  File "<string>", line 1, in <module>
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at /.../sysdeps/unix/sysv/linux/raise.c:50
...
#4  0x0000000000489a8e in _Py_FatalErrorFunc (func=0x898c80 <__func__.18925.lto_priv.0> "PyThreadState_Get", 
    msg=0x8f9400 "the function must be called with the GIL held, but the GIL is released (the current Python thread state is NULL)")
    at /.../Python/pylifecycle.c:2823
#5  0x00000000004a65c4 in _Py_FatalError_TstateNULL (func=<optimized out>) at /.../Python/ceval.c:330
#6  0x00000000004df357 in _Py_EnsureFuncTstateNotNULL (tstate=0x0, func=<optimized out>) at /.../Include/internal/pycore_pystate.h:100
#7  PyThreadState_Get () at /.../Python/pystate.c:1204
#8  subtype_dealloc (self=<Filter() at remote 0x7ffff6f56110>) at /.../Objects/typeobject.c:1360
#9  0x00007ffff7555b55 in Py_DECREF (op=<optimized out>) at /usr/include/python3.11/object.h:538
#10 pygit2_filter_payload_free (payload=0xccac80) at src/filter.c:228
#11 0x00007ffff745649b in git_filter_list_free () from /usr/local/lib/libgit2.so.1.7
#12 0x00007ffff744a28d in git_diff.oid_for_entry () from /usr/local/lib/libgit2.so.1.7
#13 0x00007ffff744be7a in git_diff.from_iterators () from /usr/local/lib/libgit2.so.1.7
#14 0x00007ffff744c621 in git_diff_index_to_workdir () from /usr/local/lib/libgit2.so.1.7
#15 0x00007ffff6fec73f in _cffi_f_git_diff_index_to_workdir (self=<optimized out>, args=<optimized out>)
    at /.../pygit2._libgit2.c:6139
#16 0x00000000005ed370 in cfunction_call (
    func=<built-in method git_diff_index_to_workdir of _cffi_backend.Lib object at remote 0x7ffff7609710>, args=<optimized out>, 
    kwargs=<optimized out>) at /.../Objects/methodobject.c:553
#17 0x0000000000639d1d in _PyObject_MakeTpCall (tstate=0xad0d78 <_PyRuntime+166328>, 
    callable=<built-in method git_diff_index_to_workdir of _cffi_backend.Lib object at remote 0x7ffff7609710>, args=<optimized out>, 
    nargs=<optimized out>, keywords=0x0) at /.../Objects/call.c:214
...
#41 0x00000000006691ce in _start () at /.../Parser/parser.c:1378

(Closes iterative/studio#8623)

@0x2b3bfa0 0x2b3bfa0 changed the title Ensure GIL during pygit2_filter_cleanup filter: ensure GIL during pygit2_filter_cleanup Dec 20, 2023
🎹 Ha! Zweifelst du an meiner Treue? ⛵
@jdavid
Copy link
Member

jdavid commented Dec 20, 2023

Thanks @0x2b3bfa0 ; could you add a unit test?

@0x2b3bfa0

This comment was marked as resolved.

@jdavid jdavid merged commit 5cabcfe into libgit2:master Dec 20, 2023
6 checks passed
@0x2b3bfa0 0x2b3bfa0 deleted the patch-3 branch December 20, 2023 17:02
@0x2b3bfa0
Copy link
Contributor Author

0x2b3bfa0 commented Jan 23, 2024

Note for others: we've been running the following commands on our production containers for a month, while waiting for the next pygit2 release, and they work like a charm.

apt install --yes cmake
curl --location https://github.com/libssh2/libssh2/archive/refs/tags/libssh2-1.11.0.tar.gz | tar --extract --gzip
cmake -S libssh2-* -B libssh2-* && cmake --build libssh2-* --target install && rm --recursive libssh2-*
curl --location https://github.com/libgit2/libgit2/archive/refs/tags/v1.7.1.tar.gz | tar --extract --gzip
cmake -S libgit2-* -B libgit2-* -D USE_SSH=ON && cmake --build libgit2-* --target install && rm --recursive libgit2-*
ldconfig
pip install --force-reinstall git+https://github.com/libgit2/pygit2.git@5cabcfe08b58036f90e6da5761c9b19fdb3cbc6b

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