Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChartUtils Crash on 32 Bit Devices #1737

Closed
ObjColumnist opened this issue Oct 28, 2016 · 13 comments
Closed

ChartUtils Crash on 32 Bit Devices #1737

ObjColumnist opened this issue Oct 28, 2016 · 13 comments

Comments

@ObjColumnist
Copy link

On 32 Bit Devices the function decimals in ChartUtils causes a bad memory access when the default formatter for a Chart is created.

    internal class func decimals(_ number: Double) -> Int
    {
        if number.isNaN || number.isInfinite || number == 0.0
        {
            return 0
        }

        let i = roundToNextSignificant(number: Double(number))

        if i.isInfinite
        {
            return 0
        }

        return Int(ceil(-log10(i))) + 2
    }

That is because i is NaN so the line return Int(ceil(-log10(i))) + 2 crashes.

ObjColumnist added a commit to DADco/Charts that referenced this issue Oct 28, 2016
Check that i is valid after rounding to prevent a crash.
@liuxuan30
Copy link
Member

liuxuan30 commented Oct 31, 2016

need more info - why it is NaN in the first place?

@danielgindi
Copy link
Collaborator

Problem is I have no 32bit device to test on. I don't see why would the value end up being NaN just because of 32 bit device.
What's the input number in your case?

@ObjColumnist
Copy link
Author

It happens when the chart creates its default formatter.

It was a huge number though, which might be an issue in itself.

I can check next time have access to a 32 Bit device (later in the week).

@jimmyjeng
Copy link

i run on iPad4

NSMutableArray *dataSets = [[NSMutableArray alloc] init];
LineChartData *data = [[LineChartData alloc] initWithDataSets:dataSets];
_chartView.data = data;

Then , the var magnitude in fun roundToNextSignificant would be 0 , so i will be NaN.

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 3, 2016

would you post more vars that lead magnitude to be 0? We don't know what your data entries are. And not giving enough info

@jimmyjeng
Copy link

i reproduce this crash in ChartsDemo by del one line in fun setDataCount of LineChart1ViewController.m .

        NSMutableArray *dataSets = [[NSMutableArray alloc] init];
//        [dataSets addObject:set1];
        LineChartData *data = [[LineChartData alloc] initWithDataSets:dataSets];
        _chartView.data = data;

then , the var pw Int -308 would lead magnitude to be 0 on iPad4.
let magnitude = pow(Double(10.0), Double(pw))

@spalmen
Copy link

spalmen commented Nov 4, 2016

I just encountered this crash today, in Charts 3, on an 9.2.1 iPhone 4S device, when ChartData contains an empty DataSet.
let lineChartYDataSet = LineChartDataSet(values: nil, label: nil)
lineChartView.data = LineChartData(dataSets: [ lineChartYDataSet ])

ChartViewBase.data setter:
// calculate how many digits are needed
setupDefaultFormatter(min: _data!.getYMin(), max: _data!.getYMax())

Because ChartData's DataSet is empty, its yMin and yMax are still DBL_MAX and -DBL_MAX, respectively.
ChartViewBase.setupDefaultFormatter(min: DBL_MAX, max: -DBL_MAX)
ChartData.entryCount is 0, so 'reference' is set to DBL_MAX.
reference = absMin > absMax ? absMin : absMax
. . .
// setup the formatter with a new number of digits
let digits = ChartUtils.decimals(reference)

ChartUtils.decimals(_ number: DBL_MAX)
'number' isn't NaN, or Infinite, or 0 so it evades the sanity check:
let i = roundToNextSignificant(number: Double(number))

ChartUtils.roundToNextSignificant(number: DBL_MAX) // number: 1.7976931348623157E+308
let d = ceil(log10(number < 0.0 ? -number : number)) // d: 309
let pw = 1 - Int(d) // pw: -308
let magnitude = pow(Double(10.0), Double(pw)) // magnitude: 0 <-- uhoh
let shifted = round(number * magnitude) // shifted: 0
return shifted / magnitude // NaN

