Repository: sphinx-doc/sphinx

Issue:
[bug] `sphinx.config.is_serializable` is not safe against circular references
### Describe the bug

Sphinx Version: 7.1.2

We are running into a RecursionError that impedes Sphinx's ability to process documentation content and generate output.  That error is encountered during execution of the is_serializable function defined in sphinx/config.py.

Given that our documentation suite is very large and that each of our documents can be hundreds of pages long, we require flexibility in defining our documents' contents.  We employ a combination of .yml (key:value pairs) and .j2 (content templates) files to build the content that is fed to Sphinx for pickling and generating output.

Since we support several different system configurations, we must vary the number of and data for each of the Jinja contexts we define for building the content that is fed to Sphinx.  In rudimentary terms, a main context is composed of its core block of data plus two lists: a) contexts_list which contains the data from sibling contexts, and b) additional_contexts_list which contains the data from children contexts.  We came up with that scheme to allow all contexts to be visible at the same time and be able to collect data from all of them when building a document.  We have been using this approach for the past 6 years while using older versions of Sphinx.

It is when encountering either our contexts_list or additional_contexts_list that the RecursionError is produced.  Forcing Sphinx 7.1.2 to bypass the is_serializable function or running the exact same data with an older Sphhinx version that does not have that check/function allows Sphinx to successfully generate the .tex and .pdf files we are after.

Each sibling or child context block of data can be estimated at below 10MB and above 1MB.

Can is_serializable be modified to handle the approach from above?  ALTERNATIVELY, does Sphinx provide a way to access data from another context when working within one context?

### How to Reproduce

I cannot copy-paste the details or amount of technical data that makes up each environment within either the contexts_list or the additional_contexts_list in here.  The proprietary nature and volume of the data tie my hands.

As questions come up, I can work with someone to answer them.

The high-level command we issue is "sphinx-build -b latex -d build/doctrees source build/latex".

### Environment Information

```text
We are running Gitlab with a runner built using the "sphinxdoc/sphinx-latexpdf" image from Docker Hub.  When the StopIteration issue is encountered and Sphinx dies, Gitlab automatically performs cleanup and removes the container (i.e., the runner).  Attempting to execute "sphinx-build --bug-report" does not pan out in that situation.

Below is an abbreviated stack trace of the error.

pickling environment... failed
[app] emitting event: 'build-finished'(RecursionError('maximum recursion depth exceeded in __instancecheck__'),)
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/sphinx/cmd/build.py", line 290, in build_main
    app.build(args.force_all, args.filenames)
  File "/usr/local/lib/python3.11/site-packages/sphinx/application.py", line 351, in build
    self.builder.build_update()
  File "/usr/local/lib/python3.11/site-packages/sphinx/builders/__init__.py", line 287, in build_update
    self.build(['__all__'], to_build)
  File "/usr/local/lib/python3.11/site-packages/sphinx/builders/__init__.py", line 327, in build
    pickle.dump(self.env, f, pickle.HIGHEST_PROTOCOL)
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 323, in __getstate__
    if key.startswith('_') or not is_serializable(value):
                                  ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 47, in is_serializable
    if not is_serializable(key) or not is_serializable(value):
                                       ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 47, in is_serializable
    if not is_serializable(key) or not is_serializable(value):
                                       ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 50, in is_serializable
    return all(is_serializable(i) for i in obj)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 50, in <genexpr>
    return all(is_serializable(i) for i in obj)
               ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 47, in is_serializable
    if not is_serializable(key) or not is_serializable(value):
                                       ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 47, in is_serializable
    if not is_serializable(key) or not is_serializable(value):
                                       ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 50, in is_serializable
    return all(is_serializable(i) for i in obj)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 50, in <genexpr>
    return all(is_serializable(i) for i in obj)
               ^^^^^^^^^^^^^^^^^^


< ...line 47 and line 50 continue to be called a very large number of times... >


  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 50, in <genexpr>
    return all(is_serializable(i) for i in obj)
               ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 47, in is_serializable
    if not is_serializable(key) or not is_serializable(value):
                                       ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 47, in is_serializable
    if not is_serializable(key) or not is_serializable(value):
           ^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sphinx/config.py", line 43, in is_serializable
    if isinstance(obj, UNSERIALIZABLE_TYPES):
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
RecursionError: maximum recursion depth exceeded in __instancecheck__
Recursion error:
maximum recursion depth exceeded in __instancecheck__
This can happen with very large or deeply nested source files. You can carefully increase the default Python recursion limit of 1000 in conf.py with e.g.:
    import sys; sys.setrecursionlimit(1500)
```


