libgit2 / pygit2

Python bindings for libgit2
https://www.pygit2.org/
Other
1.58k stars 382 forks source link

filter: ensure GIL during pygit2_filter_cleanup #1259

Closed 0x2b3bfa0 closed 6 months ago

0x2b3bfa0 commented 6 months ago

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.

https://github.com/libgit2/pygit2/blob/8da921b4c2a6f8e64ad91e6395c0f3720df90083/src/pygit2.c#L322

https://github.com/libgit2/pygit2/blob/8da921b4c2a6f8e64ad91e6395c0f3720df90083/src/filter.c#L541-L546

https://github.com/libgit2/pygit2/blob/8da921b4c2a6f8e64ad91e6395c0f3720df90083/src/filter.c#L222-L234

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)

jdavid commented 6 months ago

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

0x2b3bfa0 commented 6 months ago

@jdavid, done!

0x2b3bfa0 commented 5 months ago

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