You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@allura.apache.org by gc...@apache.org on 2026/09/17 20:55:38 UTC
(allura) 01/03: [#8618] read git blob content via a one-off subprocess, not the shared cat-file pipe
This is an automated email from the ASF dual-hosted git repository.
asf-gitbox-commits pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/allura.git
commit 45090eb14c45a03ce83e86358f2ddafa4f2a6fd1
Author: Dave Brondsema <da...@brondsema.net>
AuthorDate: Wed Sep 9 13:30:42 2026 -0400
[#8618] read git blob content via a one-off subprocess, not the shared cat-file pipe
open_blob() now runs a dedicated `git cat-file -p <sha>` subprocess per read
instead of going through GitPython's persistent, shared `cat-file --batch`
process. A raw file download hands its blob content off to the WSGI layer as
a streaming response, and that response's consumption (and therefore its
cleanup) is outside our control: a vanished client or a WSGI server that
doesn't reliably close an abandoned response iterator can leave the shared
process holding unread bytes. Because MongoDB session cleanup for a streaming
response is deferred until the response is closed or exhausted, a later,
unrelated request on the same repo can end up reusing that same dirty
session and pipe, misreading the leftover bytes as its own response.
blob_size() stays on the shared persistent process: it always finishes in
one synchronous call and is never handed off to a response iterator, so it
isn't exposed to this risk, and keeping it on the fast path avoids adding
subprocess-spawn overhead to the common file-view/size-check case.
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
Claude-Session: https://claude.ai/code/session_012x5jtE8bYdSJRQuXBC2jzg
---
ForgeGit/forgegit/model/git_repo.py | 32 ++++++++++++---
.../forgegit/tests/functional/test_controllers.py | 45 ++++++++++++++++++++++
ForgeGit/forgegit/tests/model/test_repository.py | 38 ++++++++++++++++++
3 files changed, 109 insertions(+), 6 deletions(-)
diff --git a/ForgeGit/forgegit/model/git_repo.py b/ForgeGit/forgegit/model/git_repo.py
index 6a3b900ac..ae24595a4 100644
--- a/ForgeGit/forgegit/model/git_repo.py
+++ b/ForgeGit/forgegit/model/git_repo.py
@@ -524,11 +524,23 @@ def _iter_commits_with_refs(self, *args, **kwargs):
commit_lines.append(line)
def open_blob(self, blob):
- return _OpenedGitBlob(
- self._object(blob._id).data_stream)
+ # A dedicated `git cat-file -p <sha>` subprocess, not GitPython's persistent,
+ # shared `cat-file --batch` process (used by e.g. `.data_stream`/`.parents`/
+ # `.tree`). This blob's bytes get streamed straight into an HTTP response.
+ # If that is ever abandoned partway through, only this one throwaway
+ # subprocess is affected, and GitPython's own AutoInterrupt wrapper kills it
+ # on garbage collection. Nothing else ever shares this pipe, so nothing else
+ # can be corrupted by bytes left over in it.
+ proc = self._git.git.cat_file(blob._id, p=True, as_process=True) # -p: print the object's content
+ return _OpenedGitBlob(proc)
def blob_size(self, blob):
- return self._object(blob._id).data_stream.size
+ # Unlike open_blob(), this always finishes in one synchronous call -- never handed off to an HTTP response
+ # so it's safe to use GitPython's shared persistent process
+ stream = self._object(blob._id).data_stream
+ size = stream.size
+ del stream # drain any unread body bytes before the persistent process is reused
+ return size
def _setup_hooks(self, source_path=None):
'Set up the git post-commit hook'
@@ -822,11 +834,14 @@ def merge_request_commits(self, mr):
class _OpenedGitBlob:
CHUNK_SIZE = 4096
- def __init__(self, stream):
- self._stream = stream
+ def __init__(self, proc: git.Git.AutoInterrupt):
+ self._proc = proc
+ self._stream = proc.stdout
def read(self):
- return self._stream.read()
+ data = self._stream.read()
+ self._proc.wait() # reap the process now, and raise if git itself failed
+ return data
def __iter__(self):
'''
@@ -849,8 +864,13 @@ def __iter__(self):
break
yield buffer[:eol + 1]
buffer = buffer[eol + 1:]
+ self._proc.wait() # reached naturally, not via close()/abandonment: reap now
def close(self):
+ # no explicit cleanup needed: AutoInterrupt.__del__ closes the pipes and
+ # terminates the process (if still running) once `self._proc` is
+ # garbage collected, whether that's because of this close() call, the
+ # generator in __iter__() being closed early, or neither ever happening
pass
diff --git a/ForgeGit/forgegit/tests/functional/test_controllers.py b/ForgeGit/forgegit/tests/functional/test_controllers.py
index 8210429f4..29cd5f888 100644
--- a/ForgeGit/forgegit/tests/functional/test_controllers.py
+++ b/ForgeGit/forgegit/tests/functional/test_controllers.py
@@ -331,6 +331,51 @@ def test_file_raw(self):
assert (resp.headers.get('Content-Disposition') ==
'attachment;filename="with%22%26%3Aspecials.txt"')
+ def _start_raw_download(self, url):
+ '''
+ Call the real WSGI stack for a `format=raw` blob download, but return the response iterator
+ after reading only its first chunk -- bypassing WebTest's normal do_request(), which always fully drains it
+ '''
+ environ = self.app._make_environ()
+ url, qs = url.split('?', 1)
+ environ['QUERY_STRING'] = qs
+ req = self.app.RequestClass.blank(url, environ)
+ _status, _headers, app_iter, exc_info = req.call_application(
+ self.app.app, catch_exc_info=True)
+ del exc_info
+ it = iter(app_iter)
+ next(it)
+ return app_iter, it
+
+ def test_abandoned_raw_download_does_not_corrupt_next_request(self):
+ # Regression test: a client that disconnects or WSGI server that doesn't close
+ # an abandoned response iterator from a raw download
+ # It must not corrupt later requests. This used to fail:
+ # GitPython's persistent `cat-file` subprocess still held unread bytes
+ ci_master = '/p/test/src-git/ci/1e146e67985dcd71c74de79613719bef7bddca4a/'
+ ci_zz = '/p/test/src-git/ci/5c47243c8e424136fd5cdd18cd94d34c66d1955c/'
+
+ with patch.object(GM.git_repo._OpenedGitBlob, 'CHUNK_SIZE', 5):
+ # hold _abandoned_download on `self` so it doesn't get garbage collected
+ self._abandoned_download = self._start_raw_download(ci_master + 'tree/README?format=raw')
+
+ resp = self.app.get(ci_zz + 'tree/bad?format=raw')
+ assert resp.body == b'Not root\n'
+
+ def test_closed_raw_download_does_not_corrupt_next_request(self):
+ # Same as above, but the first download's iterator is properly closed. Control case.
+ ci_master = '/p/test/src-git/ci/1e146e67985dcd71c74de79613719bef7bddca4a/'
+ ci_zz = '/p/test/src-git/ci/5c47243c8e424136fd5cdd18cd94d34c66d1955c/'
+
+ with patch.object(GM.git_repo._OpenedGitBlob, 'CHUNK_SIZE', 5):
+ app_iter, it = self._start_raw_download(ci_master + 'tree/README?format=raw')
+ it.close()
+ if hasattr(app_iter, 'close'):
+ app_iter.close()
+
+ resp = self.app.get(ci_zz + 'tree/bad?format=raw')
+ assert resp.body == b'Not root\n'
+
def test_file_too_large(self):
ci = self._get_ci()
with h.push_config(tg.config, **{'scm.view.max_file_bytes': '3'}):
diff --git a/ForgeGit/forgegit/tests/model/test_repository.py b/ForgeGit/forgegit/tests/model/test_repository.py
index f5c5591b4..eed4c38cd 100644
--- a/ForgeGit/forgegit/tests/model/test_repository.py
+++ b/ForgeGit/forgegit/tests/model/test_repository.py
@@ -20,6 +20,7 @@
import stat
import datetime
import email.iterators
+import threading
import pytest
import mock
import git
@@ -602,6 +603,43 @@ def test_ls_with_prev(self):
self.test_ls()
+ def test_open_blob_thread_safety(self):
+ # regression test: open_blob() spawns its own one-off `git cat-file` subprocess
+ # to avoid concurrent read corruption
+ impl = self.repo._impl
+ tree = self.repo.commit('zz').tree
+ blob_readme = tree['README']
+ blob_bad = tree['bad']
+ errors = []
+
+ def read_readme():
+ for _ in range(50):
+ data = impl.open_blob(blob_readme).read()
+ if data != b'This is readme\nAnother Line\n':
+ errors.append(('readme', data))
+
+ def read_bad():
+ for _ in range(50):
+ data = impl.open_blob(blob_bad).read()
+ if data != b'Not root\n':
+ errors.append(('bad', data))
+
+ threads = (
+ [threading.Thread(target=read_readme) for _ in range(5)]
+ + [threading.Thread(target=read_bad) for _ in range(5)]
+ )
+ for t in threads:
+ t.start()
+ for t in threads:
+ t.join()
+
+ assert errors == []
+
+ def test_blob_size(self):
+ tree = self.repo.commit('zz').tree
+ assert self.repo._impl.blob_size(tree['README']) == 28
+ assert self.repo._impl.blob_size(tree['bad']) == 9
+
def test_tarball_status(self):
tmpdir = tg.config['scm.repos.tarball.root']
if os.path.isfile(os.path.join(tmpdir, "git/t/te/test/testgit.git/test-src-git-HEAD.zip")):