[PATCH] libblkid: Fix crash while parsing config with libeconf

[PATCH] libblkid: Fix crash while parsing config with libeconf

[Stanislav Brabec]

As the whhole econf_file structure is freed by econf_free(file) at the end
of blkid_read_config, econf_file structure cannot be defined as static and
initialized only once. The econf_free() is not robust enough and keeps a
pointer to the garbage after the first call. And if /etc/blkid.conf does
not exist, it is called second time.

Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
Cc: Stefan Schubert <schubi@suse.de>
---
 libblkid/src/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libblkid/src/config.c b/libblkid/src/config.c
index 934d28d65..39024b42e 100644
--- a/libblkid/src/config.c
+++ b/libblkid/src/config.c
@@ -154,7 +154,7 @@ struct blkid_config *blkid_read_config(const char *filename)
 
 #else /* !HAVE_LIBECONF */
 
-	static econf_file *file = NULL;
+	econf_file *file = NULL;
 	char *line = NULL;
 	bool uevent = false;
 	econf_err error;
-- 
2.48.1

Re: [PATCH] libblkid: Fix crash while parsing config with libeconf

[Stanislav Brabec]

I forgot to mention:
https://bugzilla.opensuse.org/show_bug.cgi?id=1242705

with a full reproducer and backtrace

Re: [PATCH] libblkid: Fix crash while parsing config with libeconf

[Stanislav Brabec]

Stanislav Brabec wrote:
>As the whhole econf_file structure is freed by econf_free(file) at the end

> of blkid_read_config, econf_file structure cannot be defined as static and
> initialized only once. The econf_free() is not robust enough and keeps a
> pointer to the garbage after the first call. And if /etc/blkid.conf does
> not exist, it is called second time.

However the patch is correct and fixes the crash, there are still open questions:

- Why blkid_read_config() and econf_readConfig() are called twice with the same parameters? Is it intended behavior?

- If yes, why we don't recycle the configuration and call econf_free()?
- If not, why it happens?
- And finally, is a similar code in logindefs.c vulnerable to a similar type of crash?


-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Křižíkova 148/34 (Corso IIa)                    tel: +420 284 084 060
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] libblkid: Fix crash while parsing config with libeconf

[Karel Zak]

On Tue, May 20, 2025 at 04:24:07PM +0200, Stanislav Brabec wrote:
> Stanislav Brabec wrote:
> > As the whhole econf_file structure is freed by econf_free(file) at the end
> 
> > of blkid_read_config, econf_file structure cannot be defined as static and
> > initialized only once. The econf_free() is not robust enough and keeps a
> > pointer to the garbage after the first call. And if /etc/blkid.conf does
> > not exist, it is called second time.
> 
> However the patch is correct and fixes the crash, there are still open questions:
> 
> - Why blkid_read_config() and econf_readConfig() are called twice with the same parameters? Is it intended behavior?

This code pattern (e.g., libblkid/src/evaluate.c):

     conf = blkid_read_config(NULL);
     ...

     cachefile = blkid_get_cache_filename(conf);
     rc = blkid_get_cache(&c, cachefile);

If blkid_get_cache_filename() returns NULL, then blkid_get_cache()
reads the configuration again. Additionally, blkid_get_cache_filename() can read the
configuration if 'conf' is NULL.

Yes, it's not elegant.

> - And finally, is a similar code in logindefs.c vulnerable to a similar type of crash?

The `logindefs` uses a global `list` variable, which should be filled only once.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] libblkid: Fix crash while parsing config with libeconf

[Karel Zak]

On Wed, May 21, 2025 at 08:55:37AM +0200, Karel Zak wrote:
> On Tue, May 20, 2025 at 04:24:07PM +0200, Stanislav Brabec wrote:
> > Stanislav Brabec wrote:
> > > As the whhole econf_file structure is freed by econf_free(file) at the end
> > 
> > > of blkid_read_config, econf_file structure cannot be defined as static and
> > > initialized only once. The econf_free() is not robust enough and keeps a
> > > pointer to the garbage after the first call. And if /etc/blkid.conf does
> > > not exist, it is called second time.
> > 
> > However the patch is correct and fixes the crash, there are still open questions:
> > 
> > - Why blkid_read_config() and econf_readConfig() are called twice with the same parameters? Is it intended behavior?
> 
> This code pattern (e.g., libblkid/src/evaluate.c):
> 
>      conf = blkid_read_config(NULL);
>      ...
> 
>      cachefile = blkid_get_cache_filename(conf);
>      rc = blkid_get_cache(&c, cachefile);
> 
> If blkid_get_cache_filename() returns NULL, then blkid_get_cache()
> reads the configuration again. Additionally, blkid_get_cache_filename() can read the
> configuration if 'conf' is NULL.
> 
> Yes, it's not elegant.

Added to TODO:
https://github.com/util-linux/util-linux/issues/3580

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com