proj/portage:master commit in: lib/portage/dbapi/, lib/portage/tests/dbapi/, lib/portage/tests/resolver/
"Matt Turner" <[email protected]>
| Newsgroups | gmane.linux.gentoo.cvs |
|---|---|
| Message-ID | <1786672955.0efa775a4cc15720c09e2af69e6c316bc7727207.mattst88@gentoo> |
commit: 0efa775a4cc15720c09e2af69e6c316bc7727207
Author: Matt Turner <mattst88 <AT> gentoo <DOT> org>
AuthorDate: Sun Aug 9 05:05:19 2026 +0000
Commit: Matt Turner <mattst88 <AT> gentoo <DOT> org>
CommitDate: Fri Aug 14 02:02:35 2026 +0000
URL: https://gitweb.gentoo.org/proj/portage.git/commit/?id=0efa775a
vartree: validate the metadata file against the package dir mtime
The metadata file was trusted across sessions with nothing to say it was
still current. vdb_metadata.pickle, which this series removes, validated
each entry against the package directory's mtime, so replacing it left the
read path with a weaker freshness guarantee than the cache it replaced.
Record the directory's st_mtime_ns in the file and reject it when the
directory has changed since. That restores the pickle's guarantee, now
per-package instead of one global file. Anything that adds, removes or
replaces an entry in the package directory invalidates the snapshot and
the caller falls back to the individual files; an in-place rewrite of a
field file still goes unnoticed, exactly as it did with the pickle.
"#dir_mtime=" is appended after the atomic write rather than included in
it. write_atomic() renames into place and that rename bumps the
directory's mtime, so a value recorded before it would never match.
Appending creates no directory entry, so it leaves the mtime alone. A file
left without the line by an interrupted write is rejected, which is the
behaviour we want.
_aux_get() already stat()s the package directory and passes that stat in,
so validation costs no extra syscall. The read path is complete as of this
commit. Measured on this machine: 1742 installed packages, 23 keys, best
of 7 full-VDB passes through _aux_get(), syscalls via strace -f -c.
master 305.7 ms, 41012 openat
this commit 32.6 ms, 2689 openat
The same validation makes the incremental metadata updates in aux_update()
and setfile() pointless, so both are removed. write_atomic() of a field
file, and os.unlink() of one, each change the package directory, so by the
time either function looked at the metadata file it could never still
validate: the patch-in-place branch was unreachable and every call fell
through to a full rebuild. aux_update() now does that rebuild once, after
its writes, and only for a package that already has a metadata file.
ResolverPlayground writes the metadata file last when creating an
installed package, after the .ebuild and environment.bz2. Creating either
afterwards would change the directory and invalidate the file it had just
written, leaving every test on the per-field fallback.
Signed-off-by: Matt Turner <mattst88 <AT> gentoo.org>
lib/portage/dbapi/vartree.py | 136 +++++++++++++----------
lib/portage/tests/dbapi/test_vdb_metadata.py | 72 ++++++++++--
lib/portage/tests/resolver/ResolverPlayground.py | 9 +-
3 files changed, 149 insertions(+), 68 deletions(-)
diff --git a/lib/portage/dbapi/vartree.py b/lib/portage/dbapi/vartree.py
index 667d7b546..3b5338a41 100644
--- a/lib/portage/dbapi/vartree.py
+++ b/lib/portage/dbapi/vartree.py
@@ -110,6 +110,7 @@ _METADATA_FILE_FIELDS = frozenset(
)
_METADATA_FILE_FORMAT_VERSION = 1
_METADATA_FORMAT_PREFIX = "#format="
+_METADATA_DIR_MTIME_PREFIX = "#dir_mtime="
def _in_metadata_file(fname):
@@ -117,27 +118,36 @@ def _in_metadata_file(fname):
return fname in _METADATA_FILE_FIELDS
-def _read_metadata_file(path):
+def _read_metadata_file(path, dir_st=None):
"""Parse KEY=value\\n metadata file.
Returns dict[str, str], or None if the file is not a snapshot this
portage version can use.
- A returned dict is treated as a *complete* snapshot: every field matching
- _METADATA_FIELD_RE that existed when the file was written is present, so a
- field missing from it is served as empty rather than falling back to a
- per-field read. That holds only while reader and writer agree on which
- fields get written, so a file whose "#format=" header is absent or does
- not match _METADATA_FILE_FORMAT_VERSION is rejected, and the caller falls
- back to the individual files.
-
- The field set is therefore part of the format: bump
- _METADATA_FILE_FORMAT_VERSION on any change to _METADATA_FILE_FIELDS, in
- either direction. Adding a field would otherwise make an older file
- lacking it read as saying it is empty, and dropping one would do the same
- to an older portage reading a newer file. A version this portage does not
- know is rejected, so both skews fall back to the individual files rather
- than serving a wrong answer.
+ A returned dict is treated as a *complete* snapshot: every field accepted
+ by _in_metadata_file() that existed when the file was written is present,
+ so a field missing from it is served as empty rather than falling back to
+ a per-field read. Two things must hold for that to be sound, and a file
+ failing either is rejected so the caller falls back to the individual
+ files:
+
+ - Reader and writer must agree on which fields get written, so the
+ "#format=" header must match _METADATA_FILE_FORMAT_VERSION. The field
+ set is therefore part of the format: bump that constant on any change to
+ _METADATA_FILE_FIELDS, in either direction. Adding a field would
+ otherwise make an older file lacking it read as saying it is empty, and
+ dropping one would do the same to an older portage reading a newer file.
+ A version this portage does not know is rejected, so both skews fall
+ back to the individual files rather than serving a wrong answer.
+ - The package directory must not have changed since the file was written,
+ so the recorded "#dir_mtime=" must match the directory's st_mtime_ns.
+ This is the same freshness signal vdb_metadata.pickle validated against,
+ and it is what makes a stale file fall back rather than lie. Pass dir_st
+ when the caller already stat()ed the directory; otherwise it is stat()ed
+ here.
+
+ "#dir_mtime=" is written last, so a file left truncated by an interrupted
+ write lacks it and is rejected rather than read as a short snapshot.
Other lines beginning with '#' are ignored.
"""
@@ -145,6 +155,7 @@ def _read_metadata_file(path):
result = {}
version = None
+ dir_mtime = None
with open(path, encoding=_encodings["repo.content"], errors="replace") as f:
for line in f:
line = line.rstrip("\n")
@@ -154,12 +165,28 @@ def _read_metadata_file(path):
version = int(line[len(_METADATA_FORMAT_PREFIX) :])
except ValueError:
return None
+ # Written first, so a file we cannot use is abandoned
+ # before parsing the rest of it.
+ if version != _METADATA_FILE_FORMAT_VERSION:
+ return None
+ elif line.startswith(_METADATA_DIR_MTIME_PREFIX):
+ try:
+ dir_mtime = int(line[len(_METADATA_DIR_MTIME_PREFIX) :])
+ except ValueError:
+ return None
continue
if "=" not in line:
continue
k, v = line.split("=", 1)
result[k] = v
- if version != _METADATA_FILE_FORMAT_VERSION:
+ if version is None or dir_mtime is None:
+ return None
+ if dir_st is None:
+ try:
+ dir_st = os.stat(os.path.dirname(path))
+ except OSError:
+ return None
+ if dir_mtime != dir_st.st_mtime_ns:
return None
return result
@@ -171,18 +198,22 @@ def _write_metadata_file(dbdir, data):
values are whitespace-normalized here (the same normalization _aux_get
applies to single-line fields). Doing it here stops a caller that passes a
raw multi-line value from silently truncating the file.
+
+ The "#dir_mtime=" line the reader validates against is appended after the
+ atomic write rather than included in it: write_atomic() renames into
+ place, and that rename bumps dbdir's mtime, so a value recorded before it
+ would never match. Appending does not create or remove a directory entry,
+ so it leaves dbdir's mtime alone and the recorded value stays true.
"""
from portage import _encodings
from portage.util import write_atomic
- content = f"#format={_METADATA_FILE_FORMAT_VERSION}\n"
+ path = os.path.join(dbdir, _METADATA_FILE)
+ content = f"{_METADATA_FORMAT_PREFIX}{_METADATA_FILE_FORMAT_VERSION}\n"
content += "".join(f"{k}={' '.join(v.split())}\n" for k, v in sorted(data.items()))
- write_atomic(
- os.path.join(dbdir, _METADATA_FILE),
- content,
- mode="w",
- encoding=_encodings["repo.content"],
- )
+ write_atomic(path, content, mode="w", encoding=_encodings["repo.content"])
+ with open(path, mode="a", encoding=_encodings["repo.content"]) as f:
+ f.write(f"{_METADATA_DIR_MTIME_PREFIX}{os.stat(dbdir).st_mtime_ns}\n")
def _consolidate_to_metadata_file(dbdir):
@@ -969,7 +1000,9 @@ class vardbapi(dbapi):
metadata_data = None
try:
- metadata_data = _read_metadata_file(os.path.join(mydir, _METADATA_FILE))
+ metadata_data = _read_metadata_file(
+ os.path.join(mydir, _METADATA_FILE), dir_st=st
+ )
except OSError:
pass
@@ -1113,20 +1146,17 @@ class vardbapi(dbapi):
os.unlink(os.path.join(self.getpath(cpv), k))
except OSError:
pass
- # Remove from metadata file if present.
- metadata_path = os.path.join(self.getpath(cpv), _METADATA_FILE)
- try:
- existing = _read_metadata_file(metadata_path)
- if existing is None:
- # Rejected: rebuild the whole snapshot rather than
- # patching one written under a field set we no longer
- # agree on.
- _consolidate_to_metadata_file(self.getpath(cpv))
- elif k in existing:
- del existing[k]
- _write_metadata_file(self.getpath(cpv), existing)
- except OSError:
- pass
+ # Writing or removing an individual file changes the package
+ # directory, so a metadata file it had no longer validates. Rebuild it
+ # once here rather than patching each field: a rejected file is only
+ # ignored, but leaving it that way would cost a per-field read on
+ # every later aux_get() for this package.
+ pkgdir = self.getpath(cpv)
+ if os.path.exists(os.path.join(pkgdir, _METADATA_FILE)):
+ try:
+ _consolidate_to_metadata_file(pkgdir)
+ except OSError:
+ pass
self._bump_mtime(cpv)
@staticmethod
@@ -5073,9 +5103,6 @@ class dblink:
) as f:
f.write(f"{counter}")
- # Consolidate all per-field metadata files into a single metadata file.
- _consolidate_to_metadata_file(self.dbtmpdir)
-
self.updateprotect()
# if we have a file containing previously-merged config file md5sums, grab it.
@@ -5214,6 +5241,15 @@ class dblink:
self.unlockdb()
showMessage(_(">>> Original instance of package unmerged safely.\n"))
+ # Consolidate the per-field metadata files into a single metadata
+ # file. This has to be the last write into dbtmpdir: the file records
+ # the directory's mtime and is rejected if the directory changes
+ # afterwards, and CONTENTS is written above via a rename that would
+ # otherwise invalidate it for every freshly merged package. Renaming
+ # dbtmpdir into place below does not alter its own mtime, so the
+ # recorded value survives the move.
+ _consolidate_to_metadata_file(self.dbtmpdir)
+
# We hold both directory locks.
self.dbdir = self.dbpkgdir
self.lockdb()
@@ -6236,22 +6272,6 @@ class dblink:
kwargs["encoding"] = "utf-8"
write_atomic(os.path.join(self.dbdir, fname), data, **kwargs)
- # Keep the metadata file in sync if it exists.
- if isinstance(data, str) and _in_metadata_file(fname):
- metadata_path = os.path.join(self.dbdir, _METADATA_FILE)
- try:
- existing = _read_metadata_file(metadata_path)
- except OSError:
- return
- if existing is None:
- # Stale format: the individual file above is already current,
- # so rebuild the snapshot instead of patching one written
- # under a field set we no longer agree on.
- _consolidate_to_metadata_file(self.dbdir)
- return
- existing[fname] = " ".join(data.split())
- _write_metadata_file(self.dbdir, existing)
-
def getelements(self, ename):
if not os.path.exists(self.dbdir + "/" + ename):
return []
diff --git a/lib/portage/tests/dbapi/test_vdb_metadata.py b/lib/portage/tests/dbapi/test_vdb_metadata.py
index fc9696abe..1824a524f 100644
--- a/lib/portage/tests/dbapi/test_vdb_metadata.py
+++ b/lib/portage/tests/dbapi/test_vdb_metadata.py
@@ -66,10 +66,18 @@ class VdbMetadataReadWriteTestCase(TestCase):
result = _read_metadata_file(path)
self.assertEqual(result, data)
- def _write_raw(self, content):
+ def _write_raw(self, content, stamp=True):
+ """Write raw metadata content, appending a valid #dir_mtime= by default.
+
+ The stamp is taken after the file exists, since creating it changes the
+ directory mtime the reader validates against.
+ """
path = os.path.join(self._tmpdir, _METADATA_FILE)
with open(path, "w") as f:
f.write(content)
+ if stamp:
+ with open(path, "a") as f:
+ f.write(f"#dir_mtime={os.stat(self._tmpdir).st_mtime_ns}\n")
return path
def test_rejects_missing_format_header(self):
@@ -86,11 +94,45 @@ class VdbMetadataReadWriteTestCase(TestCase):
path = self._write_raw("#format=bogus\nEAPI=8\n")
self.assertIsNone(_read_metadata_file(path))
- def test_other_comments_ignored(self):
+ def test_rejects_missing_dir_mtime(self):
+ # An interrupted write leaves the file without its trailing
+ # #dir_mtime=, and a short snapshot must not be read as complete.
path = self._write_raw(
- f"#format={_METADATA_FILE_FORMAT_VERSION}\n# a comment\nEAPI=8\n"
+ f"#format={_METADATA_FILE_FORMAT_VERSION}\nEAPI=8\n", stamp=False
)
+ self.assertIsNone(_read_metadata_file(path))
+
+ def test_rejects_stale_dir_mtime(self):
+ # Anything that changes the package directory after the file was
+ # written invalidates it, so the caller falls back to per-field reads.
+ path = self._write_raw(f"#format={_METADATA_FILE_FORMAT_VERSION}\nEAPI=8\n")
self.assertEqual(_read_metadata_file(path), {"EAPI": "8"})
+ os.utime(self._tmpdir, ns=(0, 0))
+ self.assertIsNone(_read_metadata_file(path))
+
+ def test_rejects_non_integer_dir_mtime(self):
+ path = self._write_raw(
+ f"#format={_METADATA_FILE_FORMAT_VERSION}\nEAPI=8\n#dir_mtime=bogus\n",
+ stamp=False,
+ )
+ self.assertIsNone(_read_metadata_file(path))
+
+ def test_dir_st_argument_used(self):
+ # _aux_get passes the stat it already holds; it must be honored.
+ path = self._write_raw(f"#format={_METADATA_FILE_FORMAT_VERSION}\nEAPI=8\n")
+ st = os.stat(self._tmpdir)
+ self.assertEqual(_read_metadata_file(path, dir_st=st), {"EAPI": "8"})
+ os.utime(self._tmpdir, ns=(0, 0))
+ # A caller passing the pre-change stat still validates against it.
+ self.assertEqual(_read_metadata_file(path, dir_st=st), {"EAPI": "8"})
+ self.assertIsNone(_read_metadata_file(path, dir_st=os.stat(self._tmpdir)))
+
+ def test_write_then_read_survives_rename(self):
+ # write_atomic() renames into place, bumping the directory mtime; the
+ # stamp is taken afterwards so the file it just wrote is readable.
+ _write_metadata_file(self._tmpdir, {"EAPI": "8", "SLOT": "0"})
+ path = os.path.join(self._tmpdir, _METADATA_FILE)
+ self.assertEqual(_read_metadata_file(path), {"EAPI": "8", "SLOT": "0"})
def test_format_version_header_written(self):
_write_metadata_file(self._tmpdir, {"EAPI": "8"})
@@ -107,11 +149,9 @@ class VdbMetadataReadWriteTestCase(TestCase):
self.assertEqual(result["EAPI"], "8")
def test_comment_lines_ignored(self):
- path = os.path.join(self._tmpdir, _METADATA_FILE)
- with open(path, "w") as f:
- f.write("#format=1\n")
- f.write("# another comment\n")
- f.write("EAPI=8\n")
+ path = self._write_raw(
+ f"#format={_METADATA_FILE_FORMAT_VERSION}\n# another comment\nEAPI=8\n"
+ )
result = _read_metadata_file(path)
self.assertEqual(result, {"EAPI": "8"})
@@ -131,6 +171,22 @@ class VdbMetadataReadWriteTestCase(TestCase):
result = _read_metadata_file(path)
self.assertEqual(result["HOMEPAGE"], "https://example.com/?foo=bar")
+ def test_value_with_hash(self):
+ # A '#' inside a value must not be mistaken for a comment: only a
+ # line *starting* with '#' is one.
+ data = {"HOMEPAGE": "https://example.com/#anchor"}
+ _write_metadata_file(self._tmpdir, data)
+ path = os.path.join(self._tmpdir, _METADATA_FILE)
+ result = _read_metadata_file(path)
+ self.assertEqual(result["HOMEPAGE"], "https://example.com/#anchor")
+
+ def test_value_with_dots_and_hash(self):
+ data = {"HOMEPAGE": "http://127.0.0.1/?a=1#anchor"}
+ _write_metadata_file(self._tmpdir, data)
+ path = os.path.join(self._tmpdir, _METADATA_FILE)
+ result = _read_metadata_file(path)
+ self.assertEqual(result["HOMEPAGE"], "http://127.0.0.1/?a=1#anchor")
+
def test_empty_value(self):
data = {"IUSE": "", "EAPI": "8"}
_write_metadata_file(self._tmpdir, data)
diff --git a/lib/portage/tests/resolver/ResolverPlayground.py b/lib/portage/tests/resolver/ResolverPlayground.py
index 4c1d628cd..ce255097b 100644
--- a/lib/portage/tests/resolver/ResolverPlayground.py
+++ b/lib/portage/tests/resolver/ResolverPlayground.py
@@ -526,8 +526,6 @@ class ResolverPlayground:
f.write(f"{v}\n")
if _in_metadata_file(k):
metadata_kv[k] = str(v)
- if metadata_kv:
- _write_metadata_file(vdb_pkg_dir, metadata_kv)
ebuild_path = os.path.join(vdb_pkg_dir, a.cpv.split("/")[1] + ".ebuild")
with open(ebuild_path, "w") as f:
@@ -540,6 +538,13 @@ class ResolverPlayground:
with open(ebuild_path, "rb") as inputfile:
f.write(inputfile.read())
+ # Written last: the metadata file records the package directory's
+ # mtime and is rejected if it no longer matches, so creating any
+ # further entry in the directory afterwards would invalidate it and
+ # leave tests silently exercising only the per-field fallback.
+ if metadata_kv:
+ _write_metadata_file(vdb_pkg_dir, metadata_kv)
+
def _create_profile(
self, ebuilds, eclasses, installed, profile, repo_configs, user_config, sets
):