EXTERNAL - [PATCH v4 6/6] test: binman: Add test for pkcs11 signed capsule

[Simon Glass]

Hi Quentin,

On Tue, 27 Jan 2026 at 00:43, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Simon,
>
> On 1/22/26 11:46 PM, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 22 Jan 2026 at 02:06, Quentin Schulz <quentin.schulz at cherry.de> wrote:
> >>
> >> Hi Wojciech,
> >>
> >> On 1/21/26 1:43 PM, Wojciech Dubowik wrote:
> >>> On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote:
> >>> Hello Quentin,
> >>>> Hi Wojciech,
> >>>>
> >>>> On 1/20/26 9:12 AM, Wojciech Dubowik wrote:
> >> [...]
> >>>>> +        os.environ['SOFTHSM2_CONF'] = softhsm2_conf
> >>>>
> >>>> This is wrong, you'll be messing up with the environment of all tests being
> >>>> run in the same thread. You must use the "with
> >>>> unittest.mock.patch.dict('os.environ'," implementation I used in
> >>>> testFitSignPKCS11Simple.
> >>>
> >>> Well, I have done so in my V2 but has been commented as wrong by the
> >>> first reviewer. I will restore it back.
> >>>
> >>
> >> Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't
> >> how we should be doing it.
> >>
> >> This is likely fine on its own because there's only one test that is now
> >> modifying os.environ's SOFTHSM2_CONF but this will be a problem next
> >> time a test wants to modify it. I actually hit this issue when
> >> developing the PKCS11 fit signing tests as I had two tests modifying the
> >> environment.
> >>
> >> The only trace of it left is the changelog in
> >> https://lore.kernel.org/u-boot/20251121-binman-engine-v3-0-b80180aaa783@cherry.de/
> >>
> >> """
> >> - fixed issues due to modification of the environment in tests failing
> >>     other tests, by using unittest.mock.patch.dict() on os.environ as
> >>     suggested by the unittest.mock doc,
> >> """
> >>
> >> and you can check the diff between the v2 and v3 to check I used to
> >> modify the env directly but now mock it instead.
> >>
> >> Sorry for not catching this, should have answered to Simon in the v2.
> >
> > In practice we try to set values for various which are important, so
> > future tests should explicitly delete the var if needed. But I am OK
>
> This is not working. See this very simple example (too lazy to use
> threading.Lock so synchronization done via time.sleep instead):
>
> """
> #!/usr/bin/env python3
>
> import os
> import time
> import threading
>
>
> def thread_func(n):
>      if n == 1:
>          time.sleep(1)
>      print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
>      if n == 1:
>          time.sleep(1)
>      print(f'Thread {n} set environ var FOO to foo{n}')
>      os.environ['FOO'] = f'foo{n}'
>      if n == 0:
>          time.sleep(5)
>      print(f'Thread {n} read environ var FOO={os.environ["FOO"]}')
>      if n == 0:
>          print(f'Thread {n} removes environ var FOO')
>          del os.environ["FOO"]
>      else:
>          time.sleep(10)
>          print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>
>
> threads = []
>
> os.environ["FOO"] = "foo"
>
> for i in range(0, 2):
>      t = threading.Thread(target=thread_func, args=(i,))
>      threads.append(t)
>
> for t in threads:
>      t.start()
>
> for t in threads:
>      t.join()
>
> """
>
> This results in:
>
> """
> Thread 0 read environ var FOO=foo
> Thread 0 set environ var FOO to foo0
> Thread 1 read environ var FOO=foo0
> Thread 1 set environ var FOO to foo1
> Thread 1 read environ var FOO=foo1
> Thread 0 read environ var FOO=foo1
> Thread 0 removes environ var FOO
> Thread 1 read environ var FOO=None
> """
>
> You see that modification made to os.environ in a different thread
> impacts the other threads. A test should definitely NOT modify anything
> for another test, especially not when it's already running.
>
> So now, I implemented mocking instead (like in my tests for PKCS11 in
> tools/binman/ftest.py) because I know it works.
>
> See:
>
> """
> #!/usr/bin/env python3
>
> import os
> import time
> import threading
> import unittest.mock
>
>
> def thread_func(n):
>      if n == 1:
>          time.sleep(1)
>      print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>      if n == 1:
>          time.sleep(1)
>      with unittest.mock.patch.dict('os.environ',
>                                    {'FOO': f'foo{n}'}):
>          print(f'Thread {n} set environ var FOO to foo{n}')
>          if n == 0:
>              time.sleep(5)
>          print(f'Thread {n} read mocked environ var
> FOO={os.environ.get("FOO")}')
>          if n == 1:
>              time.sleep(6)
>      print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}')
>
>
> threads = []
>
> for i in range(0, 2):
>      t = threading.Thread(target=thread_func, args=(i,))
>      threads.append(t)
>
> for t in threads:
>      t.start()
>
> for t in threads:
>      t.join()
> """
>
> Lo and behold, it.... does NOT work???????
>
> I get:
>
> """
> Thread 0 read environ var FOO=None
> Thread 0 set environ var FOO to foo0
> Thread 1 read environ var FOO=foo0
> Thread 1 set environ var FOO to foo1
> Thread 1 read mocked environ var FOO=foo1
> Thread 0 read mocked environ var FOO=foo1
> Thread 0 read environ var FOO=None
> Thread 1 read environ var FOO=foo0
> """
>
> I've read that os.environ isn't thread-safe, due to setenv() in glibc
> not being thread-safe:
> https://sourceware.org/glibc/manual/latest/html_node/Environment-Access.html#Environment-Access-1
>
> """
> Modifications of environment variables are not allowed in multi-threaded
> programs.
> """
>
> I'm not sure if this applies to any other Python implementation than
> CPython? But that is likely the one that most people are using.
>
> So... In short, I'm at a loss, no clue how to fix this (if it is even
> fixable). The obvious answer is "spawn multiple processes instead of
> multiple threads" but I can guarantee you, you don't want to be going
> this route as multiprocessing is a lot of headaches in Python. We could
> have the Python thread spawn a subprocess which has a different
> environment if we wanted to (via the `env` command for example), but
> that means not using binman Python API, rather its CLI. We could have
> bintools accept an environment dict that needs to be passed via the
> `env` command or the `env` kwargs of subprocess.Popen().
>
> Headaches, headaches.

I would argue that requiring a particular environment var for a test
is not a great idea. Better to pass an argument through the call stack
and have the external program run with a new environment containing
what is needed.

Regards,
Simon