Back in ChartUtils.decimals
if i.isInfinite // i: NaN
{
return 0
}
return Int(ceil(-log10(i))) + 2
fatal error: Double value cannot be converted to Int because it is either infinite or NaN

Interestingly, on a 9.3.5 iPhone 5s device (64-bit) and the Xcode 8.1 iPhone 4S (9.2) simulator, pow() with the same inputs returns a different result:
let magnitude = pow(Double(10.0), Double(pw)) // pw: -308, magnitude: 9.999999999999999E-309 <-- Abby Subnormal?
let shifted = round(number * magnitude) // shifted: 2
return shifted / magnitude // +Inf

But ChartUtils.decimals' 'if i.isInfinite' saves the day.

Unfortunately, I don't have a 32-bit device running iOS 10 to see how pow() behaves here.

As a hacky workaround, I found that adding a dummy DataEntry(x:0, y:0) to the DataSet (only public way I could find to set yMin and yMax to 0), then clearing the DataSet to remove it, before setting ChartView.data, avoids the crash.

(I also have to fuss with the LineChartView.xAxis to suppress the labels the axis renderer so helpfully wants to display for a empty dataset, but my usage is probably atypical. The chart displays realtime data and at this point, before I've collected any data points, I don't want any xAxis labels.)

I'm a Charts nooB, so I won't embarrass myself by suggesting where to guard against this DBL_MAX issue. :) Hope this helps…

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 7, 2016

Thanks for the info! Looks like the 32bit device has troubles.. @danielgindi

@brunce
Copy link

brunce commented Nov 17, 2016

I stumbled on this issue yesterday when creating and assigning an empty ChartData:

LineChartData *chartData = (LineChartData *)chartView.data;        
if (!chartData) {
    chartData = [[LineChartData alloc] init];
    chartView.data = chartData;
}

// create dataSets and add to chartData 

But when adding dataSets before assigning to chartData, everything works fine:

LineChartData *chartData = (LineChartData *)chartView.data;
if (!chartData) {
    chartData = [[LineChartData alloc] init];
}

// create dataSets and add to chartData

chartView.data = chartData;

Only crashed on the 32 bit, not on the 64 bit device.
Charts 3.0.0, iPad3, iOS 9.2

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 18, 2016

@brunce make sure you call chartView.data = chartData at last, because the data setter will invoke notifyDataSetChanged() and involve new calculation about the contentRect, axis property, matrix, etc. Setting chartData wildly would cause unexpected issues.

@ObjColumnist I think you made the same mistake. You shouldn't feed data when you only have empty array. Everything is like default DBL_MAX /DBL_MIN and on 64 devices, the i is then +inf, while you say it's NaN on 32 bit device. I will try to find a 32 bit device to test

@liuxuan30
Copy link
Member

liuxuan30 commented Nov 18, 2016

Alright, I found an iPhone 5 to test, it's indeed because the CPU arch, same as @spalmen:
32 bit:

(lldb) po pow(10.0, -308.0)
0.0

64bit:

(lldb) po pow(10.0, -308.0)
9.9999999999999991e-309

Then, due to the difference, shifted will be 2.0 on 64 bit, but 0 on 32 bit:
64 bit

(lldb) po number
1.7976931348623157e+308

(lldb) po magnitude
9.9999999999999991e-309

(lldb) po shifted
2.0

(lldb) po shifted/magnitude
inf

32 bit

(lldb) po number
1.7976931348623157e+308

(lldb) po magnitude
0.0

(lldb) po shifted
0.0

(lldb) po shifted/magnitude
nan

@danielgindi I will merge that PR. I tested it on iPhone 5, should be good.

jean-mouton-git pushed a commit to SBGMobile/Charts that referenced this issue Mar 2, 2017
Added a check against NaN, fix ChartsOrg#1737
SLIV3RZA pushed a commit to SBGMobile/Charts that referenced this issue Mar 2, 2017
Added a check against NaN, fix ChartsOrg#1737
@acegreen
Copy link

This is [still] present on 64bit devices (iPhone 6S plus)

@liuxuan30
Copy link
Member

@acegreen please give proofs how this is still present like I tested above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants