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

[Quentin Schulz]

Hi Wojciech,

On 2/3/26 9:17 AM, Wojciech Dubowik wrote:
> On Tue, Jan 27, 2026 at 04:00:44PM +1300, Simon Glass wrote:
> Hi Simon,
>> 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.
> Would it be ok to call softhsm utils with softhsm_conf like
> SOFTHSM2_CONF=... softhsm2-util? Then we don't have to mock environment.
> I would still have to set SOFTHSM install dir location with the env variable
> for mkeficapsule in binman generation. I guess it doesn't affect other
> tests as we don't need to change it.
> 

I would recommend to hook this to command.run_pipe(..., env=env, ...) in 
tools/binman/bintool.py:Bintool:run_cmd_result() if possible, such that 
it isn't bintool-specific.

We already override the env with the path to our tools, so we could 
simply extend it. I guess we could have a Bintool:set_env() or 
add_to_env() and store the result in some variable (have 
tools.get_env_with_path() in it by default in the object init?) and have 
run_cmd_result() use the bintool object's self.env dict insted of 
calling tools.get_env_with_path()? Then in ftest.py, after b = 
bintool.Bintool.create(), b.add_to_env({'SOFTHSM2_CONF': '...'})?

I'm not sure, maybe there's a reason tools.get_env_with_path() is called 
at the moment the command needs to be run and not when the bintool is 
created, in which case maybe store extra_env and update the env returned 
by tools.get_env_with_path() with that?

Cheers,
Quentin