[Libreoffice] [REVIEW] fix for a chart crasher

[Kevin Hunter]

At 4:22am -0500 Thu, 03 Mar 2011, Tor Lillqvist wrote:
>> for a cherry-pick candidate to libreoffice-3-3. It's a very simple&
>> safe fix, and fixes a crasher as reported in
>
> But do you know how those values can end up being negative in the
> first place, is that a symptom of something being horribly wrong
> otherwise, and this patch now then just plasters over that and
> instead of crashing it might permit silent data corruption elsewhere
> then instead?

Alas, in my limited 20 minutes last night I was not able to get the full 
scope in my mind, so I pose it as a question here instead:  I'm a little 
concerned with /why/ either of those two functions are returning a 
negative number at all.  (I certainly didn't give LO one in the input!) 
  Is there ever a time that either of those should be negative?  The 
rest of the code in getAllTicks() assumes that they're always >= 0.

At first glance, getTickDepth() and getMaxTickCount() read to me as 
requests for non-negative values (from an English/math sense).  However, 
I note that the return values are sal_Int32 instead of sal_uInt32.

Going one call down in the stack trace:

sal_Int32 TickmarkHelper::getTickDepth() const
{
     return m_rIncrement.SubIncrements.getLength() + 1;
}

If that's truly a length (where are the frickin' data definitions?!?!), 
then, sans a wraparound, that certainly shouldn't be negative.  Surely 
we aren't at MAX_INT?

sal_Int32 TickmarkHelper::getMaxTickCount( sal_Int32 nDepth ) const

Again, I've run out of time, but my spidey sense is pointing to a 
combination of "too small a range" = close enough to zero for the 
computer to interpret as 0 or returning a negative "almost 0", and then 
line 484 or 495:

484: sal_Int32 nIntervalCount = static_cast<sal_Int32>( fSub / 
m_rIncrement.Distance );

I have not yet sussed out what approxSub() or isFinite() does to fSub. 
Are they making nIntervalCount negative, through the fSub proxy?

495: nTickCount = nIntervalCount * 
(m_rIncrement.SubIncrements[nDepth-1].IntervalCount-1);

If it's 495, I don't know how it's getting past the if above.

Thoughts?

Kevin