diff mbox series

[v6] fetch/local: verify checksums for file:// urls

Message ID 20260930212125.118877-1-setairidis@gmail.com
State New
Headers show
Series [v6] fetch/local: verify checksums for file:// urls | expand

Commit Message

Savvas Etairidis Sept. 30, 2026, 9:21 p.m. UTC
[YOCTO #9993]

A checksum given in a file:// url (e.g. UNINATIVE_CHECKSUM when
UNINATIVE_URL points at a local file://) was never verified, because
Local disables the donestamp and so verify_checksum() was never called.
Override verify_donestamp() in Local to check the file against those
checksums on every fetch, via a new
FetchData.verify_checksum_if_expected() helper. A mismatch raises
ChecksumError before the rename/mirror handling, so the user's file is
never renamed.

Hashing directly rather than enabling the donestamp avoids trusting a
cached checksum for a file edited with an old mtime, and avoids stamp
and lock paths in DL_DIR derived from the url path colliding with other
recipes or real downloads.
The file is re-hashed on every fetch; this is a known cost, but only
for file:// urls that carry a checksum, which are typically small.

Based on a patch by Jhonata Poma-Hansen.

AI-Generated: Uses Claude Code (Claude Opus 5.5)
Signed-off-by: Savvas Etairidis <setairidis@gmail.com>

---
changes in v6:
- Rebased on top of master
- Fixed issues reported by reviewer
- Check the file directly in Local.verify_donestamp(), via a new
  FetchData.verify_checksum_if_expected() helper, instead of enabling
  the donestamp. Fixes stale checksums for files edited with an old
  mtime and DL_DIR stamp/lock path collisions
- Drop the rename_bad_checksum() refactor, no longer needed
- Document the cost of re-hashing on every fetch
- Tests: use a correct md5sum and a wrong sha256sum in the mismatch
  test, add a test for a file edited with an old mtime, and check that
  nothing is written to DL_DIR
- Tested with BB_SKIP_NETTESTS=yes bin/bitbake-selftest bb.tests.fetch

changes in v5:
- Rebased on top of master, added changelog entry

changes in v4:
- Rebased on top of master

changes in v3:
- Cherry-picked the patch from Jhonata Poma-Hansen

---
---
 lib/bb/fetch/__init__.py | 13 ++++++++++++
 lib/bb/fetch/local.py    | 16 ++++++++++++++
 lib/bb/tests/fetch.py    | 46 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)

Comments

