Re: [bitbake-devel] [PATCH] fetch2/local: verify checksums for file:// urls

Paul Barker <[email protected]> Fri, 08 May 2026 15:13:26 +0100
Newsgroups	org.openembedded.lists.bitbake-devel
Message-ID	<[email protected]>
On Thu, 2026-04-30 at 16:25 +0200, Jhonata Poma-Hansen via
lists.openembedded.org wrote:
> Local.urldata_init unconditionally cleared ud.needdonestamp, which
> short-circuited the donestamp + verify_checksum cycle for every
> file:// url. As a result, a checksum supplied as a url parameter
> (for instance UNINATIVE_CHECKSUM expanded into the uninative
> tarball's file:// url) was never compared against the file's
> actual contents - the fetcher silently used whatever bytes were
> at the path.
> 
> Keep needdonestamp at the FetchData default (True) when the url
> carries any of the known checksum parameters, so the standard
> verify_checksum flow runs and a mismatch raises ChecksumError just
> like it does for http:// and friends. The check uses CHECKSUM_LIST
> and handles both the bare ;<algo>sum= form and the named
> ;name=foo;foo.<algo>sum= form that recipes (and UNINATIVE_CHECKSUM)
> actually use. When no checksum is supplied (the common case for
> file:// recipe patches) needdonestamp stays False and behavior is
> unchanged - no .done / .lock files appear in DL_DIR for those urls.
> 
> For checksummed file:// urls, the donestamp + lockfile do land in
> DL_DIR (keyed by basename, alongside the existing http(s) state
> for the same basename); this is intentional and matches how every
> other fetcher tracks verified content.
> 
> An earlier fix (b8b14d975a25 "fetch2: Ensure we don't have file
> downloads overwriting each other", 2017) added a short-circuit at
> the top of verify_donestamp() so that when origud is a file:// url
> with needdonestamp=False, the function returns True early. That
> short-circuit was protecting the SSTATE_MIRRORS case where a
> file:// url is mapped to an http:// mirror and the http path would
> otherwise race against the file:// origud. It still does the
> right thing under this change: the only file:// origud whose
> needdonestamp now flips to True is one that carries an explicit
> checksum parameter, and in that case we actively want the mirror
> to run verify_donestamp(), not bypass it. The unchecksummed
> file:// origud case (the original SSTATE_MIRRORS scenario) keeps
> needdonestamp=False and the 2017 short-circuit fires unchanged.
> 
> Two related changes in fetch2 are worth pointing at:
> 
> * commit 6424f4b7e9c1 ("fetch2: Ensure mirror tarballs don't enforce
>   checksum", 2022) marks SSTATE_MIRRORS targets with
>   newud.ignore_checksums = True. Its commit message states "local
>   file fetches now validate checksums" - this fix is what finally
>   makes that statement true. The ignore_checksums flag also acts as
>   the safety hatch that prevents flipping needdonestamp on for
>   checksummed file:// urls from propagating verification onto
>   mirror-of-git-repo tarballs (whose checksums by design do not
>   match).
> * commit 4b8de2e7d126 ("wget: Avoid bad checksum race issues",
>   2022) established the don't-mutate-on-mismatch doctrine for
>   downloaded artifacts via the localpath= and fatal_nochecksum=
>   parameters. The rename_bad_checksum() early-return below applies
>   the same doctrine to a fetcher whose 'localpath' is the user's
>   source tree.
> 
> A checksum mismatch on a file:// url must not mutate the user's
> source tree the way it does for downloaded artifacts in DL_DIR.
> The standard error path renames <localpath> to
> <localpath>_bad-checksum_<sha> via rename_bad_checksum(), which
> for file:// would rename a recipe-shipped patch or an absolute
> path the user had supplied. Skip that rename inside
> rename_bad_checksum() when ud.type == 'file' so all four call
> sites stay consistent and a ChecksumError surfaces cleanly without
> on-disk side effects.
> 
> Add unit tests in FetcherLocalTest covering: the no-checksum hot
> path keeps needdonestamp=False (no regression for recipe patches);
> matching and mismatching sha256 and md5 checksums; the named
> ;name=foo;foo.sha256sum= form; and that a directory url with a
> checksum is silently ignored (supports_checksum() is False for
> directories). The mismatch tests assert that the source file is
> not renamed.
> 
> Link: https://bugzilla.yoctoproject.org/show_bug.cgi?id=9993
> [YOCTO #9993]
> 
> Signed-off-by: Jhonata Poma-Hansen <[email protected]>

Hi,

The commit message here is incredibly verbose, to the point that it is
difficult to read. While being verbose, it fails to mention that
UNINATIVE_URL uses 'https://' in our metadata and so the issue only
arises when you replace this with a local 'file://' URL (this is
mentioned in the description of issue #9993).

Please simplify the commit message and focus on the relevant
information. 

> ---
>  lib/bb/fetch2/__init__.py |   8 ++++
>  lib/bb/fetch2/local.py    |  19 ++++++++-
>  lib/bb/tests/fetch.py     | 104 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index 52d5556d..35874e25 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -1075,6 +1075,14 @@ def rename_bad_checksum(ud, suffix):
>      if ud.localpath is None:
>          return
>  
> +    # For file:// URLs the localpath points at the user's source tree
> +    # (e.g. recipe-shipped files under FILESPATH or an absolute path the
> +    # user passed in). Renaming those out from under the user is
> +    # destructive and surprising; surface the ChecksumError without
> +    # mutating their files.
> +    if ud.type == 'file':
> +        return

We should not be adding special cases for specific fetchers in
__init__.py. Perhaps we could instead check if ud.localpath is under
DL_DIR, or move this function to the FetchMethod class so that the Local
fetcher can override it with an empty function.

> +
>      new_localpath = "%s_bad-checksum_%s" % (ud.localpath, suffix)
>      bb.warn("Renaming %s to %s" % (ud.localpath, new_localpath))
>      if not bb.utils.movefile(ud.localpath, new_localpath):
> diff --git a/lib/bb/fetch2/local.py b/lib/bb/fetch2/local.py
> index fda56a56..2abcc5eb 100644
> --- a/lib/bb/fetch2/local.py
> +++ b/lib/bb/fetch2/local.py
> @@ -17,7 +17,7 @@ import os
>  import urllib.request, urllib.parse, urllib.error
>  import bb
>  import bb.utils
> -from   bb.fetch2 import FetchMethod, FetchError, ParameterError
> +from   bb.fetch2 import CHECKSUM_LIST, FetchMethod, FetchError, ParameterError
>  from   bb.fetch2 import logger
>  
>  class Local(FetchMethod):
> @@ -31,7 +31,22 @@ class Local(FetchMethod):
>          # We don't set localfile as for this fetcher the file is already local!
>          ud.basename = os.path.basename(ud.path)
>          ud.basepath = ud.path
> -        ud.needdonestamp = False
> +        # Without a donestamp the verify_checksum path is bypassed entirely
> +        # for file:// urls, so an explicit checksum on the url (for instance
> +        # UNINATIVE_CHECKSUM expanded into the uninative tarball file:// url)
> +        # is never compared against the file's actual contents. Keep
> +        # needdonestamp at the FetchData default (True) when any of the
> +        # known checksum parameters is present, so the standard
> +        # donestamp + verify_checksum cycle runs and a mismatch raises
> +        # ChecksumError. When no checksum is supplied (the common case for
> +        # recipe patches under file://) needdonestamp stays False and
> +        # behavior is unchanged.
> +        name = ud.parm.get("name")
> +        ud.needdonestamp = any(
> +            (name and "%s.%ssum" % (name, a) in ud.parm)
> +            or "%ssum" % a in ud.parm
> +            for a in CHECKSUM_LIST
> +        )

The comment here is of about the right level of detail for the commit
message. A one or two line comment here to explain the conditional may
be useful, but more than that is probably too verbose.

>          if "*" in ud.path:
>              raise bb.fetch2.ParameterError("file:// urls using globbing are no longer supported. Please place the files in a directory and reference that instead.", ud.url)
>          return
> diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
> index 077f741e..3347c60d 100644
> --- a/lib/bb/tests/fetch.py
> +++ b/lib/bb/tests/fetch.py
> @@ -813,6 +813,110 @@ class FetcherLocalTest(FetcherTest):
>          tree = self.fetchUnpack(['file://archive.tar.bz2;subdir=bar;striplevel=1'])
>          self.assertEqual(tree, ['bar/c', 'bar/d', 'bar/subdir/e'])
>  
> +    def test_local_no_checksum_no_donestamp(self):
> +        # The common case: file:// recipe patches without a checksum
> +        # parameter must keep the historical "no donestamp tracking"
> +        # behavior so this fix does not regress the hot path. After
> +        # download(), no .done / .lock entries must appear in DL_DIR
> +        # for the bare file:// url.
> +        with open(os.path.join(self.localsrcdir, 'plain'), 'wb') as f:
> +            f.write(b"plain\n")
> +        fetcher = bb.fetch.Fetch(['file://plain'], self.d)
> +        ud = fetcher.ud[fetcher.urls[0]]
> +        self.assertFalse(ud.needdonestamp)
> +        # When needdonestamp is False, FetchData leaves ud.donestamp
> +        # unset (None); we assert that no .done / .lock entries are
> +        # written to DL_DIR for the bare file:// url after download.
> +        fetcher.download()
> +        for entry in os.listdir(self.dldir):
> +            self.assertFalse(entry.startswith('plain.done'))
> +            self.assertFalse(entry.startswith('plain.lock'))

I don't think this test case adds much value.

> +
> +    def test_local_checksum_match(self):
> +        # A correct sha256 must drive the verify_checksum cycle to
> +        # completion: download() returns without raising AND the
> +        # donestamp lands in DL_DIR. The donestamp is the on-disk
> +        # evidence that update_stamp() ran after verify_checksum()
> +        # passed - without the fix, needdonestamp=False short-circuits
> +        # update_stamp() and no donestamp is written even on "match".
> +        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)
> +        ud = fetcher.ud[fetcher.urls[0]]
> +        self.assertTrue(ud.needdonestamp)
> +        fetcher.download()
> +        self.assertTrue(os.path.exists(ud.donestamp))

This test case looks good, but the comment is again too verbose, please
simplify it.

> +
> +    def test_local_checksum_mismatch(self):
> +        content = b"file:// checksum mismatch test\n"
> +        srcpath = os.path.join(self.localsrcdir, 'sumfile')
> +        with open(srcpath, 'wb') as f:
> +            f.write(content)
> +        bad = "0" * 64
> +        fetcher = bb.fetch.Fetch(['file://sumfile;sha256sum=' + bad], self.d)
> +        with self.assertRaises(bb.fetch2.FetchError):
> +            fetcher.download()
> +        # The user's source tree must not be mutated. rename_bad_checksum
> +        # would otherwise leave a <path>_bad-checksum_<sha> sibling and
> +        # remove the original.
> +        self.assertTrue(os.path.exists(srcpath))
> +        with open(srcpath, 'rb') as f:
> +            self.assertEqual(f.read(), content)
> +        for entry in os.listdir(self.localsrcdir):
> +            self.assertNotIn('_bad-checksum_', entry)

This test case is mostly good. I don't think we need to loop through the
whole of the directory listing, can we just confirm that the
bad-checksum marker for sumfile doesn't exist?

> +
> +    def test_local_checksum_mismatch_md5(self):
> +        # Coverage for an algorithm other than sha256 so a regression
> +        # that special-cased sha256 detection does not slip through.
> +        # Mirror the sha256 mismatch assertions: source file present,
> +        # contents unchanged, no _bad-checksum_ sibling left behind.
> +        content = b"file:// md5 mismatch test\n"
> +        srcpath = os.path.join(self.localsrcdir, 'sumfile_md5')
> +        with open(srcpath, 'wb') as f:
> +            f.write(content)
> +        bad = "0" * 32
> +        fetcher = bb.fetch.Fetch(['file://sumfile_md5;md5sum=' + bad], self.d)
> +        with self.assertRaises(bb.fetch2.FetchError):
> +            fetcher.download()
> +        self.assertTrue(os.path.exists(srcpath))
> +        with open(srcpath, 'rb') as f:
> +            self.assertEqual(f.read(), content)
> +        for entry in os.listdir(self.localsrcdir):
> +            self.assertNotIn('_bad-checksum_', entry)

I don't think this test case adds value, the implementation is not
sha256sum specific.

> +
> +    def test_local_checksum_named(self):
> +        # UNINATIVE_CHECKSUM (the original reproducer for this bug) and
> +        # several other recipe-driven file:// urls use the name= prefix
> +        # form: foo.sha256sum= rather than bare sha256sum=. Confirm the
> +        # named form flips needdonestamp AND that the verify cycle runs
> +        # (donestamp on disk after download).
> +        import hashlib
> +        content = b"file:// named checksum test\n"
> +        with open(os.path.join(self.localsrcdir, 'sumfile_named'), 'wb') as f:
> +            f.write(content)
> +        good = hashlib.sha256(content).hexdigest()
> +        fetcher = bb.fetch.Fetch(
> +            ['file://sumfile_named;name=blob;blob.sha256sum=' + good], self.d)
> +        ud = fetcher.ud[fetcher.urls[0]]
> +        self.assertTrue(ud.needdonestamp)
> +        fetcher.download()
> +        self.assertTrue(os.path.exists(ud.donestamp))

This is a case of testing at the wrong level of detail. Do we already
have test coverage that setting foo.some_property for a SRC_URI entry
with name=foo results in the property being set for the appropriate
entry? If so, we don't need further test coverage here. If not, we
should add a generic test case for that. 

> +
> +    def test_local_checksum_directory_ignored(self):
> +        # Local supports directory urls; supports_checksum returns False
> +        # for directories. A checksum parm on a directory url should
> +        # therefore be a no-op rather than a hard error: verify_checksum
> +        # short-circuits on supports_checksum() and no ChecksumError is
> +        # raised even when the checksum value is bogus.
> +        bad = "0" * 64
> +        fetcher = bb.fetch.Fetch(['file://dir;sha256sum=' + bad], self.d)
> +        ud = fetcher.ud[fetcher.urls[0]]
> +        self.assertFalse(ud.method.supports_checksum(ud))
> +        fetcher.download()

This tests that we suppress an error that we should actually be raising.
If the checksum is invalid, then we should raise an error.

Best regards,

-- 
Paul Barker
signature.asc (application/pgp-signature, 252 B)