[PATCH] fix(env_config_file): memory leak if ebg partitions are already mounted

[Hosgor, Tolga (IOT DS EU TR MTS)]

From: Hosgor, Tolga (IOT DS EU TR MTS) <tolga....@siemens.com>

In cases where get_mountpoint() returns non-null, do_unmount is not set
to true and umount_partition() is not called. This results in
cfgpart->mountpoint to not be freed.

Signed-off-by: Hosgor, Tolga (IOT DS EU TR MTS) <tolga....@siemens.com>
---
env/env_config_file.c | 3 +++
tools/tests/test_probe_config_file.c | 1 -
2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/env/env_config_file.c b/env/env_config_file.c
index 873fe10..9b96f20 100644
--- a/env/env_config_file.c
+++ b/env/env_config_file.c
@@ -88,6 +88,9 @@ bool probe_config_file(CONFIG_PART *cfgpart)
}
if (do_unmount) {
unmount_partition(cfgpart);
+ } else {
+ free(cfgpart->mountpoint);
+ cfgpart->mountpoint = NULL;
}
return result;
}
diff --git a/tools/tests/test_probe_config_file.c b/tools/tests/test_probe_config_file.c
index f1da154..9b1a176 100644
--- a/tools/tests/test_probe_config_file.c
+++ b/tools/tests/test_probe_config_file.c
@@ -120,7 +120,6 @@ bool __wrap_probe_config_file(CONFIG_PART *cp)
cp->not_mounted = false;
ret = __real_probe_config_file(cp);

- free(cp->mountpoint);
return ret;
}

--
2.24.1

[Jan Kiszka]

On 24.12.19 11:40, Hosgor, Tolga (IOT DS EU TR MTS) wrote:
> From: Hosgor, Tolga (IOT DS EU TR MTS) <tolga....@siemens.com>
>
> In cases where get_mountpoint() returns non-null, do_unmount is not set
> to true and umount_partition() is not called. This results in
> cfgpart->mountpoint to not be freed.
>
> Signed-off-by: Hosgor, Tolga (IOT DS EU TR MTS) <tolga....@siemens.com>
> ---
> env/env_config_file.c | 3 +++
> tools/tests/test_probe_config_file.c | 1 -
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/env/env_config_file.c b/env/env_config_file.c
> index 873fe10..9b96f20 100644
> --- a/env/env_config_file.c
> +++ b/env/env_config_file.c
> @@ -88,6 +88,9 @@ bool probe_config_file(CONFIG_PART *cfgpart)
> }
> if (do_unmount) {
> unmount_partition(cfgpart);
> + } else {
> + free(cfgpart->mountpoint);
> + cfgpart->mountpoint = NULL;

This looks wrong. There are places in the code that uses
cfgpart->pointpoint after probe_config_file in case cfgpart->not_mounted
is false. Check env/env_api_fat.c.

I rather suspect, we are missing a counterpart to bgenv_init() that
cleans config_parts up.

Jan

[Tolga Hoşgör]

On Friday, December 27, 2019 at 2:33:19 PM UTC+3, Jan Kiszka wrote:

On 24.12.19 11:40, Hosgor, Tolga (IOT DS EU TR MTS) wrote:

> From: Hosgor, Tolga (IOT DS EU TR MTS) <tolga...@siemens.com>

>
> In cases where get_mountpoint() returns non-null, do_unmount is not set
> to true and umount_partition() is not called. This results in
> cfgpart->mountpoint to not be freed.
>

> Signed-off-by: Hosgor, Tolga (IOT DS EU TR MTS) <tolga...@siemens.com>

> ---
>   env/env_config_file.c                | 3 +++
>   tools/tests/test_probe_config_file.c | 1 -
>   2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/env/env_config_file.c b/env/env_config_file.c
> index 873fe10..9b96f20 100644
> --- a/env/env_config_file.c
> +++ b/env/env_config_file.c
> @@ -88,6 +88,9 @@ bool probe_config_file(CONFIG_PART *cfgpart)
>                   }
>                   if (do_unmount) {
>                           unmount_partition(cfgpart);
> +                } else {
> +                        free(cfgpart->mountpoint);
> +                        cfgpart->mountpoint = NULL;

This looks wrong.

 

There are places in the code that uses
cfgpart->pointpoint after probe_config_file in case cfgpart->not_mounted
is false. Check env/env_api_fat.c.

I see that now.

I rather suspect, we are missing a counterpart to bgenv_init() that
cleans config_parts up.

Yes indeed. Later I found out that `devpath` is also leaking due to repeated calls to bgenv_init. Handling both in the same place would be better but I could not really decide how to implement that (where to call it). Instead, I have set the CONFIG_PARTs to {0} and tried to free them prior to memset in bgenv_init as a dirty workaround.

[Jan Kiszka]

On 27.12.19 15:12, Tolga Hoşgör wrote:
>
>
> On Friday, December 27, 2019 at 2:33:19 PM UTC+3, Jan Kiszka wrote:
>
> On 24.12.19 11:40, Hosgor, Tolga (IOT DS EU TR MTS) wrote:
> > From: Hosgor, Tolga (IOT DS EU TR MTS) <tolga...@siemens.com

> <javascript:>>

> >
> > In cases where get_mountpoint() returns non-null, do_unmount is
> not set
> > to true and umount_partition() is not called. This results in
> > cfgpart->mountpoint to not be freed.
> >
> > Signed-off-by: Hosgor, Tolga (IOT DS EU TR MTS)

> <tolga...@siemens.com <javascript:>>

Any updates on this issues?

Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[Hosgor, Tolga (IOT DS EU TR MTS)]

From: Hosgor, Tolga (IOT DS EU TR MTS) <tolga....@siemens.com>

bgenv_init() may be called multiple times during the operation of ebg
library. This causes devpath and mountpoint to be leaked.

Signed-off-by: Hosgor, Tolga (IOT DS EU TR MTS) <tolga....@siemens.com>
---
This is what we went with due to the lack of time to properly
understand source and introduce _destroy counterpart to bgenv_init

env/env_api_fat.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 9f2a5cd..ae3f25f 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -125,11 +125,15 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)
return result;
}

-CONFIG_PART config_parts[ENV_NUM_CONFIG_PARTS];
+CONFIG_PART config_parts[ENV_NUM_CONFIG_PARTS] = {0};
BG_ENVDATA envdata[ENV_NUM_CONFIG_PARTS];

bool bgenv_init()
{
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ free(config_parts[i].devpath);
+ free(config_parts[i].mountpoint);
+ }
memset((void *)&config_parts, 0,
sizeof(CONFIG_PART) * ENV_NUM_CONFIG_PARTS);
/* enumerate all config partitions */
--
2.27.0