Paul Barker Oct. 1, 2026, 8:32 a.m. UTC | #1
On Wed, 2026-09-30 at 23:21 +0200, Savvas Etairidis wrote:
> [YOCTO #9993]
> 
> A checksum given in a file:// url (e.g. UNINATIVE_CHECKSUM when
> UNINATIVE_URL points at a local file://) was never verified, because
> Local disables the donestamp and so verify_checksum() was never called.
> Override verify_donestamp() in Local to check the file against those
> checksums on every fetch, via a new
> FetchData.verify_checksum_if_expected() helper. A mismatch raises
> ChecksumError before the rename/mirror handling, so the user's file is
> never renamed.
> 
> Hashing directly rather than enabling the donestamp avoids trusting a
> cached checksum for a file edited with an old mtime, and avoids stamp
> and lock paths in DL_DIR derived from the url path colliding with other
> recipes or real downloads.
> The file is re-hashed on every fetch; this is a known cost, but only
> for file:// urls that carry a checksum, which are typically small.

We shouldn't assume that anything coming from a file:// URL is
"typically small", or that it is on fast local storage (it could be on a
HDD array accessed over NFS).

> 
> Based on a patch by Jhonata Poma-Hansen.
> 
> AI-Generated: Uses Claude Code (Claude Opus 5.5)
> Signed-off-by: Savvas Etairidis <setairidis@gmail.com>
> 
> ---
> changes in v6:
> - Rebased on top of master

You don't need to mention this if it didn't result in actual change.

> - Fixed issues reported by reviewer

Such as? This is a completely different approach so it's useful to know
which bits of feedback were actually relevant and which were made
obsolete by the change in approach.

> - Check the file directly in Local.verify_donestamp(), via a new
>   FetchData.verify_checksum_if_expected() helper, instead of enabling
>   the donestamp. Fixes stale checksums for files edited with an old
>   mtime and DL_DIR stamp/lock path collisions

What does "edited with an old mtime" mean?

> - Drop the rename_bad_checksum() refactor, no longer needed
> - Document the cost of re-hashing on every fetch
> - Tests: use a correct md5sum and a wrong sha256sum in the mismatch
>   test, add a test for a file edited with an old mtime, and check that
>   nothing is written to DL_DIR

Why complicate this by using different hash algorithms for the different
tests?

> - Tested with BB_SKIP_NETTESTS=yes bin/bitbake-selftest bb.tests.fetch
> 
> changes in v5:
> - Rebased on top of master, added changelog entry
> 
> changes in v4:
> - Rebased on top of master
> 
> changes in v3:
> - Cherry-picked the patch from Jhonata Poma-Hansen

Best regards,
diff mbox series

Patch

diff --git a/lib/bb/fetch/__init__.py b/lib/bb/fetch/__init__.py
index fb6278439..418a031ec 100644
--- a/lib/bb/fetch/__init__.py
+++ b/lib/bb/fetch/__init__.py
@@ -1413,6 +1413,19 @@  class FetchData(object):
         self.donestamp = basepath + '.done'
         self.lockfile = basepath + '.lock'
 
+    def verify_checksum_if_expected(self, d):
+        """
+        Verify localpath against the checksums given in the url, if any.
+        """
+        expected = False
+        for checksum_id in CHECKSUM_LIST:
+            if getattr(self, "%s_expected" % checksum_id):
+                expected = True
+                break
+
+        if expected:
+            verify_checksum(self, d)
+
     def setup_revisions(self, d):
         self.revision = srcrev_internal_helper(self, d, self.name)
 
diff --git a/lib/bb/fetch/local.py b/lib/bb/fetch/local.py
index dfc5b564c..d7551221b 100644
--- a/lib/bb/fetch/local.py
+++ b/lib/bb/fetch/local.py
@@ -36,6 +36,22 @@  class Local(FetchMethod):
             raise bb.fetch.ParameterError("file:// urls using globbing are no longer supported. Please place the files in a directory and reference that instead.", ud.url)
         return
 
+    def verify_donestamp(self, ud, d):
+        # file:// urls have no donestamp; verify any checksum given in the url
+        # against the file itself on every fetch, since it may be edited in
+        # place without its mtime changing. A missing file is reported by
+        # download() with the searched paths, so there's nothing to check here.
+        if not os.path.exists(ud.localpath):
+            return True
+        # Re-hashing on every run is a known cost: verify_checksum() computes
+        # every algorithm in CHECKSUM_LIST, and Fetch.download() calls
+        # verify_donestamp() twice, so a 100MB file adds ~1s per fetch.
+        # This only affects file:// urls that carry a checksum, which are
+        # typically small (e.g. uninative tarballs), so correctness is
+        # preferred over caching.
+        ud.verify_checksum_if_expected(d)
+        return True
+
     def localpath(self, urldata, d):
         """
         Return the local filename of a given url assuming a successful fetch.
diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 71b0a63fe..2bec8a66a 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -876,6 +876,52 @@  class FetcherLocalTest(FetcherTest):
             with self.subTest(striplevel=repr(value)):
                 self.assertInvalidStriplevel(value)
 
+    def test_local_checksum_match(self):
+        import hashlib
+        content = b"file:// checksum match test\n"
+        with open(os.path.join(self.localsrcdir, 'sumfile'), 'wb') as f:
+            f.write(content)
+        good = hashlib.sha256(content).hexdigest()
+        fetcher = bb.fetch.Fetch(['file://sumfile;sha256sum=' + good], self.d)
+        fetcher.download()
+        # No stamp or lock files should be created in DL_DIR for file:// urls
+        self.assertEqual(os.listdir(self.dldir), [])
+
+    def test_local_checksum_modified_with_old_mtime(self):
+        import hashlib
+        srcpath = os.path.join(self.localsrcdir, 'sumfile')
+        with open(srcpath, 'wb') as f:
+            f.write(b"file:// checksum original content\n")
+        good = hashlib.sha256(b"file:// checksum original content\n").hexdigest()
+        url = 'file://sumfile;sha256sum=' + good
+        bb.fetch.Fetch([url], self.d).download()
+
+        # Replace the content but keep an old mtime, as cp -p or rsync -a would
+        old = os.stat(srcpath).st_mtime - 3600
+        with open(srcpath, 'wb') as f:
+            f.write(b"file:// checksum modified content\n")
+        os.utime(srcpath, (old, old))
+        with self.assertRaises(bb.fetch.ChecksumError):
+            bb.fetch.Fetch([url], self.d).download()
+        self.assertTrue(os.path.exists(srcpath))
+
+    def test_local_checksum_mismatch(self):
+        import hashlib
+        content = b"file:// checksum mismatch test\n"
+        srcpath = os.path.join(self.localsrcdir, 'sumfile')
+        with open(srcpath, 'wb') as f:
+            f.write(content)
+        md5 = hashlib.md5(content).hexdigest()
+        # verify_checksum_if_expected() stops at the first checksum it finds
+        # (md5); a wrong checksum later in CHECKSUM_LIST must still be caught
+        url = 'file://sumfile;md5sum=%s;sha256sum=%s' % (md5, "0" * 64)
+        fetcher = bb.fetch.Fetch([url], self.d)
+        with self.assertRaises(bb.fetch.ChecksumError) as cm:
+            fetcher.download()
+        self.assertIn("has sha256 checksum", str(cm.exception))
+        # The user's file must not be renamed to *_bad-checksum_*
+        self.assertTrue(os.path.exists(srcpath))
+
     def dummyGitTest(self, suffix):
         # Create dummy local Git repo
         src_dir = tempfile.mkdtemp(dir=self.tempdir,