[pacman-dev] [PATCH v2] pacman/upgrade: Check malloc() return value

[Sören Brinkmann]

On Tue, 2014-03-11 at 09:57AM -0400, Andrew Gregory wrote:
> On 03/10/14 at 07:32pm, Sören Brinkmann wrote:
> > Hi Andrew,
> > 
> > On Mon, 2014-03-10 at 10:00AM -0400, Andrew Gregory wrote:
> > > On 03/09/14 at 03:47pm, Sören Brinkmann wrote:
> > > > Check the return value of malloc() before dereferencing the returned pointer.
> > > > 
> > > > Signed-off-by: Sören Brinkmann <soeren.brinkmann at gmail.com>
> > > > ---
> > > > v2:
> > > >  - add print statement to log the error
> > > > ---
> > > >  src/pacman/upgrade.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c
> > > > index 5416f6180b39..11da4dcb5e53 100644
> > > > --- a/src/pacman/upgrade.c
> > > > +++ b/src/pacman/upgrade.c
> > > > @@ -51,6 +51,10 @@ int pacman_upgrade(alpm_list_t *targets)
> > > >  	 */
> > > >  	for(i = targets; i; i = alpm_list_next(i)) {
> > > >  		int *r = malloc(sizeof(int));
> > > > +		if(r == NULL) {
> > > > +			pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n"));
> > > > +			return 1;
> > > > +		}
> > > 
> > > If you move the malloc outside the loop and make it an array we have
> > > a better chance of hitting this early enough that we still have
> > > a little memory left for cleanup.
> > 
> > I think that is not done that easily. Actually not without some
> > significant rewrite of this code. Those allocated rs go into an
> > alpm_list_t which is eventually freed using FREELIST. Doing the memory
> > management differently here is not that straight forward.
> > 
> > 	Sören
> 
> The resulting list is only used in one place and isn't passed to any
> other functions.  It would be trivial to convert it to a standard
> C array.  Leaving it as-is with a list instead of an array leaves this
> function still vulnerable to a malloc failure from alpm_list_add.  The
> only thing the list is used for is to set the siglevel, so you could
> even store the siglevels directly in the array instead of integer
> flags.

Honestly, memory management and convoluted error paths seem to be a
general problem in this code base.
I see what I can do. You're probably right and it's not as a big task as
I originally thought to get things at least slightly improved.

	Sören