[PATCH V1] libebgenv: Fix a regression introduced in c2be7c1b95b4 ("libebgenv: fix memory leak in partition probing").

[INgo Rah]

Without this fix configurations cannot be read or written on
already mounted partitions because the filename is missing.
Freeing the memory is anyhow done in bgenv_finalize at the very end.

Signed-off-by: INgo Rah <ingo...@linutronix.de>

diff --git a/env/env_config_file.c b/env/env_config_file.c
index b0cf043..e859575 100644
--- a/env/env_config_file.c
+++ b/env/env_config_file.c
@@ -85,9 +85,6 @@ bool probe_config_file(CONFIG_PART *cfgpart)
}
if (do_unmount) {
unmount_partition(cfgpart);
- } else {
- free(cfgpart->mountpoint);
- cfgpart->mountpoint = NULL;
}
return result;
}

--

[Jan Kiszka]

Can you please explain the code path where this is causing a regression?
Are you calling probe_config_file() directly, or is it something
happening within plain libebgenv as well?

Jan

--
Siemens AG, Foundational Technologies
Linux Expert Center

[INgo Rah]

On Sat, 2025-08-02 at 11:42 +0200, Jan Kiszka wrote:
>
> Can you please explain the code path where this is causing a regression?
> Are you calling probe_config_file() directly, or is it something
> happening within plain libebgenv as well?
>

Hi, thanx for the reply.
I am using the standard efibootguard v0.19 debian package (sid) or latest v0.20.
I does not happen with v0.13 (bookworm).

The Use/Test case is the following 
- Config partions are mounted
- Call bg_printenv with none or any option (-c, -p 0)
- prints all fields set to 0, no error.

The issue:
'open_config_file_from_part' needs the mountpoint which gets freed before in
probe_config_file, so it returns NULL. Only if the partition is not mounted in read_env,
mount_partition gets called which sets the mountpoint for the function.

bg_printenv
├── bgenv_init
│ ├── probe_config_partitions
│ │ ├── probe_config_file
│ │ │ └ sets _and_ frees mountpoint
│ │ └ returns true
│ ├── read_env
│ │ ├── open_config_file_from_part
│ │ │ └ returns NULL because mountpoint not set
│ │ └ returns false
│ └ returns true (but failed, because return value not checked)
├── dump_envs
│ ├── bgenv_open_by_index
│ │ └ returns handle
│ ├── dump_env dumps empty values
└── bgenv_finalize
└── free(...mountpoint)

Regards, INgo

> Jan
>

[Michael Adler]

Hi,

> The Use/Test case is the following 
> - Config partions are mounted
> - Call bg_printenv with none or any option (-c, -p 0)
> - prints all fields set to 0, no error.

I just tried this and could reproduce the issue. The suggested patch resolves the problem.

Tested-by: Michael Adler <michae...@siemens.com>

Kind regards,
Michael

--
Michael Adler

Siemens AG
Technology
Connectivity & Edge
Open Source Embedded Systems
FT RPD CED OES-DE
Friedrich-Ludwig-Bauer-Str. 3
85748 Garching, Germany

Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Jim Hagemann
Snabe; Managing Board: Roland Busch, Chairman, President and Chief Executive
Officer; Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese;
Registered offices: Berlin and Munich, Germany; Commercial registries:
Berlin-Charlottenburg, HRB 12300, Munich, HRB 6684; WEEE-Reg.-No. DE 23691322

[Jan Kiszka]

On 04.08.25 16:42, Michael Adler wrote:
> Hi,
>
>> The Use/Test case is the following 
>> - Config partions are mounted
>> - Call bg_printenv with none or any option (-c, -p 0)
>> - prints all fields set to 0, no error.
>
> I just tried this and could reproduce the issue. The suggested patch resolves the problem.
>
> Tested-by: Michael Adler <michae...@siemens.com>
>

But all this is still missing the reasoning why the original issue
reported by Felix was never an issue. Just reverting is too simple.
Reading [1] again, we at least need to address release issues when
probing fails somewhere.

Yeah, our APIs are horribly designed, making all this not a simple task...

Jan

[1]
https://groups.google.com/g/efibootguard-dev/c/iYkr2K0ywl4/m/2mRRAlQtBgAJ

[Jan Kiszka]

I suspect this is the real fix:

diff --git a/env/env_config_file.c b/env/env_config_file.c
index b0cf043..03c618a 100644
--- a/env/env_config_file.c
+++ b/env/env_config_file.c
@@ -85,7 +85,7 @@ bool probe_config_file(CONFIG_PART *cfgpart)

}
if (do_unmount) {
unmount_partition(cfgpart);
- } else {

+ } else if (!result) {
free(cfgpart->mountpoint);
cfgpart->mountpoint = NULL;
}

Jan

[INgo Rah]

On Tue, 2025-08-05 at 08:07 +0200, Jan Kiszka wrote:
>
>
> I suspect this is the real fix:
>
> diff --git a/env/env_config_file.c b/env/env_config_file.c
> index b0cf043..03c618a 100644
> --- a/env/env_config_file.c
> +++ b/env/env_config_file.c
> @@ -85,7 +85,7 @@ bool probe_config_file(CONFIG_PART *cfgpart)
>                 }
>                 if (do_unmount) {
>                         unmount_partition(cfgpart);
> -               } else {
> +               } else if (!result) {
>                         free(cfgpart->mountpoint);
>                         cfgpart->mountpoint = NULL;
>                 }
>
>

Yes, that fix is also working for me. Wasn't sure about the intention of that original change.
Another thing could be, to avoid printing "empty" values in case of an error...means handling the
return of read_env ... But I don't see this as very important..

> Jan
>