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

[Andrew Gregory]

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.

apg