Comment 71 for bug 26650

Message-ID: <email address hidden>
Date: Mon, 12 Dec 2005 15:51:09 +0100
From: Martin Schulze <email address hidden>
To: Martin Pitt <email address hidden>
Cc: Frank =?iso-8859-1?Q?K=FCster?= <email address hidden>,
 <email address hidden>, Florian Weimer <email address hidden>
Subject: Re: Bug#342292: Fwd: Re: [vendor-sec] xpdf update - patch wrong?

Martin Pitt wrote:
> > > After discovering that the same flawed multiplication is also present
> > > in upstream's other two patches, I decided to completely rework the
> > > patch.
> > >
> > > I attach the debdiff with separated out changelog. Florian, maybe you
> > > can peer-review the patch?
> >
> > Martin and Florian, Joey Schulze also sent a "fixed" patch to the bug,
> > see http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=342292;msg=131
> >
> > Would you be so kind and review it?
>
> Sorry for the delay, lots of private stuff to do on the weekend.
>
> + nVals = width * nComps;
> ++ totalBits = nVals * nBits;
> ++ if (totalBits == 0 ||
> ++ (totalBits / nBits) / nComps != width ||
> ++ totalBits + 7 < 0) {
> ++ return;
> ++ }
>
> Please do not use this part of Joey's patch. As already disdussed,
> this way of checking a multiplication overflow is unreliable. Please
> use the var1 >= INT_MAX/var2 approach, which is the 'standard way' and
> avoids integer overflows.

Sorry, that slipped through from one of the load of different patches.

It's not that problematic, the line

> ++ (totalBits / nBits) / nComps != width ||

might be optimised by the compiler, but it's not a problem since the
proper check is above the code (at least in my local copy):

+ nComps >= INT_MAX/nBits ||
+ width >= INT_MAX/nComps/nBits) {

Regards,

 Joey

--
If nothing changes, everything will remain the same. -- Barne's Law

Please always Cc to me when replying to me on the lists.