| Message ID | 20260727172856.2304118-1-amery@apptly.co |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] bitbake-worker: Evaluate exported_vars() before emptying the environment | expand |
On Mon, 2026-07-27 at 17:28 +0000, Alejandro Mery via lists.openembedded.org wrote: > exported_vars() returns a lazy generator. It was bound before > empty_environment() cleared the process environment and only iterated > afterwards, so any datastore expansion deferred until iteration ran > against the wiped environment rather than the one bitbake started with. > > The corruption is not uniform. The loop writes each variable into > os.environ as it goes, so only the first value expands against a fully > empty environment and the rest see it partially rebuilt, in > exported_keys() order. > > Recipes whose exported variables expand a command during that loop hit > this. A gitver-style PV such as "${@get_git_pv(d, ...)}" runs git while > PATH is empty, so the git wrapper on PATH is bypassed and the real git > runs directly. Under pseudo this fakes uid 0 against a repository owned > by the real user, and git aborts with "detected dubious ownership", > failing do_package intermittently (only on reparse, when the value is > re-expanded rather than served from cache). > > Materialise the generator into a list before emptying the environment so > every expansion happens while that environment is still intact. Add > tests for the expansion timing this relies on: values materialised > before empty_environment() survive it, a generator consumed after it > does not. > > - v2: comment the ordering at the call site, and add the tests. > - v1: the list() wrap alone. > > Signed-off-by: Alejandro Mery <amery@apptly.co> > --- > bin/bitbake-worker | 5 ++++- > lib/bb/tests/data.py | 42 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+), 1 deletion(-) Thanks for the changes. I'm worried that the test case doesn't cover what I'd ideally like it to, specifically that bitbake-worker's environment can't get corrupted. The code in bitbake-worker can be changed independently of the test case and break things, which means the test isn't protecting against what I most fear happening. I did a bit more research and thinking about this and I think I might suggest a new approach. I've noticed that this is the only user of exported_vars() in the codebase, so perhaps we just stop returning a generator? It does appear to be causing a lot more problems than it solves, particularly if we just turn it into a list anyway! If we change things that way, it seems much more unlikely we can break it in future and the need for the comments is removed. Does that seem reasonable? Sorry about the change in direction but this patch just doesn't look/feel right now I realise there is only one user of the function. Cheers, Richard
On 28/07/2026 22:15, Richard Purdie wrote: > On Mon, 2026-07-27 at 17:28 +0000, Alejandro Mery via lists.openembedded.org wrote: >> exported_vars() returns a lazy generator. It was bound before >> empty_environment() cleared the process environment and only iterated >> afterwards, so any datastore expansion deferred until iteration ran >> against the wiped environment rather than the one bitbake started with. >> >> The corruption is not uniform. The loop writes each variable into >> os.environ as it goes, so only the first value expands against a fully >> empty environment and the rest see it partially rebuilt, in >> exported_keys() order. >> >> Recipes whose exported variables expand a command during that loop hit >> this. A gitver-style PV such as "${@get_git_pv(d, ...)}" runs git while >> PATH is empty, so the git wrapper on PATH is bypassed and the real git >> runs directly. Under pseudo this fakes uid 0 against a repository owned >> by the real user, and git aborts with "detected dubious ownership", >> failing do_package intermittently (only on reparse, when the value is >> re-expanded rather than served from cache). >> >> Materialise the generator into a list before emptying the environment so >> every expansion happens while that environment is still intact. Add >> tests for the expansion timing this relies on: values materialised >> before empty_environment() survive it, a generator consumed after it >> does not. >> >> - v2: comment the ordering at the call site, and add the tests. >> - v1: the list() wrap alone. >> >> Signed-off-by: Alejandro Mery <amery@apptly.co> >> --- >> bin/bitbake-worker | 5 ++++- >> lib/bb/tests/data.py | 42 ++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 46 insertions(+), 1 deletion(-) > Thanks for the changes. > > I'm worried that the test case doesn't cover what I'd ideally like it > to, specifically that bitbake-worker's environment can't get corrupted. > The code in bitbake-worker can be changed independently of the test > case and break things, which means the test isn't protecting against > what I most fear happening. true > > I did a bit more research and thinking about this and I think I might > suggest a new approach. I've noticed that this is the only user of > exported_vars() in the codebase, so perhaps we just stop returning a > generator? > > It does appear to be causing a lot more problems than it solves, > particularly if we just turn it into a list anyway! > > If we change things that way, it seems much more unlikely we can break > it in future and the need for the comments is removed. > > Does that seem reasonable? Sorry about the change in direction but this > patch just doesn't look/feel right now I realise there is only one user > of the function. absolutely. I blindly assumed you wanted it to be a generator. so, for v3 exported_vars() returns a materialised list. what would you want the tests t do?
On Wed, 2026-07-29 at 01:28 +0100, Alejandro Mery wrote: > On 28/07/2026 22:15, Richard Purdie wrote: > > On Mon, 2026-07-27 at 17:28 +0000, Alejandro Mery via lists.openembedded.org wrote: > > > > I did a bit more research and thinking about this and I think I > > might > > suggest a new approach. I've noticed that this is the only user of > > exported_vars() in the codebase, so perhaps we just stop returning > > a > > generator? > > > > It does appear to be causing a lot more problems than it solves, > > particularly if we just turn it into a list anyway! > > > > If we change things that way, it seems much more unlikely we can > > break > > it in future and the need for the comments is removed. > > > > Does that seem reasonable? Sorry about the change in direction but > > this > > patch just doesn't look/feel right now I realise there is only one > > user > > of the function. > > absolutely. I blindly assumed you wanted it to be a generator. > > so, for v3 exported_vars() returns a materialised list. what would > you want the tests t do? We probably don't need the test case now the problem we were trying to protect against isn't there. Thanks, Richard
diff --git a/bin/bitbake-worker b/bin/bitbake-worker index 4ad716975..b8dc9c862 100755 --- a/bin/bitbake-worker +++ b/bin/bitbake-worker @@ -296,7 +296,10 @@ def fork_off_task(cfg, data, databuilder, workerdata, extraconfigdata, runtask): # exported_vars() returns a generator which *cannot* be passed to os.environ.update() # successfully. We also need to unset anything from the environment which shouldn't be there - exports = bb.data.exported_vars(the_data) + # Iterating it expands each value, so materialise the generator before + # empty_environment() below: an expansion deferred past that point runs + # against the wiped environment, not the one bitbake started with. + exports = list(bb.data.exported_vars(the_data)) bb.utils.empty_environment() for e, v in exports: diff --git a/lib/bb/tests/data.py b/lib/bb/tests/data.py index fd690a9e2..06c88ffc6 100644 --- a/lib/bb/tests/data.py +++ b/lib/bb/tests/data.py @@ -723,3 +723,45 @@ class EmitVar(unittest.TestCase): self.assertEqual(self.get_output(out), ['bad_chars="a\\"b \\', 'c\\`d \\', 'e\\$f"']) + +class ExportedVars(unittest.TestCase): + """bb.data.exported_vars() expands each value as it is iterated. + + bitbake-worker's fork_off_task() therefore materialises the generator + with list() before it calls bb.utils.empty_environment(): an exported + variable whose value expands from the environment (e.g. a gitver-style + PV that runs git and needs the real PATH) has to be expanded while that + environment still exists. bitbake-worker runs on import (only its + version check sits behind a __main__ guard), so these tests reproduce + its sequence against bb.data and pin the behaviour the worker relies + on, not the worker itself. + """ + + def setUp(self): + self.saved_environment = dict(os.environ) + os.environ["BB_TEST_EXPORTED_VARS"] = "intact" + + self.d = bb.data.init() + self.d.setVar("TESTEXPORT", + "${@os.environ.get('BB_TEST_EXPORTED_VARS', 'wiped')}") + self.d.setVarFlag("TESTEXPORT", "export", "1") + + def tearDown(self): + # empty_environment() wipes the environment of this process, not a + # copy of it, so put it back whatever the test did. + os.environ.clear() + os.environ.update(self.saved_environment) + + def test_materialised_before_empty_environment(self): + # What bitbake-worker does: list() expands every value while the + # environment is still intact, so the wipe cannot reach them. + exports = list(bb.data.exported_vars(self.d)) + bb.utils.empty_environment() + self.assertEqual(dict(exports)["TESTEXPORT"], "intact") + + def test_lazy_generator_expands_after_empty_environment(self): + # Why the list() is required: a generator bound before the wipe but + # consumed after it expands against the wiped environment instead. + exports = bb.data.exported_vars(self.d) + bb.utils.empty_environment() + self.assertEqual(dict(exports)["TESTEXPORT"], "wiped")
exported_vars() returns a lazy generator. It was bound before empty_environment() cleared the process environment and only iterated afterwards, so any datastore expansion deferred until iteration ran against the wiped environment rather than the one bitbake started with. The corruption is not uniform. The loop writes each variable into os.environ as it goes, so only the first value expands against a fully empty environment and the rest see it partially rebuilt, in exported_keys() order. Recipes whose exported variables expand a command during that loop hit this. A gitver-style PV such as "${@get_git_pv(d, ...)}" runs git while PATH is empty, so the git wrapper on PATH is bypassed and the real git runs directly. Under pseudo this fakes uid 0 against a repository owned by the real user, and git aborts with "detected dubious ownership", failing do_package intermittently (only on reparse, when the value is re-expanded rather than served from cache). Materialise the generator into a list before emptying the environment so every expansion happens while that environment is still intact. Add tests for the expansion timing this relies on: values materialised before empty_environment() survive it, a generator consumed after it does not. - v2: comment the ordering at the call site, and add the tests. - v1: the list() wrap alone. Signed-off-by: Alejandro Mery <amery@apptly.co> --- bin/bitbake-worker | 5 ++++- lib/bb/tests/data.py | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-)