[parted-devel] BUG: ped_exception_throw() can go to endless loop allocating memory

[Joel Granados]

----- "Petr Uzel" <petr.uzel at suse.cz> wrote:

> Hi list!
> 
> 1	PedExceptionOption
> 2	ped_exception_throw (PedExceptionType ex_type,
> 3			     PedExceptionOption ex_opts, const char* message, ...)
> 4	{
> 5		va_list		arg_list;
> 6		int result;
> 7		static int size = 1000;
> 
> 8		if (ex)
> 9			ped_exception_catch ();
> 
> 10		ex = (PedException*) malloc (sizeof (PedException));
> 11		if (!ex)
> 12			goto no_memory;
> 
> 13		ex->type = ex_type;
> 14		ex->options = ex_opts;
> 
> 15		while (message) {
> 16				ex->message = (char*) malloc (size * sizeof (char));
> 17				if (!ex->message)
> 18						goto no_memory;
> 
> 19				va_start (arg_list, message);
> 20				result = vsnprintf (ex->message, size, message, arg_list);
> 21				va_end (arg_list);
> 
> 22				if (result > -1 && result < size)
> 23						break;
> 
> 24				size += 10;
> 25				free (ex->message);
> 26		}
> 
> 27		return do_throw ();
> 
> If this function gets NULL in 'message' parameter, it will go into
> endless loop allocating memory because vsnprintf() on line 20 will
> keep returning -1 and thus the condition on line 22 will never be
> true.

Actually this is inaccurate.  I will never reach vsnprintf.  If message is NULL then the while will evaluate to false and continue on line 27.  However, the fact that we are calling the exception function without a message at all is a cause for worry.  This would mean that the user could potentially be misinformed or worse, not informed at all.

I propose a messages like the following:

"""
No correct volume label found in the device dev->name.
"""

Suggestions for the message are appreciated.

Regards.

> 
> There is at least one place where ped_exception_throw() is called
> with
> NULL : libparted/labels/dasd.c:243    [*]
> 
> I suggest to:
> a) PED_ASSERT(message != NULL) somewhere in ped_exception_throw()
> b) change the condition on line 22 to be just 'if (result < size)',
> because when vsnprintf() once returned -1, it will probably keep
> returning the same even with larger buffer.
> c) fix [*] to something more appropriate, such as:
> 
> ped_exception_throw(PED_EXCEPTION_ERROR, PED_EXCEPTION_OK,
>                    _("%s is corrupted"),
>                    dev->path);
> 
> Of course the 'is corrupted' text has to be changed to something
> better.
> 
> BTW isn't the while loop a typical case where realloc() would be more
> appropriate then malloc()/free() ?
> 
> 
> What do you guys think?
> 
> 
> -- 
> Best regards / s pozdravem
> 
> Petr Uzel, Packages maintainer
> ---------------------------------------------------------------------
> SUSE LINUX, s.r.o.                          e-mail: puzel at suse.cz
> Lihovarská 1060/12                          tel: +420 284 028 964
> 190 00 Prague 9                             fax: +420 284 028 951
> Czech Republic                              http://www.suse.cz
> 
> _______________________________________________
> parted-devel mailing list
> parted-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/parted-devel

-- 
Joel Andres Granados
Red Hat / Brno Czech Republic