### Sphinx extensions

```python
We are running Gitlab with a runner built using the "sphinxdoc/sphinx-latexpdf" image from Docker Hub.  When the StopIteration issue is encountered and Sphinx dies, Gitlab automatically performs cleanup and removes the container (i.e., the runner).  Attempting to execute "sphinx-build --bug-report" does not pan out in that situation.


On the "sphinxdoc/sphinx-latexpdf" image's Debian OS, we perform an update, an upgrade, and add packages.

  - apt-get update
  - apt-get upgrade -y
  - apt-get install -y git
  - apt-get install -y unzip
  - apt-get install -y wget
  - apt-get install -y texlive
  - apt-get install -y texlive-bibtex-extra
  - apt-get install -y texlive-font-utils
  - apt-get install -y texlive-lang-english
  - kpsewhich -var-value=TEXMFLOCAL
  - kpsewhich -var-value=TEXMFDIST
  - unzip -qo acrotex.zip
  - cd acrotex
  - |+
    for acrofile in $(ls -1 *.ins | egrep -v 'exerquiz|acrotex')
    do
      latex $acrofile
    done
    mkdir -p /usr/share/texlive/texmf-dist/tex/latex/acrotex
    cp *.sty /usr/share/texlive/texmf-dist/tex/latex/acrotex
    cp *.cfg /usr/share/texlive/texmf-dist/tex/latex/acrotex
    cp *.def /usr/share/texlive/texmf-dist/tex/latex/acrotex
  - cd ..
  - mktexlsr /usr/share/texlive/texmf-dist
  - rm -rf acrotex*

We also need to pip install a number of Python packages that are required for our documentation to be generated.

  - python -m pip install sphinx-autobuild
  - python -m pip install sphinx-git
  - python -m pip install sphinxcontrib-actdiag
  - python -m pip install sphinxcontrib-ansibleautodoc
  - python -m pip install sphinxcontrib-autoprogram
  - python -m pip install sphinxcontrib-blockdiag
  - python -m pip install sphinxcontrib-confluencebuilder
  - python -m pip install sphinxcontrib-jsonschema
  - python -m pip install sphinxcontrib-jupyter
  - python -m pip install sphinxcontrib-nwdiag
  - python -m pip install sphinxcontrib-plantuml
  - python -m pip install sphinxcontrib-seqdiag
  - python -m pip install sphinxcontrib-websupport
  - python -m pip install ciscoconfparse
  - python -m pip install decorator
  - python -m pip install enum34
  - python -m pip install funcparserlib
  - python -m pip install gitdb
  - python -m pip install Jinja2==3.0.3
  - python -m pip install jupyter-core
  - python -m pip install netaddr
  - python -m pip install plantuml
  - python -m pip install python-dateutil
  - python -m pip install pyyaml
  - python -m pip install sets
  - python -m pip install tablib
```


### Additional context

_No response_

Discussion:
picnixz: Forgot to add a CHANGES entry but I'll do it next week (I'm leaving for today).
picnixz: I may also add a simple test for `is_serializable` without circular references, just to check that I did not mess up a possible condition.
picnixz: I'm not entirely confident that the recursive guard based on the `id` is really sufficient but I cannot come up with a counter-example. Intuitively, we are not modifying the objects being tested during the check so even if they are mutable containers, since we are only checking their type, it should not be an issue. However, if the collections happens to be mutated (without changing its length) for whatever reason, then there might be an issue.

Now, I will not make it entirely foolproof because, as the name indicates, I don't want to protect against fools too much.

Patch:
diff --git a/sphinx/config.py b/sphinx/config.py
index ef1eaa848..9e49423d5 100644
--- a/sphinx/config.py
+++ b/sphinx/config.py
@@ -51,17 +51,30 @@ class ConfigValue(NamedTuple):
     rebuild: _ConfigRebuild
 
 
-def is_serializable(obj: Any) -> bool:
+def is_serializable(obj: object, *, _recursive_guard: frozenset[int] = frozenset()) -> bool:
     """Check if object is serializable or not."""
     if isinstance(obj, UNSERIALIZABLE_TYPES):
         return False
