Prevent unregistering a filter that's still in use in a FilterList

Author: jorio

pygit2/__init__.py

@@ -286,7 +286,6 @@
     _cache_enums,
     discover_repository,
     filter_register,
-    filter_unregister,
     hash,
     hashfile,
     init_file_backend,
@@ -545,6 +544,27 @@ def clone_repository(
     return Repository._from_c(crepo[0], owned=True)
 
 
+def filter_unregister(name: str) -> None:
+    """
+    Unregister the given filter.
+
+    Note that the filter registry is not thread safe. Any registering or
+    deregistering of filters should be done outside of any possible usage
+    of the filters.
+
+    In particular, any FilterLists that use the filter must have been garbage
+    collected before you can unregister the filter.
+    """
+    from .filter import FilterList
+
+    if FilterList._is_filter_in_use(name):
+        raise RuntimeError(f"filter still in use: '{name}'")
+
+    c_name = to_bytes(name)
+    err = C.git_filter_unregister(c_name)
+    check_error(err)
+
+
 tree_entry_key = functools.cmp_to_key(tree_entry_cmp)
 
 settings = Settings()
@@ -610,7 +630,6 @@ def clone_repository(
     # Low Level API (not present in .pyi)
     'FilterSource',
     'filter_register',
-    'filter_unregister',
     'GIT_APPLY_LOCATION_BOTH',
     'GIT_APPLY_LOCATION_INDEX',
     'GIT_APPLY_LOCATION_WORKDIR',

pygit2/_pygit2.pyi

@@ -857,6 +857,5 @@ def reference_is_valid_name(refname: str) -> bool: ...
 def tree_entry_cmp(a: Object, b: Object) -> int: ...
 def _cache_enums() -> None: ...
 def filter_register(name: str, filter: type[Filter]) -> None: ...
-def filter_unregister(name: str) -> None: ...
 
 _OidArg = str | Oid

pygit2/decl/filter.h

@@ -11,6 +11,9 @@ typedef enum {
 	GIT_FILTER_ATTRIBUTES_FROM_COMMIT = ...,
 } git_filter_flag_t;
 
+int git_filter_unregister(
+	const char *name);
+
 int git_filter_list_load(
 	git_filter_list **filters,
 	git_repository *repo,

pygit2/filter.py

@@ -23,6 +23,9 @@
 # the Free Software Foundation, 51 Franklin Street, Fifth Floor,
 # Boston, MA 02110-1301, USA.
 
+from __future__ import annotations
+
+import weakref
 from collections.abc import Callable
 from typing import TYPE_CHECKING
 
@@ -116,16 +119,34 @@ def close(self, write_next: Callable[[bytes], None]) -> None:
 
 
 class FilterList:
+    _all_filter_lists: set[weakref.ReferenceType[FilterList]] = set()
+
     _pointer: GitFilterListC
 
     @classmethod
     def _from_c(cls, ptr: GitFilterListC):
         if ptr == ffi.NULL:
             return None
+
         fl = cls.__new__(cls)
         fl._pointer = ptr
+
+        # Keep track of this FilterList until it's garbage collected. This lets
+        # `filter_unregister` ensure the user isn't trying to delete a filter
+        # that's still in use.
+        ref = weakref.ref(fl, FilterList._all_filter_lists.remove)
+        FilterList._all_filter_lists.add(ref)
+
         return fl
 
+    @classmethod
+    def _is_filter_in_use(cls, name: str) -> bool:
+        for ref in cls._all_filter_lists:
+            fl = ref()
+            if fl is not None and name in fl:
+                return True
+        return False
+
     def __contains__(self, name: str) -> bool:
         if not isinstance(name, str):
             raise TypeError('argument must be str')

src/pygit2.c

@@ -345,31 +345,6 @@ filter_register(PyObject *self, PyObject *args, PyObject *kwds)
     return result;
 }
 
-PyDoc_STRVAR(filter_unregister__doc__,
-    "filter_unregister(name: str) -> None\n"
-    "\n"
-    "Unregister the given filter.\n"
-    "\n"
-    "Note that the filter registry is not thread safe. Any registering or\n"
-    "deregistering of filters should be done outside of any possible usage\n"
-    "of the filters.\n");
-
-PyObject *
-filter_unregister(PyObject *self, PyObject *args)
-{
-    const char *name;
-    Py_ssize_t size;
-    int err;
-
-    if (!PyArg_ParseTuple(args, "s#", &name, &size))
-        return NULL;
-    if ((err = git_filter_unregister(name)) < 0)
-        return Error_set(err);
-
-    Py_RETURN_NONE;
-}
-
-
 static void
 forget_enums(void)
 {
@@ -441,7 +416,6 @@ PyMethodDef module_methods[] = {
     {"reference_is_valid_name", reference_is_valid_name, METH_O, reference_is_valid_name__doc__},
     {"tree_entry_cmp", tree_entry_cmp, METH_VARARGS, tree_entry_cmp__doc__},
     {"filter_register", (PyCFunction)filter_register, METH_VARARGS | METH_KEYWORDS, filter_register__doc__},
-    {"filter_unregister", filter_unregister, METH_VARARGS, filter_unregister__doc__},
     {"_cache_enums", _cache_enums, METH_NOARGS, _cache_enums__doc__},
     {NULL}
 };

test/test_filter.py

@@ -1,4 +1,5 @@
 import codecs
+import gc
 from collections.abc import Callable, Generator
 from io import BytesIO
 
@@ -56,32 +57,34 @@ class _UnmatchedFilter(_Rot13Filter):
     attributes = 'filter=rot13'
 
 
+def _filter_fixture(name: str, filter: type[Filter]) -> Generator[None, None, None]:
+    pygit2.filter_register(name, filter)
+    yield
+
+    # Collect any FilterLists that may use this filter before unregistering it
+    gc.collect()
+
+    pygit2.filter_unregister(name)
+
+
 @pytest.fixture
 def rot13_filter() -> Generator[None, None, None]:
-    pygit2.filter_register('rot13', _Rot13Filter)
-    yield
-    pygit2.filter_unregister('rot13')
+    yield from _filter_fixture('rot13', _Rot13Filter)
 
 
 @pytest.fixture
 def passthrough_filter() -> Generator[None, None, None]:
-    pygit2.filter_register('passthrough-rot13', _PassthroughFilter)
-    yield
-    pygit2.filter_unregister('passthrough-rot13')
+    yield from _filter_fixture('passthrough-rot13', _PassthroughFilter)
 
 
 @pytest.fixture
 def buffered_filter() -> Generator[None, None, None]:
-    pygit2.filter_register('buffered-rot13', _BufferedFilter)
-    yield
-    pygit2.filter_unregister('buffered-rot13')
+    yield from _filter_fixture('buffered-rot13', _BufferedFilter)
 
 
 @pytest.fixture
 def unmatched_filter() -> Generator[None, None, None]:
-    pygit2.filter_register('unmatched-rot13', _UnmatchedFilter)
-    yield
-    pygit2.filter_unregister('unmatched-rot13')
+    yield from _filter_fixture('unmatched-rot13', _UnmatchedFilter)
 
 
 def test_filter(testrepo: Repository, rot13_filter: Filter) -> None:
@@ -153,3 +156,29 @@ def test_filterlist_crlf(testrepo: Repository) -> None:
 
     with pytest.raises(TypeError):
         1234 in fl  # type: ignore
+
+
+def test_filterlist_rot13(testrepo: Repository, rot13_filter: Filter) -> None:
+    fl = testrepo.load_filter_list('hello.txt')
+    assert fl is not None
+    assert 'rot13' in fl
+
+
+def test_filterlist_dangerous_unregister(testrepo: Repository) -> None:
+    pygit2.filter_register('rot13', _Rot13Filter)
+
+    fl = testrepo.load_filter_list('hello.txt')
+    assert fl is not None
+    assert len(fl) == 1
+    assert 'rot13' in fl
+
+    # Unregistering a filter that's still in use in a FilterList is dangerous!
+    # Our built-in check (that raises RuntimeError) may avert a segfault.
+    with pytest.raises(RuntimeError):
+        pygit2.filter_unregister('rot13')
+
+    # Delete any FilterLists that use the filter, and only then is it safe
+    # to unregister the filter.
+    del fl
+    gc.collect()
+    pygit2.filter_unregister('rot13')