-    elif isinstance(obj, dict):
+
+    # use id() to handle un-hashable objects
+    if id(obj) in _recursive_guard:
+        return True
+
+    if isinstance(obj, dict):
+        guard = _recursive_guard | {id(obj)}
         for key, value in obj.items():
-            if not is_serializable(key) or not is_serializable(value):
+            if (
+                not is_serializable(key, _recursive_guard=guard)
+                or not is_serializable(value, _recursive_guard=guard)
+            ):
                 return False
-    elif isinstance(obj, (list, tuple, set)):
-        return all(map(is_serializable, obj))
+    elif isinstance(obj, (list, tuple, set, frozenset)):
+        guard = _recursive_guard | {id(obj)}
+        return all(is_serializable(item, _recursive_guard=guard) for item in obj)
 
+    # if an issue occurs for a non-serializable type, pickle will complain
+    # since the object is likely coming from a third-party extension (we
+    # natively expect 'simple' types and not weird ones)
     return True
 
 


Test patch:
diff --git a/tests/test_config/test_config.py b/tests/test_config/test_config.py
index ee305274e..d269b7169 100644
--- a/tests/test_config/test_config.py
+++ b/tests/test_config/test_config.py
@@ -1,7 +1,11 @@
 """Test the sphinx.config.Config class."""
+from __future__ import annotations
+
 import pickle
 import time
+from collections import Counter
 from pathlib import Path
+from typing import TYPE_CHECKING
 from unittest import mock
 
 import pytest
@@ -14,10 +18,51 @@
     _Opt,
     check_confval_types,
     correct_copyright_year,
+    is_serializable,
 )
 from sphinx.deprecation import RemovedInSphinx90Warning
 from sphinx.errors import ConfigError, ExtensionError, VersionRequirementError
 
+if TYPE_CHECKING:
+    from collections.abc import Iterable
+    from typing import Union
+
+    CircularList = list[Union[int, 'CircularList']]
+    CircularDict = dict[str, Union[int, 'CircularDict']]
+
+
+def check_is_serializable(subject: object, *, circular: bool) -> None:
+    assert is_serializable(subject)
+
+    if circular:
+        class UselessGuard(frozenset[int]):
+            def __or__(self, other: object, /) -> UselessGuard:
+                # do nothing
+                return self
+
+            def union(self, *args: Iterable[object]) -> UselessGuard:
+                # do nothing
+                return self
+
+        # check that without recursive guards, a recursion error occurs
+        with pytest.raises(RecursionError):
+            assert is_serializable(subject, _recursive_guard=UselessGuard())
+
+
+def test_is_serializable() -> None:
+    subject = [1, [2, {3, 'a'}], {'x': {'y': frozenset((4, 5))}}]
+    check_is_serializable(subject, circular=False)
+
+    a, b = [1], [2]  # type: (CircularList, CircularList)
+    a.append(b)
+    b.append(a)
+    check_is_serializable(a, circular=True)
+    check_is_serializable(b, circular=True)
+
+    x: CircularDict = {'a': 1, 'b': {'c': 1}}
+    x['b'] = x
+    check_is_serializable(x, circular=True)
+
 
 def test_config_opt_deprecated(recwarn):
     opt = _Opt('default', '', ())
@@ -102,6 +147,151 @@ def test_config_pickle_protocol(tmp_path, protocol: int):
     assert repr(config) == repr(pickled_config)
 
 
+def test_config_pickle_circular_reference_in_list():
+    a, b = [1], [2]  # type: (CircularList, CircularList)
+    a.append(b)
+    b.append(a)
+
+    check_is_serializable(a, circular=True)
+    check_is_serializable(b, circular=True)
+
+    config = Config()
+    config.add('a', [], '', types=list)
+    config.add('b', [], '', types=list)
+    config.a, config.b = a, b
+
+    actual = pickle.loads(pickle.dumps(config))
+    assert isinstance(actual.a, list)
+    check_is_serializable(actual.a, circular=True)
+
+    assert isinstance(actual.b, list)
+    check_is_serializable(actual.b, circular=True)
+
+    assert actual.a[0] == 1
+    assert actual.a[1][0] == 2
+    assert actual.a[1][1][0] == 1
+    assert actual.a[1][1][1][0] == 2
+
+    assert actual.b[0] == 2
+    assert actual.b[1][0] == 1
+    assert actual.b[1][1][0] == 2
+    assert actual.b[1][1][1][0] == 1
+
+    assert len(actual.a) == 2
+    assert len(actual.a[1]) == 2
+    assert len(actual.a[1][1]) == 2
+    assert len(actual.a[1][1][1]) == 2
+    assert len(actual.a[1][1][1][1]) == 2
+
+    assert len(actual.b) == 2
+    assert len(actual.b[1]) == 2
+    assert len(actual.b[1][1]) == 2
+    assert len(actual.b[1][1][1]) == 2
+    assert len(actual.b[1][1][1][1]) == 2
+
+    def check(
+        u: list[list[object] | int],
+        v: list[list[object] | int],
+        *,
+        counter: Counter[type, int] | None = None,
+        guard: frozenset[int] = frozenset(),
+    ) -> Counter[type, int]:
+        counter = Counter() if counter is None else counter
+
+        if id(u) in guard and id(v) in guard:
+            return counter
+
+        if isinstance(u, int):
+            assert v.__class__ is u.__class__
+            assert u == v
+            counter[type(u)] += 1
+            return counter
+
+        assert isinstance(u, list)
+        assert v.__class__ is u.__class__
+        assert len(u) == len(v)
+
+        for u_i, v_i in zip(u, v):
+            counter[type(u)] += 1
+            check(u_i, v_i, counter=counter, guard=guard | {id(u), id(v)})
+
+        return counter
+
+    counter = check(actual.a, a)
+    # check(actual.a, a)
+    #   check(actual.a[0], a[0]) -> ++counter[dict]
+    #       ++counter[int] (a[0] is an int)
+    #   check(actual.a[1], a[1]) -> ++counter[dict]
+    #       check(actual.a[1][0], a[1][0]) -> ++counter[dict]
+    #           ++counter[int] (a[1][0] is an int)
+    #       check(actual.a[1][1], a[1][1]) -> ++counter[dict]
+    #           recursive guard since a[1][1] == a
+    assert counter[type(a[0])] == 2
+    assert counter[type(a[1])] == 4
+
+    # same logic as above
+    counter = check(actual.b, b)
+    assert counter[type(b[0])] == 2
+    assert counter[type(b[1])] == 4
+
+
+def test_config_pickle_circular_reference_in_dict():
+    x: CircularDict = {'a': 1, 'b': {'c': 1}}
+    x['b'] = x
+    check_is_serializable(x, circular=True)
+
+    config = Config()
+    config.add('x', [], '', types=dict)
+    config.x = x
+
+    actual = pickle.loads(pickle.dumps(config))
+    check_is_serializable(actual.x, circular=True)
+    assert isinstance(actual.x, dict)
+
+    assert actual.x['a'] == 1
+    assert actual.x['b']['a'] == 1
+
+    assert len(actual.x) == 2
+    assert len(actual.x['b']) == 2
+    assert len(actual.x['b']['b']) == 2
+
+    def check(
+        u: dict[str, dict[str, object] | int],
+        v: dict[str, dict[str, object] | int],
+        *,
+        counter: Counter[type, int] | None = None,
+        guard: frozenset[int] = frozenset(),
+    ) -> Counter:
+        counter = Counter() if counter is None else counter
+
+        if id(u) in guard and id(v) in guard:
+            return counter
+
+        if isinstance(u, int):
+            assert v.__class__ is u.__class__
+            assert u == v
+            counter[type(u)] += 1
+            return counter
+
+        assert isinstance(u, dict)
+        assert v.__class__ is u.__class__
+        assert len(u) == len(v)
+
+        for u_i, v_i in zip(u, v):
+            counter[type(u)] += 1
+            check(u[u_i], v[v_i], counter=counter, guard=guard | {id(u), id(v)})
+        return counter
+
+    counters = check(actual.x, x, counter=Counter())
+    # check(actual.x, x)
+    #   check(actual.x['a'], x['a']) -> ++counter[dict]
+    #       ++counter[int] (x['a'] is an int)
+    #   check(actual.x['b'], x['b']) -> ++counter[dict]
+    #       recursive guard since x['b'] == x
+    assert counters[type(x['a'])] == 1
+    assert counters[type(x['b'])] == 2
+
+
 def test_extension_values():
     config = Config()